boundingbox cleanup

[ffreyer]

In short I think we should

1. Clarify what data_limits and boundingbox does
2. Include the extent of, for example, a scattered marker in boundingbox
3. Add a space option in boundingbox
4. Add per-point boundingboxes and other intermediate stages

---

Currently we have both data_limits and boundingbox which for most plots only differ by an application of the model matrix

Makie.jl/src/layouting/boundingbox.jl

Lines 1 to 8 in bc13edb

	function parent_transform(x)
	p = parent(transformation(x))
	isnothing(p) ? Mat4f(I) : p.model[]
	end
	function boundingbox(x, exclude = (p)-> false)
	return parent_transform(x) * data_limits(x, exclude)
	end

which often times is just an identity transform. The exception to this is text which returns a pixel space boundingbox for the scattered string(s). text also includes the string boundingbox depending on markerspace in data_limits

Makie.jl/src/layouting/data_limits.jl

Lines 43 to 53 in 84a1cdf

	function point_iterator(text::Text{<: Tuple{<: Union{GlyphCollection, AbstractVector{GlyphCollection}}}})
	if is_data_space(text.markerspace[])
	return decompose(Point, boundingbox(text))
	else
	if text.position[] isa VecTypes
	return [to_ndim(Point3f, text.position[], 0.0)]
	else
	return convert_arguments(PointBased(), text.position[])[1]
	end
	end
	end

which goes against my understanding of what data_limits is supposed to be.

I think data_limits should only care about the positional arguments passed to a plot, as it does in most cases now. boundingbox on the other hand should be a tight boundingbox around the full plot, including markersize, linewidth, etc. Including those will require some back and forth transformation, e.g. space -> markerspace, add markersize, markerspace -> space for scatter. (1, 2)

In general it would be good to be able to specify the output space of a boundingbox. That way we could unify text with the other bounding boxes, and have some methods that skip the final transformation. (3)

Making different stages of the boundingbox computation available would also be useful to skip some of steps which might be unnecessary in some use cases. The pipeline here might be: (4)

1. Per position rawboundingboxes(plot) without the position argument applied (probably always in markerspace, as positions may influence the result in other spaces)
2. Per position boundingboxes(plot, space = plot.markerspace) with positions applied (transform positions from space -> markerspace here, user defined output space)
3. The combined boundingbox(plot, space = plot.space) which merges all the individual bounding boxes from the previous step

These steps don't make sense for every (primitive) plot, but I think it's quite obvious when a plot has them and when it doesn't. Though it might be a bit more difficult to generalize this to recipe. Maybe we could just use boundingboxes on the child plots that implement them, or simply make that function only work on the primitive types.

[jkrumbiegel]

I think it makes sense to do a boundingbox including markers / text glyphs in screen space, and optionally offer to transform that back into 2d data space if desired. The thing is that it will not always be possible to even transform to the data space again, when nonlinear transformations are involved.

[ffreyer]

Oh yea, the boundingbox(plot, index) I mentioned in #2800 would also be a good to avoid having to calculate all bounding boxes when you just care about one or a few elements.

[SimonDanisch]

We should also cache boundingboxes, since they're somewhat expensive to calculate, and it's quite natural to query for them across different plotting calls/ functions...

[asinghvi17]

That sounds good. Maybe each atomic (or plot object) could cache its own boundingbox? Or maybe just know if it's outdated or not (based on changes in input data and transformation matrix)

[ffreyer]

#3002 made me trip over this:

Makie.jl/src/layouting/data_limits.jl

Lines 140 to 142 in a26e6a1

	# TODO: without this, axes with log scales error. Why?
	trans_func = identity # transform_func(t)
	# trans_func = identity

We should fix this too.

I also think it would be good to finish up #2766 before this and differentiate what would be world space and model/local space in opengl. I.e the space post trans_funcand model, and the space before any transformations. That would give you a clean way to specify which transformations should be applied.

[asinghvi17]

It seems like the transform is being applied twice somewhere, but I can't figure out where. If we can resolve that, then bounding boxes would probably be a lot easier to manage..

[ffreyer]

Another thing to consider here is how to handle different input spaces with children. Let's say we have

parent with undefined space
- child1 with space = :pixel
- child2 with space = :data

If we call boundingbox(parent, space = :data) should that exclude child1? Should it convert? Both have their use (exclude for limits, convert for e.g. text bounding boxes), so I think both should be available.

We could add a convert = (spaces...) kwarg for this, which contains all the spaces which should be converted into the target space. Probably with convert = :all as a shorthand.

[ffreyer]

boundingbox methods for Scene, Camera, camera controllers, Axis, etc would probably also be useful. They'd give you the size of the viewable area, derived from limits, (lookat, eyeposition, up), or the projectionview matrix. We already have

Makie.jl/src/basic_recipes/hvlines.jl

Lines 41 to 46 in bd315b4

	function projview_to_2d_limits(pv)
	xmin, xmax = minmax((((-1, 1) .- pv[1, 4]) ./ pv[1, 1])...)
	ymin, ymax = minmax((((-1, 1) .- pv[2, 4]) ./ pv[2, 2])...)
	origin = Vec2f(xmin, ymin)
	return Rect2f(origin, Vec2f(xmax, ymax) - origin)
	end

which effectively does this, and I have some code somewhere for 3D cases.

[ffreyer]

Fixing #2245 should be part of this

[ffreyer]

#3112 too

[ffreyer]

From #3176 / #3179:

The default pipeline for data limits of recipes is currently data_limits > iterate_transformed > point_iterator where point_iterator then recursively combines input arguments of child plots. This means that you have to implement a custom point_iterator instead of data_limits if you want to tweak a recipes reported data_limits. Otherwise wrapping that recipe in another recipe will return the wrong limits.

TODO: Move the recursion to data_limits. Should reduce allocations as well.

A similar problem appears with h/vlines and h/vspan. In this case we want to ignore one dimension which currently happens via autolimits attributes. If these recipes are part of another recipe these attributes get ignored by Axis, and limits become unreasonable. Maybe we could handle this through data_limits instead, making the dimension in question NaN?

[jkrumbiegel]

Yes let's continue the discussion here rather than in that other issue. I thought a Union{Nothing, approach might be nicer for a plot to communicate its raw limits, as then the merging could be sure that this is intended and not a computing artifact of something going wrong. I get that we need NaN as "nothing" for something like Cairo line gaps, but in normal Julia code I don't like it too much.

[ffreyer]

I always shy away from unions because of performance but it probably doesn't matter much here. The majority of time is going to be spend generating limits, not evaluating the output.

Something like Union{Nothing, Rect} wouldn't be enough here, because sometimes you only want to invalidate one dimension. Rect{Union{Nothing, Float32} seems fishy to me since it probably breaks a bunch of functions that calculate things, like maximum. So maybe we should make out own type instead?

I could also see some use in having the "invalid" limits around. For vlines and the like we really have one part data space limts and one part relative space limits (at least in theory) which could both be useful to know. So maybe we should just flag them by space with some kind of unknown/invalid/special space for when they don't fit in any of the normal boxes.

[jkrumbiegel]

I thought Tuple{Union{Nothing,Tuple{Float32,Float32}},Union{Nothing,Tuple{Float32,Float32}}} and then at the end make a rect out of it.

[jkrumbiegel]

And for the special spaces, I would think we'd make special bbox request functions?