Select the projection mode with a sum type by bjmorgan · Pull Request #85 · bjmorgan/hofmann

bjmorgan · 2026-08-16T07:18:56Z

Replaces ViewState.perspective / ViewState.view_distance with a single
projection field holding Orthographic or Perspective, and consolidates the
camera-to-screen mapping onto one code path.

Why

The old encoding used perspective = 0.0 as a sentinel for "orthographic",
which left view_distance meaningless whenever perspective was off and gave no
answer to whether 0.0 meant disabled or very weak. A viewing distance could be
set on a view that ignored it. The sum type makes both states unrepresentable:
a parallel projection is Orthographic(), and Perspective validates that its
strength is positive.

Separately, the perspective scale was open-coded in three places -- project,
_project_point, and the cell-edge sub-segment loop. Three copies of one
formula can drift apart silently, leaving cell edges projected differently from
the atoms they attach to. ViewState.project_camera is now the single mapping,
and all three route through it.

Behaviour

Rendered output is unchanged, bit-for-bit, for both projection modes. Verified
by capturing every drawn polygon vertex, in draw order, plus viewport limits,
across three scenes and nine camera configurations, and diffing against main at
each step of the refactor. The harness was checked to have the resolution to
detect a real change before being relied on.

New warnings, for cases that were silently degenerate before: project_camera
and _scene_extent both emit a UserWarning when the scene reaches the
perspective eye plane (view_distance <= depth * strength), naming the settings
to adjust. Previously this produced mirrored geometry or a blank viewport with
no diagnostic.

Two interactive keys change, because the state they mutated no longer exists in
the orthographic case: d and D now act only in perspective mode, where
before they adjusted a distance that had no effect until perspective was
switched on; and P steps out to Orthographic() at the bottom of the ladder
rather than approaching zero and stranding on a float residue. The viewing
distance is remembered across an orthographic excursion, so P to the bottom
then p back restores the distance the session was using -- matching main,
where view_distance was an independent field that survived. None of this is
visible in rendered output.

Migration

scene.view.perspective = 0.3         ->  scene.view.set_perspective(0.3)
scene.view.perspective = 0.0         ->  scene.view.set_orthographic()
scene.view.view_distance = 5.0       ->  scene.view.set_perspective(strength, 5.0)

The setters chain with look_along. Assigning a mode value to projection
directly stays available for callers that already hold one, and Projection
(Orthographic | Perspective) is exported for annotating such code.

Notes for review

project recomputes the perspective denominator rather than recovering it
from the scale project_camera returns -- that division round trip is not
bit-exact, and the exact-output claim depends on it.
Every match on the projection carries assert_never (four sites: project,
project_camera, _scene_extent, _foreshortening_distance), so adding a
third projection mode fails mypy at each unhandled site rather than failing at
runtime.
The silhouette radius keeps an approximate form (sqrt(d^2 - r^2), exact only
at full strength). Deliberate: bond end caps foreshorten to the same reference
distance, so the two must be corrected together to keep atoms and their bonds
consistent -- scheduled together in the follow-up perspective-correctness work.
_collect_cell_edges measures 2.52 ms against 2.99 ms on main for a
216-atom scene: the sub-segment endpoints are now interpolated and projected
in one call rather than pair by pair.
The axes orientation widget deliberately does not route through
project_camera. It is fixed-size screen furniture that ignores zoom and
shows direction only.

Introduce frozen dataclasses for the two projection modes, validated at construction. Nothing consumes them yet. Perspective takes a positive strength, so a parallel projection is Orthographic rather than a zero strength: the sentinel encoding it replaces made view_distance meaningless whenever perspective was off, and left no answer to whether 0.0 meant "disabled" or "very weak". The screen scale D / (D - z * s) places the eye at view_distance / strength, so strength 1.0 is a true pinhole at view_distance.

Replace ViewState.perspective and ViewState.view_distance with a single projection field holding Orthographic or Perspective, and translate every consumer. The arithmetic is carried over unchanged: rendered output is bit-for-bit identical to the two-field implementation for both modes, verified across a spread of scenes and camera configurations. Two interactive keys change behaviour, because the state they used to mutate no longer exists in the orthographic case: d and D now act only in perspective mode, where previously they adjusted a view_distance that had no effect until perspective was switched on. P now steps out to Orthographic once the strength falls below a floor, rather than approaching zero and stranding on a float residue.

The perspective scale was open-coded in three places: ViewState.project, _project_point, and the cell-edge sub-segment loop. Three copies of one formula can drift apart silently, leaving cell edges or bonds projected differently from the atoms they attach to. Add ViewState.project_camera as the single mapping and route all three through it. Output is unchanged, bit-for-bit. project recomputes the perspective denominator directly for the silhouette radii rather than recovering it from the returned scale: that division round trip is not bit-exact.

set_orthographic and set_perspective build the mode from scalars, so selecting a projection needs no import and chains with look_along the way the rest of the camera API does. Assigning a mode value to projection directly stays available for callers that already hold one.

The consolidation is only worth having if it stays consolidated, so assert the property rather than the call sites: every drawn cell-edge endpoint must lie on a cell edge as ViewState projects it, under both projection modes, and _project_point must agree with project_camera. Verified to bite: perturbing the cell-edge projection by 0.01% fails both parametrised cases.

Extract _eye_distance in bond_geometry, replacing a five-line conditional duplicated between the scalar and batch paths and naming the 1e6 parallel stand-in that the sum type finally makes expressible. Hoist the radii check out of project's match, so the match dispatches on the projection alone rather than on a 2x2 of mode and radii. Forward set_perspective's defaults to Perspective's own, and drop the test that existed only to police drift between the two copies. Guard both total matches with assert_never: adding a third variant now fails mypy at every unhandled site rather than raising UnboundLocalError at runtime. Reuse the renderer's _cell_edges_3d in the drawn-geometry test instead of rebuilding the corner and edge tables, migrate the last four un-migrated assignments in the docs image generator, and alphabetise the new exports.

The sub-segment loop projected each endpoint pair separately, which made the consolidation cost 0.35 ms per cell-edge pass on a 216-atom scene and left a Python loop doing work numpy can do in one call. Interpolating and projecting all of an edge's sub-segments together measures 2.52 ms against 2.99 ms on main and 3.33 ms per-pair, and the drawn output is unchanged bit-for-bit. Also skip the multiply by a column of ones under a parallel projection, which is 11% of ViewState.project at ten thousand atoms.

Copilot

Pull request overview

This PR refactors camera projection selection from the sentinel float pair ViewState.perspective/ViewState.view_distance into a single sum-type field ViewState.projection (Orthographic or Perspective), and consolidates camera→screen mapping through ViewState.project_camera to prevent drift between render paths.

Changes:

Introduces Orthographic/Perspective projection types (with validation) and migrates ViewState and callers to the new projection field plus chaining setters.
Centralizes projection math in ViewState.project_camera and routes _project_point and cell-edge rendering through it.
Updates tests and documentation to reflect the new projection API and interactive behavior changes.

Reviewed changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/test_rendering/test_projection.py	Updates projection tests and adds a regression test ensuring drawn geometry uses `ViewState` projection consistently.
tests/test_rendering/test_interactive.py	Updates interactive keybinding tests for the new `projection` model and the perspective-only distance behavior.
tests/test_model/test_view_state.py	Adds coverage for `Orthographic`/`Perspective` validation, defaults, immutability, and chaining setters.
src/hofmann/rendering/projection.py	Routes point projection through `ViewState.project_camera` and updates extent logic for the sum-type projection.
src/hofmann/rendering/interactive.py	Reworks perspective/distance key handling to operate via `projection` and immutably update `Perspective`.
src/hofmann/rendering/cell_edges.py	Consolidates cell-edge endpoint projection through `project_camera` and batches endpoint projection for performance.
src/hofmann/rendering/bond_geometry.py	Switches bond foreshortening “eye distance” logic to use the new projection types.
src/hofmann/model/view_state.py	Defines `Orthographic`/`Perspective`, adds `projection`, and introduces `project_camera` plus projection setters.
src/hofmann/model/init.py	Exports `Orthographic` and `Perspective` from `hofmann.model`.
src/hofmann/init.py	Exports `Orthographic` and `Perspective` from the top-level `hofmann` package API.
README.md	Updates example usage to use `set_perspective`.
docs/rendering.rst	Updates rendering docs to the new projection API and terminology.
docs/interactive.rst	Documents that `d`/`D` are perspective-only in interactive mode.
docs/changelog.rst	Adds an Unreleased entry describing the breaking projection API change and interactive key changes.
docs/api.rst	Adds API docs entries for `Orthographic` and `Perspective`.
docs/_static/generate_images.py	Migrates docs image generation script to use projection setters.
.gitignore	Adds ignore rules for planning/docs scratch artifacts.

Suppressed comments (1)

src/hofmann/rendering/interactive.py:76

The interactive help overlay still advertises d/D as general “Distance” controls, but _apply_key_action now makes them no-ops unless the view is already in Perspective. The help text should reflect that these keys are perspective-only to avoid misleading users.

_MIN_VIEW_DISTANCE = 0.1  # closest the eye may be brought to the centre

_HELP_TEXT = """\
Arrows     Rotate          Shift+Arrows  Pan
,  .       Roll            +  =  -       Zoom

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Preserve the viewing distance across an orthographic excursion. The sum type correctly makes view_distance unrepresentable while parallel, but nothing then remembered it, so P-to-orthographic followed by p silently reset a dialled-in distance to the default. The session state now carries the outgoing Perspective, which is where per-session UI memory already lives. P, d and D return "none" under a parallel projection rather than forcing a redraw for a guaranteed no-op, matching the file's own idiom. Cover the gaps behind three green-but-empty tests, each confirmed by mutation: zoom was never exercised under perspective, so deleting it from project_camera passed; the perspective floor was pinned from a start value whose residue was negative, so a bare > 0.0 passed; and sub-segment depths could be paired with the wrong screen positions, which the point-on-segment check cannot see, being invariant under permutation. Rename _eye_distance to _foreshortening_distance: it returns view_distance, but the eye sits at view_distance / strength, so the old name asserted something this branch's own Perspective docstring contradicts. The silhouette radius carries the same approximation and now says so at the line. Close the projection sum at _scene_extent, export a Projection alias for the union, document the view_distance migration, and correct the comment nits.

Copilot

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/hofmann/rendering/bond_geometry.py:32

_foreshortening_distance() uses a match without a catch-all branch. If a new projection mode is added, this function will silently fall through and return None, causing downstream type errors at runtime and undermining the stated goal of having assert_never enforce exhaustiveness.

    match view.projection:
        case Perspective() as p:
            return p.view_distance
        case Orthographic():
            return _PARALLEL_EYE_DISTANCE

src/hofmann/rendering/interactive.py:281

Reset (r) restores view.projection, but it does not reset state["last_perspective"]. After a reset to Orthographic(), pressing p can therefore restore a stale viewing distance/strength from earlier in the session rather than the initial/default perspective settings (the old view_distance reset behavior).

    elif key == "r":
        view.rotation = initial_view["rotation"].copy()
        view.zoom = initial_view["zoom"]
        view.centre = initial_view["centre"].copy()
        view.projection = initial_view["projection"]

test_modes_reject_unknown_attributes pinned TypeError, which is a CPython detail that moved: through 3.12 the slots check fires first and raises TypeError, from 3.13 the frozen check fires first and raises FrozenInstanceError. CI caught it on 3.13 and 3.14. Assert the behaviour that actually matters instead -- the write is refused and leaves nothing behind -- and accept either type. Close the projection sum in _foreshortening_distance, which returned None by fallthrough for an unrecognised mode rather than failing the type check like the other dispatch sites. Reset now clears the stashed perspective as well as the projection. Without it, a distance dialled in before a reset could be restored by a later p, so reset did not fully reset.

Copilot

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated no new comments.

project_camera is the single mapping every geometry path routes through, so guard the degenerate case there once rather than in each renderer. A point at or behind the perspective eye plane is drawn mirrored through the origin at an ordinary radius and sorted as if nearest the viewer, which renders as a plausible ghost rather than anything obviously wrong. The scale is left infinite or negative rather than clamped, so the geometry stays reproducible and the warning is what tells the caller. Three fixes from the previous round were unguarded, each confirmed by mutation in a sandbox with imports pinned away from the live tree: the no-op redraw kind was discarded by its test, the frozen-instance test passed on frozen alone so slots could be dropped, and the seeded perspective's strength was overwritten on read. Trim the interactive documentation back to the contract. Stepping out to an orthographic view at the bottom of the ladder is how the keys have always behaved; that the session remembers the viewing distance across the excursion is bookkeeping the reader does not need. State the silhouette approximation's symptom accurately: the error does not vanish as strength falls, so a radius does not converge to its orthographic value.

Copilot

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated no new comments.

set_perspective accepts any positive strength, so the ladder must step by a fixed increment from wherever a caller leaves it rather than assuming a grid. Nothing covered a starting strength that was not a multiple of the step; a change that snapped to one passed the suite.

Copilot

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated no new comments.

project_camera warns when the scene reaches the perspective eye plane; _scene_extent has the same denominator and the same blank-canvas pathology but stayed silent, so the two sites disagreed. Warn here too. This is the site reachable from the keyboard -- p to full strength, then hold D. The magnification maths is untouched; the degenerate case is now loud rather than silent, which was the policy set for project_camera. Alphabetise Orthographic/Perspective/Projection in __all__ (the import statement was ordered earlier but the export list was missed), and state the reason the silhouette radius keeps its approximate form: bond end caps foreshorten to the same reference distance, so the two must be corrected together or the atoms desync from their bonds.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

The two export lists used different orderings for the same names. Sort the model list the way the top-level one already is, rather than moving only the three projection names and leaving the file split between two schemes.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Follow-up to #85. Moves the projection maths off ViewState/renderer match dispatch onto a Projection abstract base class implemented by Orthographic and Perspective, so adding a third variant is a one-class change rather than an edit to every dispatch site. Projection becomes an ABC (the LegendItem pattern) with five methods -- to_screen, silhouette_radius, max_magnification, eye_distance, reaches_eye_plane. The four match/isinstance dispatch sites collapse to method calls, the assert_never guards are replaced by definition-time enforcement, and the three types move into hofmann/model/projection.py, returning view_state.py to being about ViewState (434 to 254 lines). Rendered output is unchanged, bit-for-bit, verified with a vertex-level harness after every commit. project_camera now returns xy only (the scale it returned was discarded by every caller), and the duplicated foreshortening-distance helper collapses to an eye_distance property.

bjmorgan added 7 commits August 16, 2026 07:15

Copilot AI lite review requested due to automatic review settings August 16, 2026 07:18

Copilot started reviewing on behalf of bjmorgan August 16, 2026 07:19 View session