Skip to content

Commit 0130e4c

Browse files
asinghvi17claude
andcommitted
Fix register_positions_projected! not updating when float32convert changes
When projecting from a non-data space (e.g. :relative, :pixel) to a data space, the camera matrices give f32c-scaled coordinates because the camera projection is set up in f32c space. To get actual data coordinates, we need to apply the inverse of float32convert to the output positions. This fix: - Detects when we're projecting from non-data space to data space - Adds :f32c as a dependency so output updates when float32convert changes - Applies inv(f32c) to convert f32c-scaled coordinates to data coordinates Note: This does not yet handle transform_func for these projections. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 4a67281 commit 0130e4c

File tree

2 files changed

+86
-2
lines changed

2 files changed

+86
-2
lines changed

Makie/src/utilities/projection_utils.jl

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,15 @@ function register_positions_projected!(
147147

148148
# apply projection
149149
# clip planes only apply from data/world space.
150-
if apply_clip_planes && (is_data_space(input_space) || input_space === :space)
150+
151+
# When projecting from a non-data space (e.g. :relative, :pixel) to data space,
152+
# we need to apply the inverse of float32convert. The camera matrices operate
153+
# in f32c-scaled space, so to get actual data coordinates we must undo the scaling.
154+
input_is_data_space = is_data_space(input_space) || input_space === :space
155+
output_is_data_space = is_data_space(output_space) || output_space === :space
156+
needs_inv_f32c = output_is_data_space && !input_is_data_space
157+
158+
if apply_clip_planes && input_is_data_space
151159
# easiest to transform them to the space of the projection input and
152160
# clip based on those points
153161
if apply_model
@@ -167,8 +175,24 @@ function register_positions_projected!(
167175
end
168176
end
169177

178+
elseif needs_inv_f32c
179+
# Projecting from non-data space to data space: apply inverse f32c.
180+
# f32c in the compute graph is a LinearScaling, which is callable.
181+
# inv(f32c) returns the inverse LinearScaling that we broadcast over positions.
182+
if output_space === :space # dynamic output space
183+
map!(plot_graph, [merged_matrix_name, input_name, :f32c, :space], output_name) do matrix, pos, f32c, space
184+
projected = _project(OT, matrix, pos)
185+
return is_data_space(space) ? inv(f32c).(projected) : projected
186+
end
187+
else # static output space (:data)
188+
map!(plot_graph, [merged_matrix_name, input_name, :f32c], output_name) do matrix, pos, f32c
189+
projected = _project(OT, matrix, pos)
190+
return inv(f32c).(projected)
191+
end
192+
end
193+
170194
else
171-
# no clip planes, just project everything
195+
# no clip planes, no inv_f32c needed, just project everything
172196
map!(plot_graph, [merged_matrix_name, input_name], output_name) do matrix, pos
173197
return _project(OT, matrix, pos)
174198
end

Makie/test/conversions/float32convert.jl

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -319,4 +319,64 @@ using Makie: Mat4f, Vec2d, Vec3d, Point2d, Point3d, Point4d
319319
end
320320
end
321321

322+
@testset "register_positions_projected! with float32convert changes" begin
323+
# Test that projecting from relative space to data space correctly
324+
# applies inverse float32convert and updates when f32c changes.
325+
# This is important for features like pyramids that place markers
326+
# at relative screen positions but need data-space coordinates.
327+
328+
f, a, p = lines(1:10 .* 1_000_000, 1:10 .* 1_000_000)
329+
330+
# Add input positions in relative space (0-1 range representing screen corners)
331+
input_positions = [Point2f(0, 0), Point2f(1, 1)]
332+
Makie.add_input!(p.attributes, :test_input_positions, input_positions)
333+
Makie.register_positions_projected!(
334+
p;
335+
input_space = :relative,
336+
output_space = :space,
337+
input_name = :test_input_positions,
338+
output_name = :test_dataspace_positions,
339+
)
340+
Makie.update_state_before_display!(f)
341+
342+
# Initially, float32convert should be identity (data range fits in Float32)
343+
@test is_identity_transform(a.scene.float32convert)
344+
345+
# Get initial output positions - should map relative corners to data limits
346+
initial_positions = p.test_dataspace_positions[]
347+
initial_limits = a.finallimits[]
348+
349+
# Positions should approximately match the axis limits
350+
# (relative (0,0) -> min of limits, relative (1,1) -> max of limits)
351+
@test initial_positions[1][1] initial_limits.origin[1] rtol = 0.1
352+
@test initial_positions[1][2] initial_limits.origin[2] rtol = 0.1
353+
@test initial_positions[2][1] initial_limits.origin[1] + initial_limits.widths[1] rtol = 0.1
354+
@test initial_positions[2][2] initial_limits.origin[2] + initial_limits.widths[2] rtol = 0.1
355+
356+
# Now zoom into a very small region to trigger float32convert
357+
limits!(a, (500_000, 500_100), (500_000, 500_100))
358+
Makie.update_state_before_display!(f)
359+
360+
# float32convert should now be non-identity due to precision requirements
361+
@test !is_identity_transform(a.scene.float32convert)
362+
363+
# Get updated output positions
364+
updated_positions = p.test_dataspace_positions[]
365+
updated_limits = a.finallimits[]
366+
367+
# After zooming, the relative positions should now map to the new limits
368+
# relative (0,0) -> new min, relative (1,1) -> new max
369+
@test updated_positions[1][1] updated_limits.origin[1] rtol = 0.01
370+
@test updated_positions[1][2] updated_limits.origin[2] rtol = 0.01
371+
@test updated_positions[2][1] updated_limits.origin[1] + updated_limits.widths[1] rtol = 0.01
372+
@test updated_positions[2][2] updated_limits.origin[2] + updated_limits.widths[2] rtol = 0.01
373+
374+
# The key test: positions should be in actual data coordinates, not f32c-scaled
375+
# After zoom, updated_limits should be around (500_000, 500_000) to (500_100, 500_100)
376+
@test 499_000 < updated_positions[1][1] < 501_000
377+
@test 499_000 < updated_positions[1][2] < 501_000
378+
@test 499_000 < updated_positions[2][1] < 501_000
379+
@test 499_000 < updated_positions[2][2] < 501_000
380+
end
381+
322382
end

0 commit comments

Comments
 (0)