Skip to content

Fix ViewState element type, :rotation interpolation, Pause action, and record frame count - #35

Open
asinghvi17 wants to merge 13 commits into
mainfrom
fix-viewstate-eltype-and-rotation-slerp
Open

Fix ViewState element type, :rotation interpolation, Pause action, and record frame count#35
asinghvi17 wants to merge 13 commits into
mainfrom
fix-viewstate-eltype-and-rotation-slerp

Conversation

@asinghvi17

@asinghvi17 asinghvi17 commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Bug fixes found while driving a 122 s GeoMakie globe animation built from ~750 chained moves. Each commit is one fix and carries a regression test.

  • Pause with an action threw UndefVarError (src/pathchange.jl): the method binds pause, but the body said move. The 2-argument Pause(duration, action) constructor the docstring advertised did not exist either, so the line was unreachable from outside.
  • ViewState(; ...) hard-coded Float32 (src/viewstate.jl): the element type is now promoted from the arguments, so a path built from Point3d/Float64 values stays in Float64 instead of accumulating segment times in Float32.
  • ConstrainedMove(...; constraint = :rotation) was not a slerp (src/pathchange.jl): cospi(f/2)*vold + sinpi(f/2)*vnew preserves norm(eyeposition - lookat) only at 90°; at 0° the camera flies 41% further out and back, and at 180° it passes through the point it is looking at. Now a real rotation of the direction plus a separate interpolation of the length.
  • path(t) rejected times at its own segment boundaries (src/path.jl): tend is accumulated separately from t, so t - tend can exceed the change's duration by an ulp and checkt then throws for a time that had just been assigned to that change. The local time is clamped at the call site; checkt still rejects genuinely out-of-range input.
  • The plotcamerapath recipe could not be instantiated on Makie 0.24 (ext/FlyThroughPathsMakieExt.jl): Makie.inherit(scene, ...) inside an @recipe ... do scene block throws MethodError: no method matching lookup_default, and arrows! no longer dispatches from within a recipe. Ported to the current @recipe Name (args) begin ... end form and to arrows3d!.

Also adds nframes(path, rate) and a vectorized path(ts) that walks a sorted vector of sample times in one pass (both used by the recipe), and makes LinearAlgebra a dependency.

Decisions worth a look

  1. Integer arguments keep the Float32 default. ViewState(eyeposition = [-10, 0, 0], fov = 45) is still ViewState{Float32}, matching the README, the docs, and the existing show round-trip test. Say the word if you would rather have float(promote_type(...)) throughout.
  2. The slerp interpolates length geometrically, dold * (dnew/dold)^f, so equal endpoint radii stay exactly constant and a dolly moves at a constant relative rate. A test pins the midpoint of a 10 → 5 move at sqrt(50); linear is a one-line change.
  3. The Makie compat bound narrows to 0.24. The recipe rewrite uses @recipe ... begin ... end and arrows3d!, neither of which exists across the whole 0.210.24 range the bound previously claimed. Supporting the older versions as well would mean version-conditional code in the extension.
  4. The recipe's camera_markersize::Vec3f becomes camera_markerscale::Real, since arrowsize has no equivalent in Arrows3D. It defaults to automatic, scaled from the bounding box of the path — Makie's own automatic sizes the arrow from its own bounding box, which is a single unit-length arrow and comes out invisible. The unused camera_marker attribute is dropped.

🤖 Generated with Claude Code

asinghvi17 and others added 8 commits August 7, 2026 14:23
`(::Pause)(view, t)` computed `t / duration(move)`, but the method binds
the change to `pause`, so any `Pause` carrying an action threw an
`UndefVarError` when evaluated.

The docstring already advertised `Pause(duration, [action])`, but only the
1-argument method existed, so an action could not be attached through the
public API at all. Give `action` a default in that method instead.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`path(t)` walks the changes accumulating `tend`, and a change is selected
when `t <= tend + duration(change)`. Because `tend` is accumulated
separately from the caller's `t`, `t - tend` can exceed `duration(change)`
by an ulp, and `checkt` then rejects a time we had just decided belongs to
that change.

This is not a Float32-only effect: a five-segment `Path{Float64}` of 0.2 s
moves already throws `ArgumentError: t=0.20000000000000007 is not in
[0, 0.2]` at `nextfloat(0.6)`.

Clamp at the call site instead of weakening `checkt`, which should still
catch genuinely out-of-range input.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ViewState(; kwargs...)` hard-coded `ViewState{Float32}`, so
`ViewState(eyeposition = Point3d(...), fov = 40.0)` silently narrowed to
Float32. Every `ConstrainedMove` built against such a state is then a
`PathChange{Float32}`, and `path(t)` accumulates its segment start times in
Float32; across ~750 chained moves spanning 122 s the drift reaches ~1e-6 s,
which is enough to push a frame time past a segment's end.

The element type is now promoted over the supplied values. Integers
(and the all-`nothing` case) express no preference about precision, so they
keep the `Float32` default that Makie cameras use and that the README and
docs show; `ViewState{T}(; ...)` is unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ConstrainedMove(...; constraint = :rotation)` blended the two offsets from
the lookat point as `cospi(f/2) * vold + sinpi(f/2) * vnew`. With
`d = norm(vold) = norm(vnew)` and `θ` the angle between them, that has
squared length `d^2 * (1 + sinpi(f) * cos(θ))`, i.e. the distance to the
lookat point is preserved only at `θ = 90°`. A "stationary" rotation
(`θ = 0`) swells the radius to `d*sqrt(2)` halfway through, flying the
camera 41% further out and back, and an antipodal one (`θ = 180°`) takes the
radius through zero, i.e. through the point being looked at. Only the
interior of the move was wrong; both endpoints were already correct.

Replace it with a real slerp of the direction plus a separate interpolation
of the length, so the distance to the lookat point moves monotonically
between the endpoint distances for every `θ`. Degenerate cases: a zero-length
offset falls back to a lerp, `θ ≈ 0` uses the chord (the great circle is
degenerate there), and `θ ≈ 180°` is ill-conditioned, so the rotation plane
is chosen deterministically from the axis `vold` is least aligned with.

The length is interpolated geometrically rather than linearly, a constant
relative rate of approach reading more evenly for a camera dolly.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`record(fig, file, path::Path)` sampled the path with
`round(Int, tend / framerate)` frames. That is inverted: the number of
frames is `tend * framerate`. Every path shorter than about 1.5x the
framerate produced a degenerate range, e.g. a 10 s path at 24 fps asked
`LinRange` for `round(Int, 10/24) == 0` points.

Factor the count into `FlyThroughPaths.nframes(path, rate)`, which never
returns fewer than two samples, and use it for the recipe's `density`
sampling too (`tend*density` was already the right expression there, but it
rounds to 0 for a path shorter than one sampling interval).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`record(fig, file, path::Path)` materialized `path.(trange)` and passed that
vector of `ViewState`s straight back to `Makie.record`, which threw
`MethodError: no method matching record(::Figure, ::String,
::Vector{ViewState{Float64}})` — `Makie.record` takes the per-frame function
first, and nothing in the old body applied the views to the figure anyway.
So the method could not work regardless of the frame count.

Iterate over the sample times with a function that sets the view. For a
`Figure` that view goes to its current axis, via new `set_view!` methods for
`Figure` and `FigureAxisPlot` that complete the `Scene`/`AbstractAxis` pair.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`capture_view` now preserves the element type of the camera it reads, which
is Float64 for `Camera3D` in recent versions of Makie.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The `Path` method of `record` is the one CI cannot reach, so drive it from
the script that already needs a backend.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@codecov

codecov Bot commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.50000% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.72%. Comparing base (9bb201c) to head (5fa8a57).

Files with missing lines Patch % Lines
ext/FlyThroughPathsMakieExt.jl 0.00% 8 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main      #35       +/-   ##
===========================================
+ Coverage   65.00%   77.72%   +12.72%     
===========================================
  Files           5        5               
  Lines         180      229       +49     
===========================================
+ Hits          117      178       +61     
+ Misses         63       51       -12     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread ext/FlyThroughPathsMakieExt.jl Outdated
Comment thread ext/FlyThroughPathsMakieExt.jl Outdated
Comment thread src/pathchange.jl
asinghvi17 and others added 3 commits August 7, 2026 16:26
Interpolate the direction as `cos(f*θ)*a + sin(f*θ)*dir`, with `dir` the unit
tangent `normalize((a × b) × a)` at `a`, as `GeometryOps.UnitSpherical.slerp`
does after S2.  The plane of rotation is now taken from `a × b`, which stays
accurate to a few `eps` however close the endpoints are to antipodal, so a
move that is nearly a half turn follows the great circle its endpoints
actually determine instead of falling back to an arbitrary axis.  Only an
exact half turn, where no plane is determined, still picks one arbitrarily.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The scalar `path(t)` restarts its walk over the path's changes on every call,
both to find the change that owns `t` and to accumulate the `ViewState` it
starts from.  `path(ts)` does that walk once for a sorted vector of times and
then locates each time with `searchsortedfirst` over the segment end times,
giving results identical to `path.(ts)`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`set_view!(::Figure, ...)` guessed its target with `current_axis`; drop it.
`record` therefore takes the object being flown -- a `Scene`, an axis, or a
`FigureAxisPlot`, which names its axis -- rather than a figure whose axis has
to be guessed.  Recording an axis renders the whole figure it belongs to.
The frames come from the vectorized `path(trange)`, so the path is walked
once rather than searched per frame; the `plotcamerapath` recipe samples the
same way.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@asinghvi17
asinghvi17 marked this pull request as ready for review August 8, 2026 20:14
asinghvi17 and others added 2 commits August 8, 2026 16:29
`Makie.record(figlike, file, path)` did not earn its keep: it only saved
writing the `do` block that the docs already show, and it had to decide
which object a path drives. Drop it, along with the `set_view!` method for
`FigureAxisPlot` that existed to support it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The recipe could not be instantiated at all: `Makie.inherit(scene, ...)` in
an `@recipe ... do scene` block throws `MethodError: no method matching
lookup_default(::Observable{Any}, ::Attributes)`, and `arrows!` no longer
dispatches from inside a recipe.

Move to the current `@recipe Name (args) begin ... end` form with
`@inherit`, and to `arrows3d!` with `align = :tail` (the old `:headstart`)
and `shading = true` (`MultiLightShading` is deprecated). `arrowsize` is
gone, so the vector `camera_markersize` becomes a scalar
`camera_markerscale`; it defaults to `automatic`, sized from the bounding
box of the path rather than from the arrow itself, which made it invisible.
Drops the unused `camera_marker` attribute, and narrows the `Makie` compat
bound to the version this is tested against.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@asinghvi17
asinghvi17 marked this pull request as draft August 8, 2026 20:30
@asinghvi17

Copy link
Copy Markdown
Collaborator Author

Also fixed the plotcamerapath recipe, which could not be instantiated at all on Makie 0.24 — Makie.inherit(scene, ...) inside an @recipe ... do scene block throws MethodError: no method matching lookup_default(::Observable{Any}, ::Attributes), and arrows! no longer dispatches from within a recipe. It is now on the @recipe Name (args) begin ... end form with @inherit, and on arrows3d! with align = :tail (the old :headstart) and shading = true.

Two consequences worth your judgement, both in the PR body: the Makie compat bound narrows to 0.24, since neither the new @recipe form nor arrows3d! spans the 0.210.24 range it previously claimed; and arrowsize has no Arrows3D equivalent, so the recipe's camera_markersize::Vec3f becomes a scalar camera_markerscale defaulting to automatic — sized from the bounding box of the path, because Makie's own automatic scales the arrow by its own bounding box and renders it invisible. The unused camera_marker attribute is dropped.

Verified by rendering the recipe at three times along a path with CairoMakie; the line is coloured by time and the arrow sits at the eye pointing at the lookat.

@asinghvi17
asinghvi17 marked this pull request as ready for review August 8, 2026 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant