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

Physics interpolation - Move out of Scenario #60147

Merged
merged 1 commit into from
Apr 13, 2022

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Apr 11, 2022

Move VisualServer interpolation data out of Scenario and into VisualServerScene, so the interpolation data and enabled status is now common to all Scenarios.

Fix physics interpolation in multithreaded mode by ensuring tick and pre-draw are called.

Fixes #59736
Alternative to #59875

Notes

  • When fixing the multithreading rendering, @reduz suggested we should move the interpolation data out of Scenario and have it global, so there is a global on or off. He suggested there should usually only be one SceneTree, and this should fit 99% use cases.
  • This was actually the arrangement originally when developing the interpolation, but @jordo was keen to have it selectable per SceneTree for his game (further description is in Physics interpolation - Fix multithreaded rendering #59875 (comment) )
  • An alternative might be for SceneTrees to create a RID in the VisualServer and hang the interpolation data of this, thus making it selectable per SceneTree (there is currently no concept of a SceneTree in the VisualServer afaik)
  • I'm happy to go with any of these approaches, this will have to be decided by jordo and reduz, I'm staying out of it as I don't have strong opinions 😁 . It is fairly easy for me to change either way.
  • Just to be clear in this version, the interpolation data is hanging off of VisualServerScene thus is only global in the sense of VisualServer being a singleton, and thus if @jordo is creating several VisualServers, he may be able to independently turn interpolation on and off
    using the VisualServer::set_physics_interpolation_enabled(bool) function. I don't know enough about his version to know if this would work for his purpose.

Move VisualServer interpolation data out of Scenario and into VisualServerScene, so the interpolation data and enabled status is now common to all Scenarios.

Fix physics interpolation in multithreaded mode by ensuring tick and pre-draw are called.
@lawnjelly
Copy link
Member Author

Feedback from reduz:

  • The global approach here is fine, don't change to the SceneTree approach as it is too fringe case
  • Look and see if we can possibly remove the pre_draw() call, and instead disable the interpolation when in modes where this might be a problem (low processor mode, possibly more).
    (The pre_draw() is essentially there currently for the case where the game is ticked but draw() is not called, and makes sure there are no leaks in the interpolation lists, which could cause high memory use / crash.)
  • Possibly look at self_lists as another way of achieving the same (I did already use this approach in one of the earlier PRs, I think there were problems, but I can re-evaluate)

Given that the latter two could introduce new bugs, and this PR currently fixes the outstanding multitthreading bug with (hopefully) no other side effects, we should probably also consider merging this in the meantime so we have a solid build in the current 3.5. The latter two may take a bit of time to come up with good improvements (as they are essentially housekeeping changes), and probably deserve to be considered in separate PRs where they can be examined in more detail.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go with this for 3.5 beta 4 as discussed.

@akien-mga akien-mga merged commit e1eb3c2 into godotengine:3.x Apr 13, 2022
@akien-mga
Copy link
Member

Thanks!

@oeleo1
Copy link

oeleo1 commented Apr 13, 2022

@lawnjelly Cool stuff. Does this men that now it be used from 2D as well?

Unfortunately it looks like all the major sexy features of the recent 3.x betas like this one and ubershader don’t apply to a 2D/gles2 setup for me to try out on an advanced project.

@Calinou
Copy link
Member

Calinou commented Apr 13, 2022

Does this men that now it be used from 2D as well?

Not yet – physics interpolation still needs to be ported to the 2D physics engine.

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

Successfully merging this pull request may close these issues.

4 participants