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

Deterministic constraint map in Godot Physics solver #48497

Closed

Conversation

pouleyKetchoupp
Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp commented May 6, 2021

This PR iterates on #44112 by @jordo, which made 2D physics more deterministic. This PR changes the container from a List back to a Map because it was causing a drop in performance, and applies the same change to all types of physics objects (areas, rigid bodies, soft bodies) in both 2D and 3D physics.

Fixes #48303

Detailed list of changes:

  • Common constraint map in CollisionObjectSW instead of separate containers for rigid body, soft body, area
  • Removed index in rigid body constraints (it was only used to test bodies from a constraint against a specific body)
  • Added incremental constraint id as key in constraint map (instead of pointer) to help with determinism
  • In 2D, this system replaces the previous List implementation which was making the solver slower (by 30% in some cases)

@jordo: Could you please test again with your project on 4.0 to make sure it still works for you?
With your previous PR, I wasn't seeing any change in determinism in either 3.x or 4.0 (I had tested in debug only).
With this one, I can see a difference in the 3.x version (debug and release_debug) but not in 4.0.
There must be something happening with my compiler that might need more investigation later, but I'd like to make sure there's at least no regression.

Test project:
physics-2d.zip

@pouleyKetchoupp pouleyKetchoupp added this to the 4.0 milestone May 6, 2021
@pouleyKetchoupp pouleyKetchoupp requested a review from a team May 6, 2021 00:17
- Common constraint map in CollisionObject instead of separate ones in
rigid body, soft body, area
- Removed index storage in rigid body constraints (it was only used to
test bodies from a constraint against a specific body)
- Added incremental constraint id as key in constraint map to help with
determinism
- In 2D, this system replaces the previous List implementation which was
making the solver slower (by 30% in some cases)
@reduz
Copy link
Member

reduz commented Sep 4, 2021

I think this approach may be likely too much, as Set<> is more expensive to use and not so cache efficient, and keeping indices is also troublesome .

IMO to ensure determinism instead of using a numerical ID, we should simply use creation time for iteration, for which I think OrderedHashMap is likely a better, simpler and faster solution for the bookkeeping of pairs.

@jordo
Copy link
Contributor

jordo commented Sep 4, 2021

I would suggest not to use cpu clocktime anywhere in the physics engine. The idea is to be able to re-create the same internal state in the simulation regardless of when the simulation is run.

Creation time as a dependency for a physics simulation seems unnecessary, and my guess will lead to issues down the road. I think an int counter will be much more performant. I actually really support what @pouleyKetchoupp has done with ConstaintMap here, as constraint_id_counter.increment() will be more performant than asking for cpu clocktime wrt ID generation.

Iterating the constraint map is fast, which is the hot path no? And insertion/deletion will both be Olog(n).

OrderedHashMap is ordered by insertion order, not by key, and as well, it manages an internal list itself, so if List is a performance issue, just note that the internal implementation of OrderedHashMap maintains a list as well. erase() on OrderedHashMap will also be O(n), as it will scan it's internal list upon removal of an element.

@reduz
Copy link
Member

reduz commented Jul 29, 2022

HashSet is extremely fast to iterate now and unlike RBSet is deterministic, so this should warrant another look. I will try to check it out before RC.

@YuriSizov YuriSizov modified the milestones: 4.0, 4.1 Feb 9, 2023
@YuriSizov YuriSizov modified the milestones: 4.1, 4.2 Jun 14, 2023
@aaronfranke aaronfranke modified the milestones: 4.2, 4.x Oct 7, 2023
@aaronfranke aaronfranke marked this pull request as draft October 7, 2023 19:41
@aaronfranke
Copy link
Member

@pouleyKetchoupp This PR needs to be rebased before it can be considered, it has not been updated in over 2 years and it has conflicts that need resolving.

@pouleyKetchoupp
Copy link
Contributor Author

Just to be clear, I've been bullied two times by different people on this project and every time project management reacted by siding with the bully, so I've left and I'm never going to contribute again.

@aaronfranke
Copy link
Member

@pouleyKetchoupp I'm sorry to hear about that, I was not aware. Does this mean we should close your other pull requests as well, since they will not be worked on further?

@pouleyKetchoupp
Copy link
Contributor Author

Sure, you can close them

@aaronfranke aaronfranke removed this from the 4.x milestone Oct 7, 2023
@jordo
Copy link
Contributor

jordo commented Oct 8, 2023

@pouleyKetchoupp Ah that's super disappointing to hear... I was not aware either, I had always appreciated the work you put into the physics improvements as that has been a major pain point for our team. Always wondered where there was a sudden drop-off in your contributions after contributing so much... Sorry you had to have that experience.

@coppolaemilio
Copy link
Member

The matter reported was addressed by the Code of Conduct (CoC) team at the time, who took appropriate actions in accordance with our policies. The process might have been slow, because after receiving the reports, the CoC team took the time to investigate it (by talking to the involved parties), and tried to mediate and give proper warnings beforehand. When the mediation and the warnings turned out to be unsuccessful, the offending party was removed from the project.

Since then, the CoC team has expanded with new members in order to improve the reaction speed to reports.

If, however, anyone involved feels that there are any unresolved issues or concerns related to this matter or any other, please do not hesitate to report them to our CoC team.

@pouleyKetchoupp
Copy link
Contributor Author

I wasn't going to get into more details, but since you are, let me make the story straight.

Here are my concerns:

  • The first time, I was bullied for months by an individual while contracting for the project. He was also bullying other contributors. I did report issues multiple times to project management to no avail.
    What you call meditation was a meeting with the project manager and the bully, during which I was asked to apologize to the bully, while he was yelling at me.
    For the next 6 months, absolutely nothing was done.
    When I tried to raise the issue to the head of the project, he told me it was a sad situation but we can't reject anybody from the project.
    What it took to get the toxic person removed from project responsibilities was for me to gather all evidence and present them to the PLC, after he had started to bully yet another person. He was never completely banned from the project.

  • The second time, a similar situation started to happen with a different bully. It was at W4 games, so the project CoC wasn't involved, but the same project managers handled the situation in the same dismissive way. That's why I left.

I hope things can one day get better on this project so other people don't go through the same experience I did. For that project managers need to take situations like those seriously, which in my experience wasn't the case even after the CoC team was created.

@YuriSizov
Copy link
Contributor

YuriSizov commented Oct 9, 2023

I don't think this is the place to discuss such things, especially if your concerns are, at least partially, related to some third party business. Please reach out to the CoC team or the project leadership if you think you've not been treated fairly. There seems to be a wild difference between how various participants see the events that you are referring to, but it's not going to get resolved by throwing public accusations. I advised the project and I'm advising you to discuss these matters privately and reach an understanding.

We were prompted to respond to your initial comment because of its nature, but furthering down on this is not a road that is going to lead anywhere. I hope you understand why this is not something that I see as appropriate means to make yourself heard. I know for a fact, personally, how project reacts to bullying of project members, and what you describe is not something that I can confirm in any way. This makes me think that you feel very wronged. So I ask again to reach out to the CoC team or the project leadership.

I'm going to lock this PR to avoid further speculations.

@godotengine godotengine locked as off-topic and limited conversation to collaborators Oct 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Godot Physics 3D constraint solving more deterministic
7 participants