tweak(drawable): Decouple physics and fade timing from render update - #2055
tweak(drawable): Decouple physics and fade timing from render update#2055bobtista wants to merge 24 commits into
Conversation
|
Can you show a video how it looks before and after? I'd recommend replicating to Generals as the very last thing you do for any PR. It's easier for the PR creator and reviewer(s). |
|
I remember I worked on this before and it was difficult to get perfectly right and then paused this. I still have the WIP branch. I would be surprised if this change fixed it with no problems at all. For easy test, take a GLA Buggy and compare its physics with Retail at different FPS. If it is not matching, then there is work left to do. |
Oh that's right, I remember watching this, there's some buggy jank addressed is in episode 0844. I was just trying to apply changes similar to your decouple stealth fade one for remaining frame based logic, hadn't started testing yet. I'll convert to draft and assume there's more to do |
Can you push the latest to your WIP branch? |
2751da9 to
2bb1cb5
Compare
|
When I increase render FPS and update physics every render, the buggy doesn't wabble or wheelie as much. At higher render FPS we're getting effectively like a higher resolution of the damped spring math, so less overshoots, less wobble, but same total force is applied. Claude says it's called the Euler integration of a damped spring. I think it makes sense to only calculate it at logic frames, and we interpolate so it's smoother visually. Otherwise we're messing with the spring math to approximate the overshoots from before, and it's purely visual right? Anyway - I just tested, and it looks right to me. Note the other changes in this PR don't have this overshoot feedback kind of issue, so they all should work with calculations on render frames. Eg Linear fades like opacity I've implemented this approach and restored the Generals changes (can replicate once this is approved). |
2bb1cb5 to
77e6dad
Compare
|
| Filename | Overview |
|---|---|
| Generals/Code/GameEngine/Include/GameClient/Drawable.h | Adds four m_prevTotal* fields to PhysicsXformInfo for interpolation state, and changes m_timeElapsedFade from UnsignedInt to Real; constructor init updated correctly. |
| GeneralsMD/Code/GameEngine/Include/GameClient/Drawable.h | Mirrors the same header changes as the Generals variant — new m_prevTotal* fields and m_timeElapsedFade type change to Real. |
| Generals/Code/GameEngine/Source/GameClient/Drawable.cpp | Implements physics interpolation (save prev state, lerp on render) and fade time-scaling; xfer migration to version 9 is correct for non-retail builds but the else branch modifies m_timeElapsedFade during SAVE mode in retail-compatible builds. |
| GeneralsMD/Code/GameEngine/Source/GameClient/Drawable.cpp | Identical changes to the Zero Hour variant; same physics interpolation and fade scaling logic, with the same xfer else branch side-effect in retail-compatible save builds. |
Sequence Diagram
sequenceDiagram
participant LF as Logic Frame
participant RF as Render Frame
participant PX as PhysicsXformInfo
participant MTX as Matrix3D
loop Every render tick
alt "WW3D::Get_Sync_Frame_Time() != 0 (logic frame fired)"
RF->>PX: "save prev{Pitch,Roll,Yaw,Z} = current{...}"
RF->>PX: "calcPhysicsXform() → update current{...}"
end
RF->>RF: "t = clamp(fractionalMs / MSEC_PER_LOGICFRAME_REAL, 0, 1)"
RF->>MTX: Translate/Rotate with lerp(prev, current, t)
end
loop Every render tick (fades)
RF->>RF: "fadeTimeScale = getActualLogicTimeScaleOverFpsRatio()"
RF->>RF: "m_timeElapsedFade += fadeTimeScale"
RF->>RF: "m_decalOpacity += m_decalOpacityFadeRate * fadeTimeScale"
end
Prompt To Fix All With AI
### Issue 1
Generals/Code/GameEngine/Source/GameClient/Drawable.cpp:5057-5062
In retail-compatible save builds (`RETAIL_COMPATIBLE_XFER_SAVE`), `currentVersion` is 5 or 7, so the `else` branch is taken during SAVE mode. `xferUnsignedInt` writes the truncated value to the stream and leaves the local alone, but the final assignment unconditionally overwrites `m_timeElapsedFade` with its integer truncation — losing any accumulated fractional fade time on every save. The same function already guards the analogous `m_prevTintStatus` reset with `getXferMode() == XFER_LOAD`; the same guard is needed here. The identical issue exists in `GeneralsMD/Code/GameEngine/Source/GameClient/Drawable.cpp`.
```suggestion
else
{
UnsignedInt timeElapsedFadeFrames = static_cast<UnsignedInt>(m_timeElapsedFade);
xfer->xferUnsignedInt( &timeElapsedFadeFrames );
if (xfer->getXferMode() == XFER_LOAD)
m_timeElapsedFade = static_cast<Real>(timeElapsedFadeFrames);
}
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (2): Last reviewed commit: "refactor(drawable): Restore untouched ph..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@xezon are you ok with copying the zero hour changes to Generals here? Greptile is right that they used different approaches, and I don't see the benefit in keeping or trying to tweak the Generals' one vs using ZH's interpolation |
Both gone - the Zero Hour side no longer queries the ratio inside the physics functions at all (they run on logic frames now), and the Generals side will be replaced with the same approach once agreed. |
|
Found three problems while re-reviewing this, all fixed now:
With those fixed the spring math always steps at the logic rate with retail constants regardless of render fps, so the buggy behaves like retail at 30fps by construction — tested at [30 / 60 / uncapped]. One caveat: the rendered tilt lags the sim by one logic frame (~33ms) because it interpolates prev→current, so a frame-by-frame retail comparison will show that offset. I still want others' thoughts on copying this approach to Generals - the timeScale-at-call-site version there changes the spring dynamics with fps (step size alters the overshoot), which is exactly what your buggy test catches. |
Buggy movement at 30fps Buggy movement at 120fps |
…caling from fixed-rate physics
04f1c20 to
d3c5b8d
Compare
|
Can this get some love? |
…nder time scaling
0451755 to
7be7c8f
Compare
| else | ||
| { | ||
| UnsignedInt timeElapsedFadeFrames = static_cast<UnsignedInt>(m_timeElapsedFade); | ||
| xfer->xferUnsignedInt( &timeElapsedFadeFrames ); | ||
| m_timeElapsedFade = static_cast<Real>(timeElapsedFadeFrames); | ||
| } |
There was a problem hiding this comment.
RETAIL_COMPATIBLE_XFER_SAVE), currentVersion is 5 or 7, so the else branch is taken during SAVE mode. xferUnsignedInt writes the truncated value to the stream and leaves the local alone, but the final assignment unconditionally overwrites m_timeElapsedFade with its integer truncation — losing any accumulated fractional fade time on every save. The same function already guards the analogous m_prevTintStatus reset with getXferMode() == XFER_LOAD; the same guard is needed here. The identical issue exists in GeneralsMD/Code/GameEngine/Source/GameClient/Drawable.cpp.
| else | |
| { | |
| UnsignedInt timeElapsedFadeFrames = static_cast<UnsignedInt>(m_timeElapsedFade); | |
| xfer->xferUnsignedInt( &timeElapsedFadeFrames ); | |
| m_timeElapsedFade = static_cast<Real>(timeElapsedFadeFrames); | |
| } | |
| else | |
| { | |
| UnsignedInt timeElapsedFadeFrames = static_cast<UnsignedInt>(m_timeElapsedFade); | |
| xfer->xferUnsignedInt( &timeElapsedFadeFrames ); | |
| if (xfer->getXferMode() == XFER_LOAD) | |
| m_timeElapsedFade = static_cast<Real>(timeElapsedFadeFrames); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: Generals/Code/GameEngine/Source/GameClient/Drawable.cpp
Line: 5057-5062
Comment:
In retail-compatible save builds (`RETAIL_COMPATIBLE_XFER_SAVE`), `currentVersion` is 5 or 7, so the `else` branch is taken during SAVE mode. `xferUnsignedInt` writes the truncated value to the stream and leaves the local alone, but the final assignment unconditionally overwrites `m_timeElapsedFade` with its integer truncation — losing any accumulated fractional fade time on every save. The same function already guards the analogous `m_prevTintStatus` reset with `getXferMode() == XFER_LOAD`; the same guard is needed here. The identical issue exists in `GeneralsMD/Code/GameEngine/Source/GameClient/Drawable.cpp`.
```suggestion
else
{
UnsignedInt timeElapsedFadeFrames = static_cast<UnsignedInt>(m_timeElapsedFade);
xfer->xferUnsignedInt( &timeElapsedFadeFrames );
if (xfer->getXferMode() == XFER_LOAD)
m_timeElapsedFade = static_cast<Real>(timeElapsedFadeFrames);
}
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.3 participants
Drawable physics and visual fades advance once per render frame, so their speed and behavior change with FPS.
This change decouples them from the render update, with two mechanisms:
Physics (spring-damper pitch/roll, wheel suspension, wobble, recoil): now calculated on logic frames only, with the rendered transform linearly interpolated between the previous and current logic frame. The spring math is a feedback system, so scaling the integration step with FPS changes the dynamics (less overshoot and wobble at high FPS). Running it at the fixed logic rate with the retail constants keeps the behavior identical to retail at 30 fps regardless of render FPS, and the interpolation keeps the motion smooth. The rendered tilt trails the simulation by one logic frame (~33 ms).
Linear fades (drawable fade in/out, decal opacity): open-loop accumulators with no feedback, so these are simply scaled by the logic/render ratio in the render update.
m_timeElapsedFadechanges from UnsignedInt to Real to accumulate fractional steps; Drawable xfer version bumped to 9 for it (non-retail-compatible saves only).Rocket-Buggy movement at 30fps
https://github.com/user-attachments/assets/c389aa0e-356e-4992-a251-f75ece0708bd
Rocket-Buggy movement at 120fps
https://github.com/user-attachments/assets/927e18dc-e532-4d5a-97be-210cd00eafea
Todo: