refactor: Consolidate mouse-pick FP-mode guard into getPickRay() - #228
Merged
fbraz3 merged 1 commit intoJul 21, 2026
Merged
Conversation
Follow-up to fbraz3#222 addressing review feedback from @meerzulee: - Move the single setFPMode() call into getPickRay(), the shared chokepoint used by screenToTerrain(), pickDrawable(), screenToWorldAtZ() and others, so all pick paths are covered by one guard. Remove the two now-redundant calls. - Reword the comments to describe a defensive FP-mode re-assert. The previous wording stated an "audio-thread FP env leak" as fact, which is incorrect: FPU state is per-thread (FPCR on arm64). The likely cause is main-thread audio work altering the FP mode, addressed separately in fbraz3#226. Closes fbraz3#227 AI-assisted: change authored with Claude. Compiles clean on macOS/ARM; in-game re-test pending (will confirm on the PR). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
|
Hey @costin-alupului, thanks again for the PR. Could you please keep the PR as a draft until it’s tested? I usually test these points on both games:
|
costin-alupului
marked this pull request as draft
July 20, 2026 13:39
Author
|
Tested on macOS/ARM (M4 Max, macOS 26) at native fullscreen, audio on — all three areas check out:
Also confirmed a camera-jump stall (control-group double-tap / Space-to-event) that was affected by the same pick math stays fixed. Marking ready for review. Thanks for the checklist, @fbraz3! |
costin-alupului
marked this pull request as ready for review
July 21, 2026 11:07
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
2 participants
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.
Summary
Follow-up to #222 addressing @meerzulee's review feedback. Closes #227.
Changes
getPickRay(). bugfix: Re-assert FP mode in mouse pick to fix wrong click location with audio active #222 guardedscreenToTerrain()andpickDrawable()separately;getPickRay()is the shared chokepoint (also used byscreenToWorldAtZ(),calcCameraAreaOffset(), and paths inW3DMouse.cpp/W3DTerrainVisual.cpp), so one guard there covers all callers. The two redundant calls are removed.Net:
-12 / +8lines, no behavior change beyond broader + correctly-documented coverage.Testing status (disclosed)
main). I will re-test in a skirmish and comment here to confirm move/build clicks still work. Since the change only relocates an existing, already-verifiedsetFPMode()call to the shared function, behavior should be identical.AI disclosure
Per CONTRIBUTING: authored with AI assistance (Claude); I reviewed the change and will verify it in-game before considering it done.