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

refactor(physics_timescale): Remove physics timestep scaled by 60 + update velocities to keep behavior #925

Merged
merged 5 commits into from
Mar 20, 2024

Conversation

MaxCWhitehead
Copy link
Collaborator

@MaxCWhitehead MaxCWhitehead commented Mar 20, 2024

Overview:
Delta time in physics update previously had a scale of 60 due to a prior fix regarding timestep.

This change removes the scaling of physics timestep by 60, and adjusts velocities / forces (gravity) to account for this so behavior does not change.

Impact: Will help with ragdoll work I am doing, velocity metadata can be used with objects simulated in rapier and on our kinematics without having to get messy scaling them back and forth, or dealing with a very large timestep in rapier (which did not go well in my experiments).

units of velocity and gravity now relate more intuitively to position + sizes of objects in terms of setting velocity based on how far a contributor may want something to move per second.

Changes:

  • velocities all scaled up by 60, gravity scaled up by 3600 (60*60) as this is multiplied by time twice.
    • Gravity values are now quite large, unless we want to change unit of gravity and add back in a magic scale factor will have to roll with this I guess. I think it's ok this way, new contributors may reference global gravity metadata to get an idea of what to set a specific body's gravity to.
  • apply_rotation now uses delta time (kinematic rotations should now be consistent regardless of frame rate)
  • Fix bullet position update from velocity not being scaled by delta time
  • Fix setting velocity.y = 0 when grounded comparing against global gravity (now corrected to use body's gravity)
  • Fix sproinger velocity condition including gravity incorrectly (don't think we need gravity in this at all)

Other Notes:
I am hoping I caught all of the values that needed updating. Testing movement / jump, throwing items, sproinger, bomb bounces, it looks right to me.

  • One thing I noticed is that some item metadata includes a "angular_velocity" value which is used as spin factor for item throw. Spin scales based on the throw velocity, so these spin values do not need to be updated. This is more of a spin factor than a true angular velocity, may consider renaming in future.
  • There are a few values hard coded that we could pull into metadata, but I didn't want to make footprint of change any larger than necessary.
  • some metadata for player movement is duplicated between player skin metadata, may want to pull this out to single metadata location in future.

grounded objects

This was using global gravity when should use KinematicBody gravity
velocity was compared with gravity incorrectly
update velocities / accelerations to account for this

velocity scaled by 60
gravity scaled by 3600

some meta values titled "angular_velocity" were not touched as these are
used for spin on throws, which is already scaled by throw velocity.
Copy link
Member

@zicklag zicklag left a comment

Choose a reason for hiding this comment

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

👌

@zicklag zicklag added this pull request to the merge queue Mar 20, 2024
Merged via the queue into fishfolk:main with commit b6b462c Mar 20, 2024
9 of 10 checks passed
@MaxCWhitehead MaxCWhitehead deleted the fix-physics-time-scale branch March 21, 2024 01:23
github-merge-queue bot pushed a commit that referenced this pull request Apr 4, 2024
…ng incorrect post-physics refactor (#957)

Found a few velocity-based meta values I missed in the #925 refactor.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants