Repository navigation
Fix turntable pitch escape from polar views - #227
Merged
Merged
Conversation
SchrodingersCattt
left a comment
Owner
Author
There was a problem hiding this comment.
Verdict: approve-ready (COMMENT — GitHub blocks self-APPROVE on author PR)
Focused fix for a real turntable bug: sampled elevation clamp treated an already-polar view as an outward violation, so the first inward pitch stayed stuck at ±90°. Replacing that with analytic Rodrigues z(t) roots and clamping only on outward boundary crossings is the right model.
What looks solid
- Early return when already beyond the cap and the pitch derivative would push further out — this is the escape-vs-freeze distinction, and the docstring/API note match it.
- Cap on
|requested|at 180° + analytic roots still prevent tunneling through a narrow polar band (covered bytest_narrow_cap_cannot_be_skipped_by_large_request). - Parametrized cases cover both poles, ±pitch escape, near-pole inward/outward, normal-view clip, and huge requests; opposite-direction check at the north pole is a good chirality guard.
- Honest PR note that the 22 full-suite failures reproduce on unmodified
0258900— not introduced here. - Call sites stay in TUI/
orbit_turntable(controller.py+ camera tests); scope matches the stated bug.
Process notes (non-blocking)
- Rebase onto current
mainafter #226 (e4663b4, scene-rename settle). This branch is still based on0258900, so Camera WebGL on the PR can still hit the rename flake until rebased; the math change itself does not touch that path. - If a Dash/WebGL orbit path ever diverges from
orbit_turntable, that would be a separate follow-up (not required for this PR).
Happy to merge once CI is green on a #226-inclusive base.
SchrodingersCattt
force-pushed
the
fix/tui-polar-escape
branch
from
October 25, 2026 07:49
9aad056 to
51e1a31
Compare
9 tasks
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.
Problem and change
An exact top-down camera starts at elevation 90°, beyond the default 89° turntable limit. The old sampled clamp treated the first inward step as a new violation, freezing
orbit(pitch_deg=-30)at 90°. Both ±30° requests now leave the pole and end at approximately 60°, in opposite horizontal directions.Compute the first outward boundary crossing analytically from the rotation arc. Preserve narrow caps near 90°, very small positive limits, tangency, and repeated outward requests at an already-clipped boundary. Use complementary angular comparisons with roundoff scaled to the polar gap, retaining inclusive tangency without erasing narrow caps. Use the measured arc amplitude for limits near zero.
Add 51 regression cases and update the TUI API contract.
Also isolate the surface-picking browser regression in a fresh server fixture. The prior orthographic-drag test persisted a DAP-4 camera in the shared module server, putting the surface-picking fixtures out of view. The unchanged sequence reproduced the CI failure locally; with independent server state, both tests pass. Picking assertions are unchanged. The branch includes main's #226 Camera WebGL settle-race fix (
e4663b4).Validation
Final-head GitHub CI: 9/9 checks passed, including Camera WebGL and Python 3.10/3.11/3.12 full suites. Run 40176981439. The earlier surface-picking failure reproduced twice in CI and in the local drag-then-pick sequence; the fixture-isolation fix passed the sequence locally and this final CI run.
Browser order regression: orthographic drag followed by surface picking reproduced the failure before fixture isolation; the same sequence passes both tests after isolation (2 passed in 92.76s).
Current head
4bd1585c35f70995c713bf7534afd8914b4d387f: all 61 new/existing camera tests pass locally; changed Python files pass Ruff.Public JSONL smoke check of the polar escape: synchronized
orbit(yaw=30, pitch=-30)moves all three fixture cameras from 90° to approximately 60° and produces distinct candidate frames.Local full suite before the concurrent numerical follow-up (test-isolation patch included): 1847 passed, 50 skipped, 23 failed. The previous 22 baseline failures remain; the additional upload concurrency test fails with the same Windows temporary-directory PermissionError on both patched and unmodified baseline runs.
The initial implementation's local full suite on
0258900reported 1819 passed, 50 skipped, and 22 failures. All 22 failures reproduced on the unmodified same base. Whole-repository Ruff's 266 findings also matched the baseline; none were in the changed Python files.Experimental provenance and the earlier deterministic checks are archived in the chirality campaign. The campaign used an older pinned revision; its counts are separate from this PR's final-head CI.