feat(physics): rework Bepu Body2DComponent and remove the unused _2D project - #3349
feat(physics): rework Bepu Body2DComponent and remove the unused _2D project#3349VaclavElias wants to merge 7 commits into
Conversation
- Deleted all source files and project file for Stride.BepuPhysics._2D - Removed 2D physics component implementation and module initializer
- Implement dynamic body component constrained to XY plane - Lock rotation about X and Y axes, correct Z drift with velocity - Adjust hull collider contact settings for 2D - Expose configurable ZTolerance property - Integrate with simulation update loop - Add XML documentation throughout
- Warn about hull colliders capping/raising physics properties - Add Display attribute to ZTolerance for editor integration
- Update and condense XML docs for readability and focus - Refine remarks to explain constraints, correction, and tuning - Remove lengthy implementation details and engine comparisons - No functional code changes made
- Add tests for constructor defaults and ZTolerance validation - Test simulation plane constraint and Z=0 convergence - Add tests for sleep after settling and rotation lock persistence
|
🤖 Draft PR — automatic CI is skipped to save runner minutes.
|
Ethereal77
left a comment
There was a problem hiding this comment.
You invoked me, and I'm here 😁
I've left some comments. Overall looks good to me, although I've not touched much physics code in a long time.
| /// <summary> | ||
| /// Gets or sets how far the body may drift off the Z = 0 plane before it is pulled back, in world | ||
| /// units. Defaults to 0.001 (one millimetre at Stride's default scale). | ||
| /// </summary> |
There was a problem hiding this comment.
I'm not very into physics simulation, but does this not mean that objects are loosing energy?
I mean, if the simulation gives an object a certain velocity, and that velocity includes a non-zero Z component (even if small), setting that component to 0 (or pulling it towards Z = 0, which is effectively the same, discarding the non XY-plane displacement) is just discarding that amount of kinetic energy, no?
I suppose it'll be negligible, but is this acceptable?
There was a problem hiding this comment.
just discarding that amount of kinetic energy, no?
It is. Theoretically the Z velocity introduced through the physics simulation should have been rerouted towards the X or Y axes in a 2D world. Given that the contact solver does not operate in 2d, some leeway is to be expected. Bepu is very flexible, but I doubt we could force it to resolve penetration in 2d. It would also likely require specific logic for 2d shapes.
Having a purpose-built solver constraint, like I mentioned in another comment, could help reduce the Z axis drift, but it would have to introduce some energy in the system when that constraint fights against the contact solver.
There was a problem hiding this comment.
Thanks for the explanation
| /// <remarks> | ||
| /// Runs before solve so correction participates in contact resolution. Sleeping bodies are skipped, | ||
| /// but the rotation lock is refreshed first so state stays valid across kinematic changes. | ||
| /// |
There was a problem hiding this comment.
Empty /// won't result in separation in the compiled docs. Use <para/>
| /// <summary> | ||
| /// Does nothing. The whole correction happens before the solve, in <see cref="SimulationUpdate"/>. | ||
| /// </summary> |
There was a problem hiding this comment.
This one is just personal preference. For summaries in XMLdocs (specially for abstract or virtual methods) I tend to prefer explaining what the method is, not what it does.
With this I mean:
SimulationUpdatewould be something likeUpdates the simulation state of the 2D body, and in the remarks I'll write theThis method runs before the Solve() blah blah to confine the body to the Z plane blah blah.- In the same way,
AfterSimulationUpdatewould be something likeMethod called after the simulation has ran on the 2D body, and the remarks would point out thatThis method does nothing, the whole correction to confine the body to the Z plane has already happened in SimulationUpdate blah blah.
I think this structure reads better when shown both in Intellisense and in the API docs.
Eideren
left a comment
There was a problem hiding this comment.
Thanks for looking into this! We'll probably need a bit more back and forth on this one
|
|
||
| MaximumRecoveryVelocity = MathF.Min(MaximumRecoveryVelocity, HullMaximumRecoveryVelocity); | ||
| SpringDampingRatio = MathF.Max(SpringDampingRatio, HullSpringDampingRatio); | ||
| SpringFrequency = MathF.Min(SpringFrequency, HullSpringFrequency); |
There was a problem hiding this comment.
You will want to document why this special case for hull is required, and not as a remark, remarks are for the callers, not the maintainers. And also for cases where we have some expectation that the caller should be aware of this and may be able to deal with it - the caller here is the internals of the engine and it calls this method through its base signature, not the overridden one, it has no way to see the remark or care about it.
Kind of a nitpick, but I would argue that early outs should only be for unexpected or uncommon cases, so invert the condition and replace that return by the scope below.
But I would also argue that this should not exist as is: from a user's point of view, where they to mutate those properties, they will see them go back to the mins and maxes defined there whenever they change the collider used. That definitely falls into unexpected side effects.
What we can do is change the default values for those properties, but you'll need to validate that those new values perform better across different contexts and don't introduce any unexpected side effects.
| /// Initializes a new <see cref="Body2DComponent"/> with interpolation enabled, so rendering stays | ||
| /// smooth when the display refreshes faster than the fixed physics step. | ||
| /// </summary> | ||
| public Body2DComponent() => InterpolationMode = InterpolationMode.Interpolated; |
There was a problem hiding this comment.
There is no reason to change this properties' default for 2D bodies specifically, either we change them for both or for neither. I'm not necessarily against changing the default for bodies, we just have to justify the additional overhead this would introduce.
| /// <summary>One millimetre at Stride's default scale.</summary> | ||
| private const float DefaultZTolerance = 0.001f; |
There was a problem hiding this comment.
Afaik this summary is incorrect, we do not have a 'default scale', let's not pretend that we do because some AI hallucinated it.
|
|
||
| /// <summary> | ||
| /// Gets or sets how far the body may drift off the Z = 0 plane before it is pulled back, in world | ||
| /// units. Defaults to 0.001 (one millimetre at Stride's default scale). |
There was a problem hiding this comment.
Same here wrt 'default scale'.
| public float ZTolerance | ||
| { | ||
| get => _zTolerance; | ||
| set => _zTolerance = float.IsFinite(value) && value > 0f ? value : DefaultZTolerance; |
There was a problem hiding this comment.
if the value is invalid it should throw instead of silently going back to the default tolerance
| /// <remarks> | ||
| /// <para> | ||
| /// Planar behavior is enforced in two places: X/Y rotation is locked by zeroing the corresponding | ||
| /// inverse-inertia terms when the body attaches, and Z drift is corrected by setting linear Z velocity | ||
| /// before each solve. | ||
| /// </para> | ||
| /// <para> | ||
| /// The positional correction is velocity-based (not teleport-based) so contact resolution stays stable. | ||
| /// Sleeping bodies are left untouched. | ||
| /// </para> |
There was a problem hiding this comment.
These are for maintainers, not for users. There is no reason for users to be aware of those implementation detail.
There was a problem hiding this comment.
I'd argue that if the current solution changes properties of the body in an unexpected way, at least some part of the comment is justified, so users don't end up wondering why the velocity is changing if they somehow set it manually.
I don't know if this is the best solution (for the reasons stated in my comment about energy loss) or if that is unavoidable. But as the current solution introduces such energy loss as a byproduct of its correction, I'll at least mention it.
Does this make sense?
There was a problem hiding this comment.
Replied to the comment you're referring to.
Velocities mutate on every tick based on collisions, friction, constraints, etc. an object would have to counteract gravity and not be in contact with another object for its velocity to stay constant. Additionally, given that it's a 2d object, manipulating its Z velocity is kind of an undefined behavior
There was a problem hiding this comment.
Still, if we can get the constraint going, this would become a non-issue as that field would be used in the simulation tick and written to by the physics engine through our constraint and the solver
There was a problem hiding this comment.
Well, that's true. Not specific to this very comment however, but is there no value in documenting for users that this 2D body implementation is "correcting" towards a fixed plane and losing energy in the process in contrast to what a true 2D physics solver would do?
There was a problem hiding this comment.
There definitely is, yeah. Placing it at the top of the class would not be visible through editor tooltips - velocity would be but we would have to make it virtual, and overwrite it just for the sake of a comment. The last spot would be in the docs I guess ?
There was a problem hiding this comment.
Placing it at the top of the class would not be visible through editor tooltips
How so? Both VS, Rider, and VSCode (I think) shows the remarks section for me. I was composing many of the docs in my Silk.NET PR verifying how it looked and read in the tooltips, and also once rendered to docs (through an extension)
The last spot would be in the docs I guess ?
Independently of how we comment in the code, if it is something important that may need further explanation, we should write it in the docs, yes. But imho only if the XMLdocs would become too large
There was a problem hiding this comment.
I meant the gamestudio, IDE would pick it up fairly easily, yeah
There was a problem hiding this comment.
Yeah, in that case it would be better in the documentation.
Not related to this conversation: One idea I had not long ago was to ditch entirely the lower part of the PropertyGrid (where the UserDocs are shown) and instead show a tooltip with the very same information, just not limited by size or format.
In parallel, we could add something to UserDocs (or other tag) to link to documentation pages explaining concepts or properties.
And bind the F1 key to launch such documentation page if the user presses it while having a property with a docs link.
I think the current design of the low panel with info in the PropertyGrid is a design long surpassed (it's from VS2010 era or before).
There was a problem hiding this comment.
Eh, I don't know. I feel like hover tooltips are kind of bad in terms of UX;
- No indication that something has a tooltip. Certain programs work around this by having the thing underlined, but that doesn't look great
- Having to wait for it to show up is kind of a pain
- It ends up hiding other elements of the UI that you're likely to look at right after reading the tooltip, you'll have to move the cursor away from the element to look at the next one
Granted the current one has issues as well;
- Tooltip can be quite far visually from the element in question
- Always take up space on screen
But I do think it's the lesser of two evil. I would like to see some of those issues addressed, or have a different implementation with inherently different set of features and issues if we do move away from the current one.
| /// Z correction uses a bounded velocity target (not teleporting), with a gentle proportional pull | ||
| /// toward the plane. <paramref name="simTimeStep"/> is intentionally unused. | ||
| /// </remarks> | ||
| public virtual void SimulationUpdate(BepuSimulation sim, float simTimeStep) |
There was a problem hiding this comment.
ISimulationUpdate is a user entrypoint, meaning that user logic may run before or after this method. Does the logic of this method still make sense when run before user logic ? I would argue that it doesn't, as users may wake, teleport, change kinematic state, or mutate velocities after this logic ran, meaning the 2d body may now be in an unexpected state once the simulation step occurs.
I would like to advance another way this could be solved;
Bepu has single-body constraints, for example our OneBodyLinearServoConstraintComponent makes a body try to match the position of an arbitrary point. We also have LinearAxisLimitConstraintComponent which constrain a body to be within at most x units from a plane oriented based on the first body.
Could you look into whether it makes sense for 2D bodies to introduce a custom bepu constraint specifically for keeping them constrained to the 2d plane - it should be far more stable and performant than the logic introduce here.
If that's not possible, the next best thing would be to collect those components within a processor and apply those changes right after the ISimulationUpdate.SimulationUpdate call but do so in parallel. Afair, none of those properties have side effects - they can be written to in parallel.
| if (bodyRef.Velocity.Linear.Z != targetVelocityZ) | ||
| { | ||
| bodyRef.Velocity.Linear.Z = targetVelocityZ; | ||
| } |
There was a problem hiding this comment.
This is significantly slower than just assigning, .Velocity is two array lookups
| if (bodyRef.Velocity.Angular.X != 0f || bodyRef.Velocity.Angular.Y != 0f) | ||
| { | ||
| bodyRef.Velocity.Angular.X = 0f; | ||
| bodyRef.Velocity.Angular.Y = 0f; | ||
| } |
| var inverseInertia = inertia.InverseInertiaTensor; | ||
|
|
||
| inverseInertia.XX = 0f; | ||
| inverseInertia.YY = 0f; | ||
| inverseInertia.YX = 0f; | ||
| inverseInertia.ZX = 0f; | ||
| inverseInertia.ZY = 0f; // ZZ is left alone, so the body can still roll in the plane | ||
|
|
||
| inertia.InverseInertiaTensor = inverseInertia; |
There was a problem hiding this comment.
var inertia = BodyInertia;
ref var inverseInertia = ref inertia.InverseInertiaTensor;
inverseInertia.XX = 0f;
inverseInertia.YY = 0f;
...
BodyInertia = inertia;Or
var inertia = BodyInertia;
inertia.InverseInertiaTensor.XX = 0f;
inertia.inverseInertia.YY = 0f;
...
BodyInertia = inertia;
@Ethereal77, @Eideren , thanks for the comments, back and forth is absolutely expected. Let me research, test and come back to you. |
PR Details
Replaces the unused Bepu 2D physics code with a self-contained
Body2DComponentthat confines a body to the XY plane.
Some logic came from the original implementation, kudos to @Nicogo1705, and the
rotation-lock approach follows Norbo's guidance in #2495.
What changed
Body2DComponent: rewritten. Rotation is locked at the source by zeroing the X/Yinverse inertia, and the Z plane is held by a small velocity correction applied
before each solve instead of teleporting the body between steps.
Simulation2DComponent,Module.cs,Stride.BepuPhysics._2D.csproj: removed.That project sat beside the
Stride.BepuPhysicsproject directory, so the SDK globnever compiled it and no solution or project referenced it,nothing shipped from it.
BepuTests.cs: tests for the constructor default,ZTolerancevalidation, thecorrection clamp, convergence to the plane, sleeping, and the kinematic re-lock.
Why this design
It matches how the rest of the engine already does 2D: the Bullet backend sets
LinearFactor = (1,1,0)andAngularFactor = (0,0,1)for 2D shapes, and Unity,Unreal and Godot expose the same idea as per-axis freeze flags. Zeroing the inverse
inertia is the angular factor; clearing out-of-plane velocity each step is the linear
one. Bepu has no linear factor to set, which is why a small positional correction
remains.
Behavioural differences from the old code
Simulation2DComponent.#warningabout kinematic transitions: switching back to dynamic restoresthe full shape inertia, which silently undid the lock. It is now reapplied.
MaximumRecoveryVelocity/SpringFrequencyand raisesSpringDampingRatio, since hulls destabilise dense piles. These are ceilings and afloor rather than overwrites, but a deliberately stiff hull body will come out softer.
Known trade-off
ISimulationUpdatedispatches per body every step, including sleeping ones, themethod early-outs, but the dispatch still scales with body count. A batch over the
active set would scale better; it was left out because it would cost every 3D-only
game a per-step walk. Happy to revisit if maintainers prefer that.
Longer term, per-axis locking on
BodyComponent(matching the Bullet backend) wouldmake 2D a preset rather than a component type, and would also cover the common 3D case
of keeping a character upright. Out of scope here.
AI assisted with this PR. Suggest noting in the release notes and XML docs that the
component is new and expects to mature with feedback.
Related Issue
#2495
Types of changes
Checklist