Return the viewport extent in scene units so zoom survives recompute - #89
Merged
Conversation
Address review follow-ups on the viewport zoom fix: - Add a non-circular test that the world-space pan step halves when the zoom doubles. The existing pan tests ran only at zoom 1 (where the `/ view.zoom` is inert) and mirrored the production expression as their own oracle, so a `/ view.zoom` -> `* view.zoom` regression would have passed unnoticed. - Document in `_scene_extent`'s docstring that the extent is in scene units and excludes `view.zoom` because the coordinates already carry it, so consumers must not re-apply it. - Reword the `_KEY_PAN_FRACTION` comment: the step is a fraction of the viewport, not of the scene extent. - Changelog: call it "zoom" to match the API.
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.
_scene_extentreturned the interactive viewport half-extent scaled byview.zoom, but the projected coordinates already carryview.zoom. When a frame-navigation redraw recomputed the extent with the current zoom baked in, the viewport and the coordinates scaled together and cancelled, silently resetting the interactive zoom.Return the extent in scene units instead (drop the
* view.zoom). The value's two consumers were already written to be correct with an unzoomed extent, so they need no change: the viewport axis limits (base_extent * 1.15) become a fixed window that the zoom-carrying coordinates grow within, and the pan step (base_extent / view.zoom) becomes a fraction of the visible scene extent, so pan steps stay proportional as you zoom rather than coarsening after a recompute.view.zoomis now an absolute magnification, where1fits the scene, so a view opened at a magnification above one starts zoomed in rather than being absorbed into the opening fit. This also makes the interactive viewer consistent with the static render path, which already crops on a preset zoom._scene_extentis interactive-only and off the static render path, so the change is pinned by a unit test — the extent is now independent ofview.zoom— rather than the golden harness.