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

[3.x] Enable setting of collision iterations in Physics2DServer #38387

Merged
merged 1 commit into from
May 19, 2021

Conversation

Rhathe
Copy link
Contributor

@Rhathe Rhathe commented May 1, 2020

This allows fine-tuning of collision iterations for more accurate collision physics with a performance cost.

Reading through the physics code it seems to be using sequential impulses, so even with higher iterations it's not guaranteed to be completely reliable, but it will help especially if you know your scenes will have a high number of stacking collisions.

Mitigates #2092. Found that the current iteration number of 8 leads to the bug immediately on load, setting it to 12 with Physics2DServer.set_collision_iterations(12) makes it not collapse immediately on my machine.

May help with #38339.

3.x version of #48827

This allows fine-tuning of collision iterations for more
accurate collision physics with a performance cost.
@Rhathe Rhathe requested a review from a team as a code owner May 1, 2020 14:17
@Rhathe
Copy link
Contributor Author

Rhathe commented May 1, 2020

A more advanced approach would be to add a feature for adaptive collision iterations i.e. determining the number of iterations based on how many collisions there are/how fast objects are moving away from each other. That way collisions with lots of movement don't need that many iterations to work while mostly static collisions, like stacked boxes, need many iterations to help stabilize.

Another option is to implement shock propagation, where rigid body collisions are ordered by position relative to collisions from forces against a static object, i.e. boxes from bottom to top due to a floor and gravity. There you treat the bottom box as an object with infinite mass with regard to linear velocity impulses and calculate the collisions from there.

@Calinou
Copy link
Member

Calinou commented May 1, 2020

Is this related to godotengine/godot-proposals#204? (To close the proposal, this would need to be added to 3D physics as well.)

@Rhathe
Copy link
Contributor Author

Rhathe commented May 1, 2020

Yeah it does seem related, though it seems that not only does this need to be added to 3D physics but also to the UI as well. Currently it can only be done by setting the server singleton's value in script.

I also haven't looked too much into it but it seems bullets have their own separate physics system?

@madmiraal
Copy link
Contributor

I think this would make more sense as a Project Setting under Physics. I wouldn't expect this to be something that would need to be changed at runtime.

@Rhathe
Copy link
Contributor Author

Rhathe commented Jun 7, 2020

Enabling a change of iterations at runtime can allow you to build an adaptive collision iteration system on top of it, as explained in my previous comment. If you know a particular frame has mostly dynamic objects at high speeds, you may want want to set the iterations low. If on the other hand you have a frame with a lot of mostly static bodies, higher iterations may be needed to keep things stable.

@Calinou Calinou added this to the 3.2 milestone Jan 5, 2021
@Rhathe Rhathe requested a review from a team as a code owner March 12, 2021 12:26
Base automatically changed from 3.2 to 3.x March 16, 2021 11:11
@aaronfranke aaronfranke modified the milestones: 3.2, 3.3 Mar 16, 2021
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Looks good to me, but there should be a project setting to change this value without having to use a script. This can be merged to 3.3 safely as it won't impact existing functionality.

This PR doesn't add the feature to 3D, but I think having this in 2D only is better than not having it at all.

@pouleyKetchoupp Any comments?

@pouleyKetchoupp
Copy link
Contributor

Yeah the change looks good, although the PR should be done against master and cherry-picked for 3.3 since it affects only 2D (in 3D some file names have changed so it requires separate PRs).

The same change in 3D and project settings would make a lot of sense but these can be added later.

@e344fde6bf
Copy link
Contributor

Would it make sense to have this as a per-object property? Then when constraint islands are generated, the max number of iterations out of all the objects in the island could be used for solving it.

@pouleyKetchoupp
Copy link
Contributor

Would it make sense to have this as a per-object property? Then when constraint islands are generated, the max number of iterations out of all the objects in the island could be used for solving it.

It might be useful in specific cases, but it's hard to say. It makes sense to start with a global value so it's easy to set, then possibly a separate proposal for per-object settings if it's needed in some use cases.

@akien-mga akien-mga modified the milestones: 3.3, 3.4 Mar 26, 2021
@akien-mga
Copy link
Member

So IIUC this is pending on a master counterpart before this can be merged?

@pouleyKetchoupp
Copy link
Contributor

So IIUC this is pending on a master counterpart before this can be merged?

Yeah, there's no reason for this change to be 3.x specific.

@Rhathe Rhathe changed the base branch from 3.x to master May 18, 2021 22:19
@Rhathe Rhathe requested review from a team as code owners May 18, 2021 22:19
@pouleyKetchoupp pouleyKetchoupp removed request for a team May 18, 2021 22:21
@YeldhamDev YeldhamDev removed request for a team May 18, 2021 22:21
@pouleyKetchoupp
Copy link
Contributor

@Rhathe Switching the branch directly doesn't work too well, better to open a new PR on master.

@akien-mga akien-mga changed the title Enable setting of collision iterations in Physics2DServer [3.x] Enable setting of collision iterations in Physics2DServer May 19, 2021
@akien-mga akien-mga merged commit cca2a9d into godotengine:3.x May 19, 2021
@akien-mga
Copy link
Member

Thanks!

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.

8 participants