Fix zoom slider incorrectly modifying tick speed - #298
Merged
Conversation
Remove `tick_timer.wait_time = tick_seconds_for_setting()` from the zoom slider's `value_changed` callback. Zoom and tick speed are unrelated settings; adjusting zoom should only affect tile rendering size, not simulation speed. The focus mode handler correctly calls this method since focus mode IS a tick speed modifier. Fixes #293 Signed-off-by: Saffron <263493777+itsmiso-ai@users.noreply.github.com>
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: MiniMax-M2.7@https://litellm.jory.dev/v1 (anthropic) — escalated (fast_low_confidence)
Recommendation: Approve
This is a clean, focused bug fix that directly addresses issue PR 293. The zoom slider callback no longer modifies tick_timer.wait_time, correctly decoupling zoom from simulation speed.
Change-by-Change Findings
scripts/main.gd (2-line deletion)
- Removed:
if tick_timer:guard andtick_timer.wait_time = tick_seconds_for_setting()from zoom slider'svalue_changedcallback - What remains:
settings['zoom_factor'] = val,save_settings(), and zoom label update — all correct behaviors per acceptance criteria - Verified: The remaining callback lines are unchanged and intact
Standards Compliance
- Repository conventions: No documented conventions conflict with this change. The fix follows the implicit convention of keeping unrelated settings orthogonal.
- File conventions: No issues.
Linked Issue Fit
| Acceptance Criterion | Status |
|---|---|
Remove tick_timer.wait_time = tick_seconds_for_setting() from zoom slider callback |
✅ Verified in diff |
| Verify zoom only affects tile rendering size, not simulation speed | ✅ Confirmed: only zoom_factor and label are updated |
| No regression: zoom slider still updates label and saves settings | ✅ Verified: both save_settings() and label update remain |
Tool Harness Findings
Confirmed via git_grep on tick_seconds_for_setting:
- Line 260: Timer initialization (correct —
tick_timer = Timer.new()setup) - Line 891:
_on_tick_speed_changed()handler (correct) - Line 1179: Function definition (no change)
- Line 276: Was the zoom slider bug — now removed
Confirmed via read_file: The zoom slider callback (lines 289-295 post-diff) contains only:
settings['zoom_factor'] = val
save_settings()
zoom_label.text = "Zoom: " + str(round(val * 100) / 100.0)CI Results
All checks passed:
- macOS validation: ✅
- Export validation (Linux, Windows, macOS): ✅
- Script test suite: ✅
- Headless smoke test: ✅
Unknowns / Needs Verification
None — the diff is minimal, the fix is precise, and CI is green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Removes the redundant
tick_timer.wait_time = tick_seconds_for_setting()reassignment from inside the zoom slider'svalue_changedcallback inscripts/main.gd.Why
The zoom slider callback was re-applying the time-scale setting on every zoom change, even though…
Fixes #293
Opened by foreman on review GO (workload wl-misospace-windowstead-293).