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] Bullet: Revert "Fixed gravity scale" #67842

Open
wants to merge 1 commit into
base: 3.x
Choose a base branch
from

Conversation

Klowner
Copy link
Contributor

@Klowner Klowner commented Oct 24, 2022

This reverts commit d250ade which appears to have been made in error? (Looks suspicious, commented out line followed by a line which ignores the parameters in favor of a hard-coded zero vector).

Aside from ensuring the gravity vector is normalized (since it gets multiplied by gravityMagnitude), I don't see how reverting this would cause any issues.

Fixes #32196

@Klowner Klowner requested a review from a team as a code owner October 24, 2022 16:40
@Calinou Calinou added bug topic:physics topic:3d cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release labels Oct 24, 2022
@Calinou Calinou added this to the 3.6 milestone Oct 24, 2022
@clayjohn clayjohn requested a review from rburing October 24, 2022 18:29
@Klowner Klowner force-pushed the bullet-runtime-gravity-32196 branch from d8db8bb to 4354db5 Compare October 31, 2022 18:06
@rburing rburing requested a review from AndreaCatania October 31, 2022 18:07
@Klowner
Copy link
Contributor Author

Klowner commented Nov 15, 2022

Also fixes #35378

@akien-mga
Copy link
Member

akien-mga commented Nov 15, 2022

This reverts commit d250ade which appears to have been made in error? (Looks suspicious, commented out line followed by a line which ignores the parameters in favor of a hard-coded zero vector).

See the PR which includes this commit: #13046.

It states:

Can you confirm that this does not reintroduce #12770?

Also fixes #35378

Please list it in the OP so that the issue can properly be identified as linked / closed when the PR is merged (this actually doesn't work when merging PRs in non-master branch but it's for the principle/consistency :)).

@Klowner
Copy link
Contributor Author

Klowner commented Nov 15, 2022

Bah, thank you, I was unaware of that one.

Looks like this does trigger that regression. Gravity must be affecting the body's velocity for a tick or two before the gravity scale setter is called.

I'll do a little more poking.

@akien-mga
Copy link
Member

Did you have any luck looking into this further?

@akien-mga akien-mga changed the title Bullet: Revert "Fixed gravity scale" / Fix #32196 [3.x] Bullet: Revert "Fixed gravity scale" / Fix #32196 Aug 18, 2023
@Klowner
Copy link
Contributor Author

Klowner commented Aug 18, 2023

Thanks for pinging me on this! I looked at this again today with (very) fresh eyes and I think I understand the problem but I don't have very strong feelings about any potential solutions.

The problem with the regression is due to the order of things being initialized, kinda:

  1. Bullet Space is constructed
  2. Gravity is set on Bullet Space (without this change, this gravity is always set to (0,0,0))
  3. RigidBody is created
  4. RigidBody gravity scale setter is called (and sets a "scratched" flag to indicate that changes have been made, fine)
  5. RigidBody is added to the Bullet Space and the Bullet Space automatically calls body->setGravity() with the Space's gravity that was initially set in step 2 ⚠️ the RigidBody's gravity scale is not accounted for. ⚠️
  6. Physics simulation tick occurs
  7. RigidBody's dispatch_callbacks() is called which detects the "scratched" flag from step 4 and then calculates a new gravity which gets multiplied by the RigidBody's gravity scale before calling body->setGravity().
  8. Second physics tick is performed, and our RigidBody with gravity now set to zero already has 1 physics's tick worth of gravitational velocity applied, so it takes a few more frames to settle.

One idea I had was to set the RigidBody's gravity after it's added to a space to overwrite the incorrect value. Seems kinda dumb but it seems like that might be the only way to include the body's gravity scale? Any input appreciated :)

@lawnjelly lawnjelly modified the milestones: 3.6, 3.7 Sep 11, 2024
@AThousandShips AThousandShips changed the title [3.x] Bullet: Revert "Fixed gravity scale" / Fix #32196 [3.x] Bullet: Revert "Fixed gravity scale" Sep 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release needs work topic:physics topic:3d
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants