Add game speed controls to the embedded game window - #1346
Conversation
WalkthroughChangesThe engine now separates user and game time scales, derives effective timing values, and exposes user-scaled physics limits. The embedded game view adds speed controls and debugger messaging. Runtime systems use the new timing accessors. Game Time-Scale Control
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GameView
participant GameViewDebugger
participant SceneDebugger
participant Engine
participant Main
GameView->>GameViewDebugger: select or reset game speed
GameViewDebugger->>SceneDebugger: send speed_changed
SceneDebugger->>Engine: set_user_time_scale
Engine->>Engine: update effective time scale and physics limits
Main->>Engine: read effective timing values
Engine-->>Main: return user physics rate, effective scale, and step limit
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@editor/run/game_view_plugin.cpp`:
- Around line 626-632: Update GameView::_reset_time_scales() to reset
time_scale_index and refresh the speed buttons before the is_visible_in_tree()
early return. Keep debugger->reset_time_scale() behind the visibility guard so
the debugger message behavior remains unchanged.
In `@modules/jolt_physics/joints/jolt_hinge_joint_3d.cpp`:
- Around line 60-61: Update the hinge joint motor state so the requested
HINGE_JOINT_MOTOR_MAX_IMPULSE is retained separately from the derived
motor_max_torque. In set_param(), store the requested impulse and derive the
torque using the current effective step; when step_scaled changes in the physics
update path, recalculate motor_max_torque from that stored impulse. Ensure
get_param() continues converting the refreshed torque back to the requested
impulse consistently.
In `@scene/debugger/scene_debugger.cpp`:
- Around line 216-220: Update SceneDebugger::_msg_speed_changed to require
exactly one numeric argument and validate that the converted time-scale value is
finite before calling Engine::get_singleton()->set_user_time_scale. Return
ERR_INVALID_DATA for empty, extra, non-numeric, or non-finite input, while
preserving the existing successful update path for valid values.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d268355c-5a18-413b-8800-08cb535702ec
📒 Files selected for processing (10)
core/config/engine.cppcore/config/engine.heditor/run/game_view_plugin.cppeditor/run/game_view_plugin.hmain/main.cppmodules/jolt_physics/joints/jolt_hinge_joint_3d.cppscene/3d/velocity_tracker_3d.cppscene/debugger/scene_debugger.cppscene/debugger/scene_debugger.hscene/gui/video_stream_player.cpp
| void GameView::_reset_time_scales() { | ||
| if (!is_visible_in_tree()) { | ||
| return; | ||
| } | ||
| time_scale_index = DEFAULT_TIME_SCALE_INDEX; | ||
| debugger->reset_time_scale(); | ||
| _update_speed_buttons(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reset the UI state when GameView is hidden.
Line 627 returns before resetting time_scale_index or button state. If a game restarts while GameView is hidden, the new session uses 1.0× but the GameView can still show the previous speed.
Reset the local state and button state before the visibility guard. Keep the debugger message behind the guard if that behavior is intentional.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@editor/run/game_view_plugin.cpp` around lines 626 - 632, Update
GameView::_reset_time_scales() to reset time_scale_index and refresh the speed
buttons before the is_visible_in_tree() early return. Keep
debugger->reset_time_scale() behind the visibility guard so the debugger message
behavior remains unchanged.
| const double step = 1.0 / engine->get_user_physics_ticks_per_second(); | ||
| const double step_scaled = step * engine->get_effective_time_scale(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Refresh hinge motor torque when the effective step changes.
Line 61 now changes during a game-view speed update. set_param() converts HINGE_JOINT_MOTOR_MAX_IMPULSE to motor_max_torque only once at lines 246-249. A later speed change leaves that stored torque stale. get_param() then reports a different impulse value at lines 202-204.
Store the requested impulse and recalculate the Jolt torque limit when the effective physics step changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/jolt_physics/joints/jolt_hinge_joint_3d.cpp` around lines 60 - 61,
Update the hinge joint motor state so the requested
HINGE_JOINT_MOTOR_MAX_IMPULSE is retained separately from the derived
motor_max_torque. In set_param(), store the requested impulse and derive the
torque using the current effective step; when step_scaled changes in the physics
update path, recalculate motor_max_torque from that stored impulse. Ensure
get_param() continues converting the refreshed torque back to the requested
impulse consistently.
| Error SceneDebugger::_msg_speed_changed(const Array &p_args) { | ||
| ERR_FAIL_COND_V(p_args.is_empty(), ERR_INVALID_DATA); | ||
| double time_scale_user = p_args[0]; | ||
| Engine::get_singleton()->set_user_time_scale(time_scale_user); | ||
| return OK; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate the time-scale value before applying it.
Line 217 only rejects an empty array. A non-finite value can reach Engine::set_user_time_scale() and propagate through runtime timing calculations.
Require one numeric, finite argument before updating the engine.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scene/debugger/scene_debugger.cpp` around lines 216 - 220, Update
SceneDebugger::_msg_speed_changed to require exactly one numeric argument and
validate that the converted time-scale value is finite before calling
Engine::get_singleton()->set_user_time_scale. Return ERR_INVALID_DATA for empty,
extra, non-numeric, or non-finite input, while preserving the existing
successful update path for valid values.
|
Thank you 🥳 |
3 participants
Pulls in godotengine/godot#107273 from Godot as recommended by @joey-wheeler99
Summary by CodeRabbit
New Features
Bug Fixes