Skip to content

tweak: decouple GUI window-move animations from the render frame rate#2833

Open
bobtista wants to merge 2 commits into
TheSuperHackers:mainfrom
bobtista:bobtista/tweak/decouple-window-move-animation
Open

tweak: decouple GUI window-move animations from the render frame rate#2833
bobtista wants to merge 2 commits into
TheSuperHackers:mainfrom
bobtista:bobtista/tweak/decouple-window-move-animation

Conversation

@bobtista

Copy link
Copy Markdown

Addresses #2832, Similar to 2056, which decoupled the GameWindowTransitions.

Problem

Control bar, diplomacy/communicator panels slide animations go too fast when you increase render FPS

Approach

AnimateWindowManager::update() now paces itself by elapsed wall-clock time: it runs the number of base-rate (30 fps) steps matching the real time since the last update, carrying the fractional remainder between updates.

Wall-clock pacing is used here (rather than #2056's getBaseOverUpdateFpsRatio()) on purpose: AnimateWindowManager is pumped on two different clocks. The shell drives it through a fixed ~30 Hz wall-clock gate (Shell::update), while in-game callers drive it once per render frame. A render-frame ratio would double-decouple the already-gated shell path. Wall-clock self-pacing is correct for both with no per-call-site changes. A 6-step catch-up cap keeps a lon snapping an animation straight to its end.

TODO:

[x] Testing
[x] Replicate to Generals

@greptile-apps

greptile-apps Bot commented Jun 27, 2026

Copy link
Copy Markdown

Greptile Summary

Introduces wall-clock-based frame-rate pacing to AnimateWindowManager::update() so GUI slide animations run at a consistent speed regardless of render FPS. The old update() body is promoted to updateStep(), and update() now accumulates elapsed time (via timeGetTime()) to run the correct number of 30 Hz base-rate steps per call, with a 6-step catch-up cap to prevent snapping on load/alt-tab stalls.

  • New m_lastUpdateTime / m_updateAccumulator members are initialized in the constructor, init(), and reset(), correctly covering all lifecycle paths.
  • The change is replicated identically to both Generals/ and GeneralsMD/ trees.

Confidence Score: 4/5

The change is safe to merge; the accumulator logic and lifecycle resets are all correct, and only cosmetic tab-alignment issues were found in the headers.

The wall-clock accumulator correctly converts elapsed milliseconds to base-rate steps, the 6-step cap prevents snap-to-end on stalls, and the new members are initialized in the constructor, init(), and reset(). The only findings are tab-alignment inconsistencies within the three newly added header declarations.

Both AnimateWindowManager.h headers have minor column-alignment inconsistencies in the new member block; the .cpp files are clean.

Important Files Changed

Filename Overview
Generals/Code/GameEngine/Source/GameClient/GUI/AnimateWindowManager.cpp Core change: new update() shell with wall-clock accumulator and updateStep() extraction; logic is correct and lifecycle resets are complete.
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/AnimateWindowManager.cpp Identical replica of the Generals/ change for the Zero Hour expansion; no divergence from the Generals/ version.
Generals/Code/GameEngine/Include/GameClient/AnimateWindowManager.h Adds updateStep() declaration and two new private members; minor tab-alignment inconsistency within the new member block.
GeneralsMD/Code/GameEngine/Include/GameClient/AnimateWindowManager.h Identical header replica for Zero Hour; same minor tab-alignment inconsistency in the new member block.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Caller as Shell / RenderFrame Caller
    participant AWM as AnimateWindowManager
    participant TGT as timeGetTime
    participant Step as updateStep

    Caller->>AWM: update()
    AWM->>TGT: timeGetTime() returns now
    AWM->>AWM: "elapsed = now minus m_lastUpdateTime"
    AWM->>AWM: "m_lastUpdateTime = now"
    AWM->>AWM: "accumulator += elapsed x (30 div 1000)"
    AWM->>AWM: "steps = floor(accumulator)"
    AWM->>AWM: "accumulator -= steps"
    AWM->>AWM: "steps = min(steps, 6)"
    loop steps greater than 0
        AWM->>Step: updateStep()
        Step->>Step: advance winMustFinishList and winList one base-rate tick
    end
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Caller as Shell / RenderFrame Caller
    participant AWM as AnimateWindowManager
    participant TGT as timeGetTime
    participant Step as updateStep

    Caller->>AWM: update()
    AWM->>TGT: timeGetTime() returns now
    AWM->>AWM: "elapsed = now minus m_lastUpdateTime"
    AWM->>AWM: "m_lastUpdateTime = now"
    AWM->>AWM: "accumulator += elapsed x (30 div 1000)"
    AWM->>AWM: "steps = floor(accumulator)"
    AWM->>AWM: "accumulator -= steps"
    AWM->>AWM: "steps = min(steps, 6)"
    loop steps greater than 0
        AWM->>Step: updateStep()
        Step->>Step: advance winMustFinishList and winList one base-rate tick
    end
Loading
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
Generals/Code/GameEngine/Include/GameClient/AnimateWindowManager.h:195-197
The `///<` comment anchors within the new three-line block are not aligned to the same column: `updateStep()` reaches the comment with more tabs than the two field declarations below it. Per the project's column-alignment rule, declarations added together in the same block should have their doc-comment columns consistent with each other.

```suggestion
	void updateStep();									///< Runs a single base-rate step of all registered window animations
	UnsignedInt m_lastUpdateTime;								///< Wall-clock time of the previous update, for frame-rate independent pacing
	Real m_updateAccumulator;									///< Carries fractional base-rate steps between updates
```

### Issue 2 of 2
GeneralsMD/Code/GameEngine/Include/GameClient/AnimateWindowManager.h:195-197
Same column-alignment inconsistency in the Zero Hour replica: `updateStep()` has a different tab count to its `///<` comment than the two field declarations in the same block.

```suggestion
	void updateStep();									///< Runs a single base-rate step of all registered window animations
	UnsignedInt m_lastUpdateTime;								///< Wall-clock time of the previous update, for frame-rate independent pacing
	Real m_updateAccumulator;									///< Carries fractional base-rate steps between updates
```

Reviews (1): Last reviewed commit: "tweak: decouple GUI window-move animatio..." | Re-trigger Greptile

@xezon xezon left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Try to use less AI code generation for better code

ProcessAnimateWindowSlideFromTopFast *m_slideFromTopFast; ///< holds the process in which the windows slide from the top,fast
ProcessAnimateWindow *getProcessAnimate( AnimTypes animType); ///< returns the process for the kind of animation we need.

void updateStep(); ///< Runs a single base-rate step of all registered window animations

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

blank line after

ProcessAnimateWindowSlideFromTopFast *m_slideFromTopFast; ///< holds the process in which the windows slide from the top,fast
ProcessAnimateWindow *getProcessAnimate( AnimTypes animType); ///< returns the process for the kind of animation we need.

void updateStep(); ///< Runs a single base-rate step of all registered window animations

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

step() is simpler name.

m_updateAccumulator -= (Real)steps;

// Cap the catch-up burst so a long stall (load, alt-tab) cannot snap an animation instantly.
const Int maxSteps = 6;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why 6 ?

void AnimateWindowManager::update()
{
const UnsignedInt now = timeGetTime();
if (m_lastUpdateTime == 0)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Try to get rid of this condition. Maybe set in init function?

ProcessAnimateWindowSlideFromTopFast *m_slideFromTopFast; ///< holds the process in which the windows slide from the top,fast
ProcessAnimateWindow *getProcessAnimate( AnimTypes animType); ///< returns the process for the kind of animation we need.

void updateStep(); ///< Runs a single base-rate step of all registered window animations

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Do not put function below member variables.

// render frame rate. The animation logic runs at the historic base rate; here we run the
// number of base-rate steps that match the elapsed wall-clock time, carrying the fractional
// remainder between updates. This is independent of how often update() is called, so callers
// pumped per render frame and callers throttled to a fixed rate both animate at the same speed.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Comment is very long. Try simplify.

// pumped per render frame and callers throttled to a fixed rate both animate at the same speed.
void AnimateWindowManager::update()
{
const UnsignedInt now = timeGetTime();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Avoid timeGetTime where possible. Use TheFramePacer->getUpdateTime()

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