Skip to content

Physics interpolation - Move out of Scenario#60147

Merged
akien-mga merged 1 commit intogodotengine:3.xfrom
lawnjelly:interpolation_out_of_scenario
Apr 13, 2022
Merged

Physics interpolation - Move out of Scenario#60147
akien-mga merged 1 commit intogodotengine:3.xfrom
lawnjelly:interpolation_out_of_scenario

Conversation

@lawnjelly
Copy link
Copy Markdown
Member

@lawnjelly lawnjelly commented Apr 11, 2022 •
edited
Loading

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.

@lawnjelly
Physics interpolation - Move out of Scenario
d2b1d29 
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 lawnjelly requested review from a team as code owners April 11, 2022 17:00
@Calinou Calinou added this to the 3.5 milestone Apr 11, 2022
@lawnjelly
Copy link
Copy Markdown
Member Author

lawnjelly commented Apr 13, 2022

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.

akien-mga
akien-mga approved these changes Apr 13, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

akien-mga commented Apr 13, 2022

Thanks!

@akien-mga akien-mga mentioned this pull request Apr 13, 2022
Closed
@lawnjelly lawnjelly deleted the interpolation_out_of_scenario branch April 13, 2022 18:31
@oeleo1
Copy link
Copy Markdown

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
Copy Markdown
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.

@lawnjelly lawnjelly mentioned this pull request Apr 13, 2022
Closed
@rburing rburing mentioned this pull request May 10, 2024
Merged
3 tasks
@rburing rburing mentioned this pull request May 26, 2024
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@akien-mga akien-mga akien-mga approved these changes

Assignees

No one assigned

Labels

enhancement topic:physics topic:3d

Projects

None yet

Milestone

3.5

Development

Successfully merging this pull request may close these issues.

4 participants

@lawnjelly @akien-mga @oeleo1 @Calinou