-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Grouping] Should group positions be always calculated from leaf element positions? #27444
Comments
Pinging @elastic/kibana-canvas |
This comment enlists the operations and their special cases to help us decide if omitting group positions is beneficial or even feasible. Please add a comment with the advantages of not storing group positions, or alternatives in the representation I might have overlooked. It got long but I'll talk about the layout engine on Jan 10 and would raise pain points and contentious issues to loop in everyone's insight, there's plenty of room for improvement across the board. tl;dr to me it looks necessary, or at least now practical to persist group positions, besides of course the amount of work involved in eliminating them :-) Original calculation of group positionIt is true that group Grouping with axis aligned bounding box (AABB)The current grouping merely wraps the parts in an AABB which can be calculated from the leftmost, rightmost, topmost etc. vertices of the parts. Group rotationThe initially axis aligned group can then be rotated, with the result that the global position of parts will be similarly impacted ( Rotated partsParts can be in arbitrary orientations: 0 degree, multiples of 90 degrees, and anything in general. Some groups will have parts with diverse orientations (eg. orienting objects around a circle), and some groups will have parts with uniform, but non-zero angled orientations (eg. shapes rotated 45 degrees). Group position reconstruction if parts are not rotated and the group isn't rotated eitherIn this case, we can reestablish the group by simply forming the AABB around the parts. It'll work even if the group was repositioned or resized since its formation. Future: grouping with oriented bounding box (OBB)Currently, the sequence of
will yield a distinct group, which is axis aligned (relative to global ie. canvas projection) and whose parts are slanted relative to it. Group position reconstruction if some parts and/or the group are rotatedIn this case, the position of the parts does not provide sufficient information to reconstruct the position of the group, see the gif on top (parts end up unrotated but the group is rotated). Even if no parts are rotated, the group could've been formed when they were rotated, and then the entire group was rotated so the net effect is restoration of 0 degree parts, but the group isn't AABB. It's a special case but users may create arbitrary arrangements. Future: other affine transformsWe currently expose translation and rotation only (the resize is something else). The plan is to eventually have: scaling (we have working code but not included now), flip/mirror, shear, 3D transforms and maybe even perspective projection. Even now we can't reconstruct the group, and with these, a reconstructed group will be farther and farther away from the user intent. Why do we care about how the group originated?Watching the gif, does it matter to the user if we omit some aspects of grouping info and we always use eg. an AABB? These are supports for preserving grouping info:
(*) While Google Sheets does preserve position info with(persistent) groups, their transient groups actually do this constant re-AABB. It feels unfinished, and the elements can't even be snap-oriented back to a sensible angle eg. 0 or 45 after the first rotation. OK assuming that preserving group position is important, is there another way?>We could, for example, retain and retrace the steps that lead to the current constellation, but it may not be scaleable. To limit the O(n) complexity, sometimes the line would have to be drawn, ie. at that point the state would still need to reflect the cumulative effects of what impacted the position (in short, retain the position). Yet it's an interesting if tangential discussion on its own, because retaining last N user ops would have other benefits:
What about integer pixel level accuracy?Originally the layout transforms simply used floating points. A report from @rashidkpc showed that on some screens, font rendering is inferior if an element is not snapped to an integral pixel. So now we round. In the future it could/should get more intricate: once there's even a little bit of rotation (except maybe multiples of 90deg) is done, whether we snap to pixel or not won't matter. Ie. we'd snap to pixel only for axis aligned elements (with the generally working assumption that texts are axis aligned within the element). In summary, the question is more about how we define grouping (is a group an object that has orientation etc. as in comparable software, or is it merely a set of its leaf elements, like maybe layers or tags in other software, and a visual bounding box is an incidental embellishment?), than about representing it in Redux, because sticking with the common meaning of grouping, it feels necessary to retain group position info. |
@monfera thanks for the description, and the images. After watching the first gif, reading what you wrote, and going back to that first gif, I think I get it. If I follow this correctly, the bounding box is sized relative to the elements in the group, and that's the box that's being stored in state. If we don't save that information, we can't re-create the container if it's modified (as demonstrated by the second gif, repeated below here). tl;dr While that value is calculated, it's not calculated every time, so we do need to store it.
I'd have to agree. @clintandrewhall you care to weigh in here? |
Closing this discussion item, as the observation initiated a discussion but didn't lead to a change request. It can be reopened if there's more to add. |
Kibana version: 6.6+
Describe the bug:
As mentioned in #25854, the group position is a calculated value and should not be part of the redux state.
Screenshots (if relevant):
The text was updated successfully, but these errors were encountered: