From 4b048df21c58ddb434e695a6b8cbea299b56c5df Mon Sep 17 00:00:00 2001 From: Frederic Freyer Date: Tue, 14 May 2024 20:17:30 +0200 Subject: [PATCH] Tweak boundingbox handling for better compatability with nonlinear transforms (#3856) * directly transform Rect as default for boundingbox * revert fast path removal * fix wrong change * update changelog * cleanup * fix typo and update hexbin * add backup for NaN boundingboxes --------- Co-authored-by: Simon --- CHANGELOG.md | 4 +- src/basic_recipes/bracket.jl | 2 +- src/basic_recipes/contours.jl | 4 +- src/basic_recipes/datashader.jl | 2 +- src/basic_recipes/error_and_rangebars.jl | 2 +- src/basic_recipes/hvlines.jl | 2 +- src/basic_recipes/hvspan.jl | 2 +- src/basic_recipes/triplot.jl | 2 +- src/basic_recipes/voronoiplot.jl | 2 +- src/camera/projection_math.jl | 5 ++ src/layouting/boundingbox.jl | 18 +---- src/layouting/text_boundingbox.jl | 8 +- src/layouting/transformation.jl | 93 +++++++++++++++++++----- src/makielayout/blocks/polaraxis.jl | 6 +- src/stats/hexbin.jl | 2 +- 15 files changed, 101 insertions(+), 53 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 07ccada2e40..3207063b1a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## [Unreleased] +- `boundingbox` now relies on `apply_transform(transform, data_limits(plot))` rather than transforming the corner points of the bounding box [#3856](https://github.com/MakieOrg/Makie.jl/pull/3856) + ## [0.21.0] - 2024-05-08 - Add `voxels` plot [#3527](https://github.com/MakieOrg/Makie.jl/pull/3527). @@ -14,7 +16,7 @@ - `data_limits` now only considers plot positions, completely ignoring transformations - `boundingbox(p::Text)` is deprecated in favor of `boundingbox(p::Text, p.markerspace[])`. The more internal methods use `string_boundingbox(p)`. [#3723](https://github.com/MakieOrg/Makie.jl/pull/3723) - `boundingbox` overwrites must now include a secondary space argument to work `boundingbox(plot, space::Symbol = :data)` [#3723](https://github.com/MakieOrg/Makie.jl/pull/3723) - - `boundingbox` now always consider `transform_func` and `model` (except for Text for the time being) + - `boundingbox` now always consider `transform_func` and `model` - `data_limits(::Scatter)` and `boundingbox(::Scatter)` now consider marker transformations [#3716](https://github.com/MakieOrg/Makie.jl/pull/3716) - **Breaking** Improved Float64 compatability of Axis [#3681](https://github.com/MakieOrg/Makie.jl/pull/3681) - This added an extra conversion step which only takes effect when Float32 precision becomes relevant. In those cases code using `project()` functions will be wrong as the transformation is not applied. Use `project(plot_or_scene, ...)` or apply the conversion yourself beforehand with `Makie.f32_convert(plot_or_scene, transformed_point)` and use `patched_model = Makie.patch_model(plot_or_scene, model)`. diff --git a/src/basic_recipes/bracket.jl b/src/basic_recipes/bracket.jl index 2222170e0f9..73d41401cd9 100644 --- a/src/basic_recipes/bracket.jl +++ b/src/basic_recipes/bracket.jl @@ -122,7 +122,7 @@ function plot!(pl::Bracket) end data_limits(pl::Bracket) = mapreduce(ps -> Rect3d([ps...]), union, pl[1][]) -boundingbox(pl::Bracket, space::Symbol = :data) = transform_bbox(pl, data_limits(pl)) +boundingbox(pl::Bracket, space::Symbol = :data) = apply_transform_and_model(pl, data_limits(pl)) bracket_bezierpath(style::Symbol, args...) = bracket_bezierpath(Val(style), args...) diff --git a/src/basic_recipes/contours.jl b/src/basic_recipes/contours.jl index 487c8211c0b..39b2d6af132 100644 --- a/src/basic_recipes/contours.jl +++ b/src/basic_recipes/contours.jl @@ -352,9 +352,9 @@ function data_limits(plot::Contour{<: Tuple{X, Y, Z}}) where {X, Y, Z} return Rect3d(mini, maxi .- mini) end function boundingbox(plot::Contour{<: Tuple{X, Y, Z}}, space::Symbol = :data) where {X, Y, Z} - return transform_bbox(plot, data_limits(plot)) + return apply_transform_and_model(plot, data_limits(plot)) end # TODO: should this have a data_limits overload? function boundingbox(plot::Contour3d, space::Symbol = :data) - return transform_bbox(plot, data_limits(plot)) + return apply_transform_and_model(plot, data_limits(plot)) end diff --git a/src/basic_recipes/datashader.jl b/src/basic_recipes/datashader.jl index a94ff2d800a..09e60a7f66f 100644 --- a/src/basic_recipes/datashader.jl +++ b/src/basic_recipes/datashader.jl @@ -469,7 +469,7 @@ function Makie.plot!(p::DataShader{<:Tuple{Dict{String, Vector{Point{2, Float32} end data_limits(p::DataShader) = p._boundingbox[] -boundingbox(p::DataShader, space::Symbol = :data) = transform_bbox(p, p._boundingbox[]) +boundingbox(p::DataShader, space::Symbol = :data) = apply_transform_and_model(p, p._boundingbox[]) function convert_arguments(P::Type{<:Union{MeshScatter,Image,Surface,Contour,Contour3d}}, canvas::Canvas, operation=automatic, local_operation=identity) pixel = Aggregation.get_aggregation(canvas; operation=operation, local_operation=local_operation) diff --git a/src/basic_recipes/error_and_rangebars.jl b/src/basic_recipes/error_and_rangebars.jl index b5ad3616a0a..f463c0dc452 100644 --- a/src/basic_recipes/error_and_rangebars.jl +++ b/src/basic_recipes/error_and_rangebars.jl @@ -304,4 +304,4 @@ end # ignore whiskers when determining data limits data_limits(bars::Union{Errorbars, Rangebars}) = data_limits(bars.plots[1]) -boundingbox(bars::Union{Errorbars, Rangebars}, space::Symbol = :data) = transform_bbox(bars, data_limits(bars)) +boundingbox(bars::Union{Errorbars, Rangebars}, space::Symbol = :data) = apply_transform_and_model(bars, data_limits(bars)) diff --git a/src/basic_recipes/hvlines.jl b/src/basic_recipes/hvlines.jl index 38d12802463..165c0be42e8 100644 --- a/src/basic_recipes/hvlines.jl +++ b/src/basic_recipes/hvlines.jl @@ -95,4 +95,4 @@ function data_limits(p::VLines) return Rect3d(Point3d(xmin, NaN, 0), Vec3d(xmax - xmin, NaN, 0)) end -boundingbox(p::Union{HLines, VLines}, space::Symbol = :data) = transform_bbox(p, data_limits(p)) \ No newline at end of file +boundingbox(p::Union{HLines, VLines}, space::Symbol = :data) = apply_transform_and_model(p, data_limits(p)) \ No newline at end of file diff --git a/src/basic_recipes/hvspan.jl b/src/basic_recipes/hvspan.jl index c277660c853..cf2eb06b10c 100644 --- a/src/basic_recipes/hvspan.jl +++ b/src/basic_recipes/hvspan.jl @@ -99,6 +99,6 @@ function data_limits(p::VSpan) return Rect3d(Point3d(xmin, NaN, 0), Vec3d(xmax - xmin, NaN, 0)) end -boundingbox(p::Union{HSpan, VSpan}, space::Symbol = :data) = transform_bbox(p, data_limits(p)) +boundingbox(p::Union{HSpan, VSpan}, space::Symbol = :data) = apply_transform_and_model(p, data_limits(p)) convert_arguments(P::Type{<:Union{HSpan, VSpan}}, x::Interval) = convert_arguments(P, endpoints(x)...) diff --git a/src/basic_recipes/triplot.jl b/src/basic_recipes/triplot.jl index c3dbf30761d..8b248c215ed 100644 --- a/src/basic_recipes/triplot.jl +++ b/src/basic_recipes/triplot.jl @@ -248,4 +248,4 @@ function data_limits(p::Triplot{<:Tuple{<:Vector{<:Point}}}) return data_limits(p.plots[1]) end end -boundingbox(p::Triplot{<:Tuple{<:Vector{<:Point}}}, space::Symbol = :data) = transform_bbox(p, data_limits(p)) +boundingbox(p::Triplot{<:Tuple{<:Vector{<:Point}}}, space::Symbol = :data) = apply_transform_and_model(p, data_limits(p)) diff --git a/src/basic_recipes/voronoiplot.jl b/src/basic_recipes/voronoiplot.jl index 086302d2eca..42796a25b52 100644 --- a/src/basic_recipes/voronoiplot.jl +++ b/src/basic_recipes/voronoiplot.jl @@ -166,7 +166,7 @@ function data_limits(p::Voronoiplot{<:Tuple{<:Vector{<:Point}}}) return data_limits(p.plots[1]) end end -boundingbox(p::Voronoiplot{<:Tuple{<:Vector{<:Point}}}, space::Symbol = :data) = transform_bbox(p, data_limits(p)) +boundingbox(p::Voronoiplot{<:Tuple{<:Vector{<:Point}}}, space::Symbol = :data) = apply_transform_and_model(p, data_limits(p)) function plot!(p::Voronoiplot{<:Tuple{<:DelTri.VoronoiTessellation}}) generators_2f = Observable(Point2f[]) diff --git a/src/camera/projection_math.jl b/src/camera/projection_math.jl index d4f235a9b70..8f4fa2c7dc1 100644 --- a/src/camera/projection_math.jl +++ b/src/camera/projection_math.jl @@ -222,6 +222,11 @@ function transformationmatrix(translation, scale, rotation::Quaternion) trans_scale*rotation end +function is_translation_scale_matrix(mat::Mat4{T}) where T + return abs(mat[2, 1]) + abs(mat[3, 1]) + abs(mat[1, 2]) + abs(mat[3, 2]) + + abs(mat[1, 3]) + abs(mat[2, 3]) < sqrt(eps(T)) +end + #Calculate rotation between two vectors function rotation(u::Vec{3, T}, v::Vec{3, T}) where T # It is important that the inputs are of equal length when diff --git a/src/layouting/boundingbox.jl b/src/layouting/boundingbox.jl index 1fbd1a23781..13c6c415e70 100644 --- a/src/layouting/boundingbox.jl +++ b/src/layouting/boundingbox.jl @@ -35,7 +35,7 @@ See also: [`data_limits`](@ref) function boundingbox(plot::AbstractPlot, space::Symbol = :data) # Assume primitive plot if isempty(plot.plots) - return Rect3d(iterate_transformed(plot)) + return apply_transform_and_model(plot, data_limits(plot)) end # Assume combined plot @@ -47,11 +47,6 @@ function boundingbox(plot::AbstractPlot, space::Symbol = :data) return bb_ref[] end -# for convenience -function transform_bbox(scenelike, lims::Rect) - return Rect3d(iterate_transformed(scenelike, point_iterator(lims))) -end - # same as data_limits except using iterate_transformed function boundingbox(plot::MeshScatter, space::Symbol = :data) # TODO: avoid mesh generation here if possible @@ -111,7 +106,7 @@ function boundingbox(plot::Scatter) return bb else - return Rect3d(iterate_transformed(plot)) + return apply_transform_and_model(plot, data_limits(plot)) end end @@ -127,12 +122,3 @@ end function iterate_transformed(plot, points::AbstractArray{<: VecTypes}) return apply_transform_and_model(plot, points) end - -# TODO: Can this be deleted? -function iterate_transformed(plot, points::T) where T - @warn "iterate_transformed with $T" - t = transformation(plot) - model = model_transform(t) # will auto-promote if points if Float64 - trans_func = transform_func(t) - [to_ndim(Point3d, project(model, apply_transform(trans_func, point, space))) for point in points] -end \ No newline at end of file diff --git a/src/layouting/text_boundingbox.jl b/src/layouting/text_boundingbox.jl index a4bc14dd588..7344162c200 100644 --- a/src/layouting/text_boundingbox.jl +++ b/src/layouting/text_boundingbox.jl @@ -9,12 +9,8 @@ function boundingbox(plot::Text, target_space::Symbol) # transformations (i.e. drops textsize etc when markerspace is not part of # the plot.space -> target_space conversion chain) if target_space == :data - if plot.space[] == plot.markerspace[] - # probably shouldn't transform... - return transform_bbox(plot, string_boundingbox(plot)) - else - return Rect3d(iterate_transformed(plot)) - end + # This also transforms string boundingboxes if space == markerspace + return apply_transform_and_model(plot, data_limits(plot)) elseif target_space == plot.markerspace[] return string_boundingbox(plot) else diff --git a/src/layouting/transformation.jl b/src/layouting/transformation.jl index 29a9f5cffad..4afb4401d31 100644 --- a/src/layouting/transformation.jl +++ b/src/layouting/transformation.jl @@ -1,3 +1,11 @@ +#= +`apply_transform` Interface for custom transformations: +- can be a struct holding information about the transformation or a function +- should implement `inverse_transform(transformation)` +- for struct: must implement `apply_transform(transform, ::VecTypes)` (for 2d and 3d points) +- for struct: must implement `apply_transform(transform, ::Rect3d)` for bounding boxes +=# + Base.parent(t::Transformation) = isassigned(t.parent) ? t.parent[] : nothing function parent_transform(x) @@ -183,37 +191,75 @@ transform_func(x) = transform_func_obs(x)[] transform_func_obs(x) = transformation(x).transform_func """ - apply_transform_and_model(plot, pos, output_type = Point3d) - apply_transform_and_model(model, transfrom_func, pos, output_type = Point3d) + apply_transform_and_model(plot, data, output_type = Point3d) + apply_transform_and_model(model, transfrom_func, data, output_type = Point3d) Applies the transform function and model matrix (i.e. transformations from -`translate!`, `rotate!` and `scale!`) to the given input +`translate!`, `rotate!` and `scale!`) to the given input. """ -function apply_transform_and_model(plot::AbstractPlot, pos, output_type = Point3d) +function apply_transform_and_model(plot::AbstractPlot, data, output_type = Point3d) return apply_transform_and_model( - plot.model[], transform_func(plot), pos, + plot.model[], transform_func(plot), data, to_value(get(plot, :space, :data)), output_type ) end -function apply_transform_and_model(model::Mat4, f, pos::VecTypes, space = :data, output_type = Point3d) - transformed = apply_transform(f, pos, space) +function apply_transform_and_model(model::Mat4, f, data, space = :data, output_type = Point3d) + transformed = apply_transform(f, data, space) + world = apply_model(model, transformed, space) + return promote_geom(output_type, world) +end + +function unchecked_apply_model(model::Mat4, transformed::VecTypes) + p4d = to_ndim(Point4d, to_ndim(Point3d, transformed, 0), 1) + p4d = model * p4d + p4d = p4d ./ p4d[4] + return p4d +end +@inline function apply_model(model::Mat4, transformed::AbstractArray, space::Symbol) + if space in (:data, :transformed) + return unchecked_apply_model.((model,), transformed) + else + return transformed + end +end +function apply_model(model::Mat4, transformed::VecTypes, space::Symbol) if space in (:data, :transformed) - p4d = to_ndim(Point4d, to_ndim(Point3d, transformed, 0), 1) - p4d = model * p4d - p4d = p4d ./ p4d[4] - return to_ndim(output_type, p4d, NaN) + return unchecked_apply_model(model, transformed) else - return to_ndim(output_type, transformed, NaN) + return transformed end end -function apply_transform_and_model(model::Mat4, f, positions::AbstractArray, space = :data, output_type = Point3d) - output = similar(positions, output_type) - return map!(output, positions) do pos - apply_transform_and_model(model, f, pos, space, output_type) +function apply_model(model::Mat4, transformed::Rect{N, T}, space::Symbol) where {N, T} + if space in (:data, :transformed) + bb = Rect{N, T}() + if !isfinite_rect(transformed) && is_translation_scale_matrix(model) + # Bbox contains NaN so matmult would make everything NaN, but model + # matrix doesn't actually mix dimensions (just translation + scaling) + scale = to_ndim(Vec{N, T}, Vec3(model[1, 1], model[2, 2], model[3, 3]), 1.0) + trans = to_ndim(Vec{N, T}, Vec3(model[1, 4], model[2, 4], model[3, 4]), 1.0) + mini = scale .* minimum(transformed) .+ trans + maxi = scale .* maximum(transformed) .+ trans + return Rect{N, T}(mini, maxi - mini) + else + for input in corners(transformed) + output = to_ndim(Point{N, T}, unchecked_apply_model(model, input), NaN) + bb = update_boundingbox(bb, output) + end + end + return bb + else + return transformed end end +promote_geom(output_type::Type{<:VecTypes}, x::VecTypes) = to_ndim(output_type, x, NaN) +promote_geom(output_type::Type{<:VecTypes}, x::AbstractArray) = promote_geom.(output_type, x) +function promote_geom(output_type::Type{<:VecTypes}, x::T) where {T <: Rect} + return T(promote_geom(output_type, minimum(x)), promote_geom(output_type, widths(x))) +end + + """ apply_transform(f, data, space) Apply the data transform func to the data if the space matches one @@ -247,7 +293,6 @@ apply_transform(f::NTuple{3, typeof(identity)}, x::VecTypes) = x apply_transform(f::NTuple{3, typeof(identity)}, x::Number) = x apply_transform(f::NTuple{3, typeof(identity)}, x::ClosedInterval) = x - struct PointTrans{N, F} f::F function PointTrans{N}(f::F) where {N, F} @@ -449,6 +494,20 @@ function apply_transform(f::Polar, point::VecTypes{N2, T}) where {N2, T} end end +# For bbox +function apply_transform(trans::Polar, bbox::Rect3d) + if trans.theta_as_x + θmin, rmin, zmin = minimum(bbox) + θmax, rmax, zmax = maximum(bbox) + else + rmin, θmin, zmin = minimum(bbox) + rmax, θmax, zmax = maximum(bbox) + end + bb2d = polaraxis_bbox((rmin, rmax), (θmin, θmax), trans.r0, trans.direction, trans.theta_0) + o = minimum(bb2d); w = widths(bb2d) + return Rect3d(to_ndim(Point3d, o, zmin), to_ndim(Vec3d, w, zmax - zmin)) +end + function inverse_transform(trans::Polar) if trans.theta_as_x return Makie.PointTrans{2}() do point diff --git a/src/makielayout/blocks/polaraxis.jl b/src/makielayout/blocks/polaraxis.jl index 5850fc0bd84..062d3a4e82f 100644 --- a/src/makielayout/blocks/polaraxis.jl +++ b/src/makielayout/blocks/polaraxis.jl @@ -167,7 +167,7 @@ function polaraxis_bbox(rlims, thetalims, r0, dir, theta_0) rmin, rmax = max.(0.0, rlims .- r0) if abs(thetamax - thetamin) > 3pi/2 - return Rect2f(-rmax, -rmax, 2rmax, 2rmax) + return Rect2d(-rmax, -rmax, 2rmax, 2rmax) end @assert thetamin < thetamax # otherwise shift by 2pi I guess @@ -185,7 +185,7 @@ function polaraxis_bbox(rlims, thetalims, r0, dir, theta_0) # Initial bbox from corners p = polar2cartesian(rmin, thetamin) - bb = Rect2f(p, Vec2f(0)) + bb = Rect2d(p, Vec2d(0)) bb = update_boundingbox(bb, polar2cartesian(rmax, thetamin)) bb = update_boundingbox(bb, polar2cartesian(rmin, thetamax)) bb = update_boundingbox(bb, polar2cartesian(rmax, thetamax)) @@ -209,7 +209,7 @@ end function setup_camera_matrices!(po::PolarAxis) # Initialization - usable_fraction = Observable(Vec2f(1.0, 1.0)) + usable_fraction = Observable(Vec2d(1.0, 1.0)) setfield!(po, :target_rlims, Observable{Tuple{Float64, Float64}}((0.0, 10.0))) setfield!(po, :target_thetalims, Observable{Tuple{Float64, Float64}}((0.0, 2pi))) setfield!(po, :target_theta_0, map(identity, po.theta_0)) diff --git a/src/stats/hexbin.jl b/src/stats/hexbin.jl index a8f5ca9f493..49015ac386f 100644 --- a/src/stats/hexbin.jl +++ b/src/stats/hexbin.jl @@ -56,7 +56,7 @@ function data_limits(hb::Hexbin) return Rect3d(no, nw) end -boundingbox(p::Hexbin, space::Symbol = :data) = transform_bbox(p, data_limits(hb)) +boundingbox(p::Hexbin, space::Symbol = :data) = apply_transform_and_model(p, data_limits(p)) get_weight(weights, i) = Float64(weights[i]) get_weight(::StatsBase.UnitWeights, i) = 1e0