Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a mechanism to declare capability dependencies at widget/gadget GetInfo #4034

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

saurtron
Copy link
Collaborator

@saurtron saurtron commented Dec 18, 2024

Work done

  • Add a mechanism to declare capability dependencies at widgets and gadgets
    • Through the new 'depends' field.
    • For now added: gl, gl4, shaders, fbo.
    • This makes the widgets and gadgets automatically inactive if capabilities not present.
  • Extends the Platform module for this.
    • Adds Platform.check(capabilities), also Platform.gl, Platform.gl4, Platform.glHaveFBO.
      • These also allow testing for the features partially inside modules in a cleaner way than the current way of gl.CreateShader or similar, and it can be a central place where the tests can be improved.

Remarks

Looking forward to hearing ideas for improving this PR.

  • This allows cleaning a lot of checks from many widgets, also making those less error prone.
    • In many situations the tests aren't there, or they're at the wrong place.
    • A common error is testing at Initialize but not at Shutdown, this can make Shutdown err.
    • Current situation is bad for automatic testing since there is no gl there and that triggers a few errors (noticed it while preparingfor automated testing)
  • The Platform module is just available for unsynced, but atm all the methods are just needed at unsynced
    • I was initially going to add a Capabilities module, but then seen the Platform module and thought better expand on that.
    • If we need this for synced could also create Platform for unsynced.
  • The new attributes inside Platform combine Vendor and lua tests for best results. For example right now Platform.glHaveGLSL always returns true even when gl.CreateShader isn't defined, like for headless, but it's conceivable in the future it could be oposite, and have situations where Platform.glHaveGLSL is false but gl.CreateShader is defined.
  • The Platform module is currently not used much, but imo it's the way to go specially if we add a few things there besides the ones the engine is already adding.

Examples

There are a few examples already added in this PR, but works like this:

function widget:GetInfo()
  return {
    name      = "DrawUnitShape GL4",
    version   = "v0.2",
    desc      = "Faster gl.UnitShape, Use WG.UnitShapeGL4",
    author    = "ivand, Beherith",
    date      = "2021.11.04",
        license   = "GNU GPL, v2 or later",
    layer     = -9999,
    enabled   = true,
    depends   = {'gl4'}, --   <----- this one
  }
end

@WatchTheFort
Copy link
Member

Is this always an error condition, or only sometimes an error condition? Does this risk hiding errors?

@saurtron
Copy link
Collaborator Author

saurtron commented Dec 18, 2024

Is this always an error condition, or only sometimes an error condition? Does this risk hiding errors?

It simply doesn't allow inserting/removing the widget when it doesn't meet the required capabilities. Doesn't work by hiding errors or with error conditions. It works by checking through the Platform module and existing lua methods.

For example if you say a widget requires `gl4' then before insertion the system will check if gl4 is actually available, and otherwise just not insert it (currently will appear yellow at the widget selector, and still allow enabling disabling, but it will just not completely enable, ie, never trigger Initialize or Shutdown, and also won't register callins).

Generally the capabilities test just does the same thing those widgets are doing atm. Just cleaner and less error prone imo.

@WatchTheFort
Copy link
Member

Not sure in what way it could hide errors, maybe in case capability test would be wrong?

From silently swallowing the error, with no feedback in game or in the log. Better to use Spring.Log with LOG.ERROR, rather than Spring.Echo

@saurtron
Copy link
Collaborator Author

Not sure in what way it could hide errors, maybe in case capability test would be wrong?

From silently swallowing the error, with no feedback in game or in the log. Better to use Spring.Log with LOG.ERROR, rather than Spring.Echo

But there is no error, we're just avoiding to activate widgets where they have known dependencies to gl.CreateShader for example.

Check the diff and you will see how all the ones where I implemented it were just silently removing themselves. At least now it's printing something, but I don't think reporting an error would be appropriate.

If we want to print an error for people that don't match certain capabilities then we can use Platform also to show something prominent instead.

@saurtron
Copy link
Collaborator Author

saurtron commented Dec 18, 2024

From silently swallowing the error, with no feedback in game or in the log. Better to use Spring.Log with LOG.ERROR, rather than Spring.Echo

Anyways the good thing with this approach, is once it's implemented everywhere it's simple to change the policy for everything in one go, since you have one central place where to change that. It's just one line a ninja commit away.

Now you need to go searching for 10s of widgets, some are throwing errors, others are not, some crashing...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants