-
Notifications
You must be signed in to change notification settings - Fork 65
Conversation
Ok I've thought a bit more about this, and I think we should change the Let's think about the different scenarios that a user might want. Multiple strings at multiple positionsThis needs an iterable of strings for the text part, and an iterable of positions with one position for each string. This is super common for labeling multiple data points in a plot with words. The positioning of the glyphs is done by the text layouting algorithm, and the glyphs can be placed / scaled either in Exampletext!(scene, strings, positions)
# or alternatively, to be able to change the number of words
text!(scene, zip(strings, positions)) One string at one positionThis is how Exampletext!(scene, string, position) One string at multiple positions (per char)I think this is quite the rare use case because the user has to compute sensible glyph positions, which is complicated. I can only imagine one might use this for text along a curve or something like that. Right now this is what powers annotations, but as I wrote above, I think this is not needed. Glyph positioning should be mostly left to the default layout algorithm. Exampletext!(scene, string, glyph_positions) SummaryWhat I would suggest:
|
src/layouting/data_limits.jl
Outdated
if x.space[] == :data | ||
return boundingbox(x) | ||
elseif x.space[] == :screen | ||
if x.position[] isa Union{StaticArrays.StaticArray, Tuple{Real, Real}, GeometryBasics.Point} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tuple{Real, Real}
should we make this NTuple{2, Real}, NTuple{3, Real}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no it's actually annoying if things don't work if you give a position = (1, 2.3)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, makes sense. We should probably generalize to the 3D case as well (Tuple{Real, Real, Real}
).
196df9d
to
a23b89a
Compare
src/layouting/data_limits.jl
Outdated
@@ -121,7 +121,7 @@ function atomic_limits(x::Text{<: Tuple{Arg1}}) where Arg1 | |||
end | |||
else | |||
if x.position[] isa Union{StaticArrays.StaticArray, Tuple{Real, Real}, GeometryBasics.Point} | |||
bb = FRect3D(x.position[]) | |||
bb = frect3d(x.position[]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should try to not to introduce these kind of secondary, abstractplotting specific APIs. I know I'm guilty of this a lot... And I learned, that it's very annoying to clean up later and explain to other contributors.
If there's something wrong with FRect3D we should fix it in GeometryBasics - or introduce a proper boundingbox function ;) But not just introduce a new function, with almost the same name, which (probably?) behaves slightly different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I think it's not very clear what the API is for these objects and I can often not find what I need quickly enough. So I write these helper functions instead of stopping to make a PR somewhere else.
What I also find super annoying is that multiplication of rectangles, addition with points, and practical things like that are either not defined or have such strong type restrictions that I can't use them without doing a lot of guard rail code.
Plus, that I can't do array_of_points .+ point
because a point behaves as a small array. That's super impractical..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we should fix that: https://github.com/JuliaGeometry/GeometryBasics.jl/blob/master/src/rectangles.jl#L271
The types look pretty unrestricted?
Plus, that I can't do array_of_points .+ point because a point behaves as a small array. That's super impractical..
Yes, totally agree... I think I'm ready to have that fixed as well^^
49650b5
to
8f97c1d
Compare
8f97c1d
to
57019a9
Compare
57019a9
to
1d17e53
Compare
annotations wouldn't work with this because right now they make a big vector of glyph positions. that needs to be refactored
- text saves glyph info in _glyphlayout attribute - boundingboxes rely on that info - text can use an array of strings and thereby basically makes annotations obsolete
This adds the capability to draw text in screen space instead of data space. For that to work, the backends need to read the
space = :data or :screen
attribute and draw offset the glyphs from the text position either in screen or in data space.That means: text position is always in data space, but how the glyphs are drawn relative to that point depends on the keyword. In most plots, people will want to use
space = :screen
because that leaves the text undistorted by axis projections.Right now, the
annotations
recipe doesn't work withspace = :screen
because it concatenates all its text parts into one big vector, and layouts them all in data space already. This means the backend couldn't separate text positions and glyph offsets anymore. This needs to be changed so that the text parts ofannotations
don't have glyph offsets baked in.There also needs to be made a decision how
boundingbox
anddata_limits
should react to the two spaces. The behavior can stay how it is forspace = :data
, but forspace = :screen
it needs to be different.It makes sense that the data limits of a text or annotations is just the boundingbox enclosing all specifed data positions. In the case of normal text, that's just one point, in the case of normal annotations, it's as many points as there are text parts.
For boundingbox it depends on how one interprets what this function does, whether it's always screen relative or not.