Skip to content

Commit 9b947a4

Browse files
asinghvi17claudeffreyer
authored
Fix register_positions_projected! not updating when float32convert changes (#5485)
* 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> * Add complete inverse transformation support to register_projected_positions! - Add apply_inverse_transform, apply_inverse_transform_func, apply_inverse_float32convert, and apply_inverse_model kwargs - These enable correct projection from non-data spaces back to data space - Add early-exit optimization to skip redundant transform/inverse pairs when input_space === output_space - Add apply_inverse_model_to_positions helper function - Update docstring with new kwargs - Add tests for inverse transform_func 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Run runic formatter Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Fix Runic weirdness * move inverse model to projections, cleanup decisions, simplify inverse transform * use Mat4d, pick model based on f32c, tweak defaults * refactor inverse float32convert to apply as matrix, fixing inverse f32c with non identity model matrix * fix tests * fix format * fix CairoMakie --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: ffreyer <frederic481994@hotmail.de>
1 parent 0b615f0 commit 9b947a4

6 files changed

Lines changed: 310 additions & 40 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
## Unreleased
44

5+
- Added complete inverse transformation support to `register_projected_positions!` with `apply_inverse_transform`, `apply_inverse_transform_func`, `apply_inverse_float32convert`, and `apply_inverse_model` kwargs. These enable correct projection from non-data spaces back to data space. Includes early-exit optimization to skip redundant transform/inverse pairs when `input_space === output_space`. [#5485](https://github.com/MakieOrg/Makie.jl/pull/5485)
56
- Fixed `bracket` not supporting `LaTeXString` text, which would render with dollar signs instead of mathematical notation [#5536](https://github.com/MakieOrg/Makie.jl/pull/5536)
67
- Added text glow to CairoMakie [#5542](https://github.com/MakieOrg/Makie.jl/pull/5542)
78
- Allow to set low or high bound of the colorrange and let the other side stay adaptive [#5555](https://github.com/MakieOrg/Makie.jl/pull/5555)

Makie/src/float32-scaling.jl

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,8 @@ function f32_convert_matrix(ls::LinearScaling)
6464
translation = to_ndim(Vec3d, ls.offset, 0)
6565
return transformationmatrix(translation, scale)
6666
end
67+
inv_f32_convert_matrix(ls::LinearScaling) = f32_convert_matrix(inv(ls))
68+
6769
function f32_convert_matrix(ls::LinearScaling, space::Symbol)
6870
return is_data_space(space) ? f32_convert_matrix(ls) : Mat4d(I)
6971
end

Makie/src/utilities/projection_utils.jl

Lines changed: 152 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,33 @@ to allow clip space clipping to happen elsewhere.)
1616
- `output_space = :pixel` sets the output space. Can be `:space` or `:markerspace` to refer to those plot attributes or any static space like `:pixel`.
1717
- `input_name = :positions` sets the source positions which will be projected.
1818
- `output_name = Symbol(output_space, :_, input_name)` sets the name of the projected positions.
19-
- `apply_transform = input_space === :space` controls whether transformations and float32convert are applied.
19+
- `apply_transform = input_space === :space` controls whether forward transformations and float32convert are applied.
2020
- `apply_transform_func = apply_transform` controls whether `transform_func` is applied.
21-
- `apply_float32convert = apply_transform` controls whether `float32convert` is applied.
21+
- `apply_float32convert = apply_transform && output_space != :space` controls whether `float32convert` is applied.
2222
- `apply_model = apply_transform` controls whether the `model` matrix is applied.
2323
- `apply_clip_planes = false` controls whether points clipped by `clip_planes` are replaced by NaN. (Does not consider clip space clipping. Only applies if `is_data_space(input_space)`.)
24+
- `apply_inverse_transform = output_space === :space && !apply_transform` controls whether inverse transformations are applied when projecting to data space.
25+
- `apply_inverse_transform_func = apply_inverse_transform` controls whether inverse `transform_func` is applied.
26+
- `apply_inverse_float32convert = apply_inverse_transform` controls whether inverse `float32convert` is applied.
27+
- `apply_inverse_model = apply_inverse_transform` controls whether inverse `model` matrix is applied.
2428
- `yflip = false` flips the `y` coordinate if set to true and `output_space = :pixel`
2529
30+
## Order of projections and transformations
31+
32+
The order in which transformations and projections apply to the positions
33+
referred to by `input_name` is:
34+
35+
1. transform func
36+
2. model
37+
3. float32convert
38+
4. projections
39+
1. clip planes
40+
2. input_space -> output_space projection
41+
3. yflip
42+
5. inverse float32convert
43+
6. inverse model
44+
7. inverse transform func
45+
2646
Related: [`register_position_transforms!`](@ref), [`register_positions_transformed!`](@ref),
2747
[`register_positions_transformed_f32c!`](@ref), [`register_projected_rotations_2d!`](@ref)
2848
"""
@@ -41,39 +61,125 @@ function register_projected_positions!(
4161
input_name::Symbol = :positions,
4262
output_name::Symbol = Symbol(output_space, :_, input_name),
4363
yflip::Bool = false,
64+
# Forward transforms
4465
apply_transform::Bool = input_space === :space,
4566
apply_transform_func::Bool = apply_transform,
46-
apply_float32convert::Bool = apply_transform,
67+
apply_float32convert::Bool = apply_transform && !is_data_space(output_space),
4768
apply_model::Bool = apply_transform,
4869
apply_clip_planes::Bool = false,
70+
# Inverse transforms
71+
apply_inverse_transform::Bool = output_space === :space && !apply_transform,
72+
apply_inverse_transform_func::Bool = apply_inverse_transform,
73+
apply_inverse_float32convert::Bool = apply_inverse_transform,
74+
apply_inverse_model::Bool = apply_inverse_transform,
4975
) where {OT <: VecTypes}
5076

51-
# Handle transform function + f32c
77+
#=
78+
- projections (space) and transformations (transform func, model) are orthogonal,
79+
meaning they don't depend on each other
80+
- float32convert is special:
81+
- (forward) f32c only applies when the input space is :data (or dynamic
82+
data via :space, :markerspace), because the other spaces can't have
83+
float32 precision issues
84+
- plots apply f32c based on their space, so it's generally desirable to
85+
have f64 (pre f32c) outputs here when targeting :data space
86+
87+
With inverse transforms we have:
88+
1. apply transform func
89+
2. apply float32convert
90+
3. apply model_f32c
91+
4. apply projection (input_space -> output_space) (+ clip planes, yflip)
92+
5. apply inverse model_f32c
93+
6. apply inverse float32convert
94+
7. apply inverse transform func
95+
96+
To figure out what can be skipped we need to figure out which parts combine
97+
to identities.
98+
99+
Note that the order of float32convert and model is swapped so that GLMakie
100+
and WGLMakie can apply a Float32 safe version of the model matrix on the GPU.
101+
f32c(model(data)) = model_f32(f32c(data))
102+
=#
103+
104+
# We can statically skip steps if they combine to an identity regardless of
105+
# what other steps do. This is the case if everything between the step and
106+
# its inverse is also an identity (statically).
107+
# There can still be dynamic identities, but these require computations to
108+
# exist for dynamic evaluation.
109+
is_static_identity_camera_projection = input_space === output_space && !yflip
110+
is_static_identity_projection = is_static_identity_camera_projection && (apply_model === apply_inverse_model)
111+
is_static_identity_f32convert = is_static_identity_projection && (apply_float32convert === apply_inverse_float32convert)
112+
is_all_static_identity = is_static_identity_f32convert && (apply_transform_func == apply_inverse_transform_func)
113+
114+
if is_all_static_identity
115+
ComputePipeline.alias!(plot_graph, input_name, output_name)
116+
return getindex(plot_graph, output_name)
117+
end
118+
119+
current_output = input_name
120+
121+
# Handle forward transform function
52122
if apply_transform_func
53-
transformed_name = Symbol(input_name, :_transformed)
54-
register_positions_transformed!(plot_graph; input_name, output_name = transformed_name)
55-
else
56-
transformed_name = input_name
123+
# This checks if plot.space[] == :data.
124+
# input_space == :data does not overwrite this
125+
transformed_name = Symbol(current_output, :_transformed)
126+
register_positions_transformed!(plot_graph; input_name = current_output, output_name = transformed_name)
127+
current_output = transformed_name
128+
end
129+
130+
# Handle forward float32convert
131+
if apply_float32convert && !is_static_identity_f32convert
132+
transformed_f32c_name = Symbol(current_output, :_f32c)
133+
register_positions_transformed_f32c!(plot_graph; input_name = current_output, output_name = transformed_f32c_name)
134+
current_output = transformed_f32c_name
57135
end
58136

59-
if apply_float32convert && !is_data_space(output_space)
60-
transformed_f32c_name = Symbol(transformed_name, :_f32c)
61-
register_positions_transformed_f32c!(plot_graph; input_name = transformed_name, output_name = transformed_f32c_name)
137+
# Use intermediate name for projection output if transform_func still needs to apply
138+
projection_output = if apply_inverse_transform_func
139+
apply_inverse_float32convert ? Symbol(current_output, :_projected_f32c) : Symbol(current_output, :_projected)
62140
else
63-
# Pipeline will apply f32c if the input space is data space, so we
64-
# should avoid it here. TODO: also dynamically
65-
transformed_f32c_name = transformed_name
141+
output_name
142+
end
143+
144+
# create inverse float32convert matrix to include at the end of projections
145+
postfix_matrix = nothing
146+
if apply_inverse_float32convert && !is_static_identity_f32convert
147+
if output_space == :space
148+
map!(inv_f32_convert_matrix, plot_graph, [:f32c, :space], :dynamic_inverse_f32c_matrix)
149+
postfix_matrix = :dynamic_inverse_f32c_matrix
150+
elseif is_data_space(output_space)
151+
map!(inv_f32_convert_matrix, plot_graph, :f32c, :inverse_f32c_matrix)
152+
postfix_matrix = :inverse_f32c_matrix
153+
end
66154
end
67155

68-
if apply_model || (input_space !== output_space) || yflip
156+
# apply projections + optional inverse float32convert matrix.
157+
# At most, this includes:
158+
# 1. model or model_f32, based on `model_name`, if `apply_model = true`
159+
# 2. projection matrix, based on `input_space` and `output_space`
160+
# 3. inverse model, if `apply_inverse_model = true`
161+
# 4. inverse float32convert, if postfix_matrix is set accordingly
162+
if !is_static_identity_projection || (apply_inverse_float32convert && !is_static_identity_f32convert)
69163
register_positions_projected!(
70164
scene_graph, plot_graph, OT;
71165
input_space, output_space,
72-
input_name = transformed_f32c_name, output_name,
73-
yflip, apply_model, apply_clip_planes
166+
input_name = current_output, output_name = projection_output,
167+
model_name = ifelse(apply_float32convert, :model_f32c, :model),
168+
yflip, apply_model, apply_inverse_model, apply_clip_planes, postfix_matrix
74169
)
75-
else
76-
ComputePipeline.alias!(plot_graph, transformed_f32c_name, output_name)
170+
current_output = projection_output
171+
end
172+
173+
if apply_inverse_transform_func
174+
map!(inverse_transform, plot_graph, :transform_func, :inverse_transform_func)
175+
# Use Makie. prefix to avoid shadowing by kwarg `apply_transform::Bool`
176+
map!(Makie.apply_transform, plot_graph, [:inverse_transform_func, current_output], output_name)
177+
current_output = output_name
178+
end
179+
180+
# Alias to final output name if different
181+
if current_output !== output_name
182+
ComputePipeline.alias!(plot_graph, current_output, output_name)
77183
end
78184

79185
return getindex(plot_graph, output_name)
@@ -117,34 +223,52 @@ function register_positions_projected!(
117223
output_space::Symbol = :pixel,
118224
input_name::Symbol = :positions_transformed_f32c,
119225
output_name::Symbol = Symbol(output_space, :_positions),
226+
model_name::Symbol = :model_f32c,
227+
postfix_matrix::Union{Nothing, Symbol} = nothing,
120228
yflip::Bool = false,
121229
apply_model::Bool = input_space === :space,
230+
apply_inverse_model::Bool = false,
122231
apply_clip_planes::Bool = false
123232
) where {OT <: VecTypes}
124233

125234
# Connect necessary projection matrix from scene
126235
projection_matrix_name = register_camera_matrix!(scene_graph, plot_graph, input_space, output_space)
127-
merged_matrix_name = Symbol(ifelse(yflip, "yflip_", "") * string(projection_matrix_name) * "_model")
236+
merged_matrix_name = Symbol(
237+
ifelse(postfix_matrix isa Symbol, "$(postfix_matrix)_", "") *
238+
ifelse(apply_inverse_model, "inverse_model_", "") *
239+
ifelse(yflip, "yflip_", "") *
240+
string(projection_matrix_name) *
241+
ifelse(apply_model, "_$model_name", "") * "4d"
242+
)
128243

129-
# connect resolution for yflip (Cairo) and model matrix if requested
130244
inputs = Symbol[]
245+
246+
if postfix_matrix isa Symbol
247+
push!(inputs, postfix_matrix)
248+
end
249+
250+
if apply_inverse_model
251+
map!(inv, plot_graph, :model, :inverse_model)
252+
push!(inputs, :inverse_model)
253+
end
254+
255+
# connect resolution for yflip (Cairo) and model matrix if requested
131256
if yflip
132257
is_pixel_space(output_space) || error("`yflip = true` is currently only allowed when targeting pixel space")
133258
if !haskey(plot_graph, :resolution)
134259
add_input!(plot_graph, :resolution, scene_graph[:resolution])
135260
end
136-
push!(inputs, :resolution)
261+
map!(plot_graph, :resolution, :yflip_matrix) do res
262+
return transformationmatrix(Vec3d(0, res[2], 0), Vec3d(1, -1, 1))
263+
end
264+
push!(inputs, :yflip_matrix)
137265
end
138266

139267
push!(inputs, projection_matrix_name)
140-
# in data space f32c is not applied and we should use the plain model matrix
141-
apply_model && push!(inputs, ifelse(is_data_space(output_space), :model, :model_f32c))
268+
apply_model && push!(inputs, model_name)
142269

143270
# merge/create projection related matrices
144-
combine_matrices(res::Vec2, pv::Mat4, m::Mat4) = Mat4f(_flip_matrix(res) * pv * m)::Mat4f
145-
combine_matrices(res::Vec2, pv::Mat4) = Mat4f(_flip_matrix(res) * pv)::Mat4f
146-
combine_matrices(pv::Mat4, m::Mat4) = Mat4f(pv * m)::Mat4f
147-
combine_matrices(pv::Mat4) = Mat4f(pv)::Mat4f
271+
combine_matrices(matrices...) = Mat4d(foldl(*, matrices))::Mat4d
148272
map!(combine_matrices, plot_graph, inputs, merged_matrix_name)
149273

150274
# apply projection
@@ -184,10 +308,6 @@ function register_model_clip_planes!(attr, modelname = :model_f32c)
184308
return
185309
end
186310

187-
function register_f32c_matrix!(attr)
188-
return map!(attr, :f32c, :f32c_matrix)
189-
end
190-
191311
################################################################################
192312

193313
function register_markerspace_positions!(@nospecialize(plot::Plot), ::Type{OT} = Point3f; kwargs...) where {OT}

Makie/test/conversions/float32convert.jl

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -318,5 +318,4 @@ using Makie: Mat4f, Vec2d, Vec3d, Point2d, Point3d, Point4d
318318
end
319319
end
320320
end
321-
322321
end

0 commit comments

Comments
 (0)