-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Redo coords #8767
Closed
Closed
Redo coords #8767
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
refactor coords refactor coords Update matrix.ts Update InteractiveObject.ts fixes options
This reverts commit dd7c1b9.
it didnt tests the order!
rm `transformPointRelativeToCanvas`, add `sendVectorToPlane`
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs 😔. Thank you for your contributions. |
7 tasks
This was referenced Aug 24, 2023
Closed
This was referenced Sep 7, 2023
This was referenced Sep 24, 2023
Closed
Ported to ShaMan123#4 |
@ShaMan123 @asturur Is there any reason why this PR wasn't merged and was ported to a fork instead? This piece of work is rad. |
Disagreements |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
coords have been longing for a rewrite.
Ever since I rewrote Group I knew that but was afraid of the daunting task.
aCoords, lineCoords and oCoords are badly named and badly calculated, don't really fit Group, or edge cases like
strokeUniform
.Description
The goal is to provide a single set of coords that represent the bounding box and a set of transformations that output matching bounding boxes. This will be done using change of basis.
In addition, we must take into account the fact that because of the infamous
strokeUniform
option we must do bbox calculation in the viewport. Once we do that we can transform the bbox anywhere, however the opposite is incorrect.strokeUniform
is not linear.As part of this effort, I will try to impl @asturur's suggestion: no need for generic width/height option for all shapes.
Previously, control positioning didn't respect flipping. We discussed this and decided to leave as is for now, leaving this as a possibility to be handled by
Control#positionHandler
.I want to change the signature ofpositionHandler
so it accepts a set of transformations that will make it easy to position controls in different circumstances (e.g. the default rotation matrix, a bbox that isn't rotated, a transformed bbox, a bbox respecting flipping etc.).If we must not break the Control API I will add a calling method that will handle legacy behavior.
positionHandler
can use any BBox to position controls.POC has uncovered a lot of bugs in control handlers.
I hope to find a way to complete the first iteration without touching that.Another issue I have noticed is the conditional usage of
qrDecompose
(e.g.getTotalAngle
in case ofgroup
). That should be considered a bug from now on since it will produce major inconsistencies in transformations.qrDecompose
is the baseline for transformations. Under this scope, flipping and rotation might be a mess. I need to look into that a bit more.Another thought in this field which is drastic.... Transform options is causing us a lot of headache. Fact is HTMLCanvas2DContext has nothing similar, rather, it exposes methods such as
rotate
,skew
,scale
,preMultiply
etc. IMO so should we. And move all calculation to use the raw transform matrix.Can it be done? Should it? I have no idea and won't be trying it for the near future but I think the change of basis approach is the solution for transform options in general.EDITED: I have exposed
ObjectTransformations
(qrDecomposed
can be avoid in layout/bbox calculations, see next paragraph).For the same reasons mentioned above control action handlers must receive a vpt pointer and bbox infos
Considering performance and
setCoords
:Only objects with
strokeUniform
orpadding
must be calculated in the viewport. The rest can be transformed/sent to the viewport while keeping accuracy.Coords are used to draw controls and to detect pointer hits. If we are handling an object that must be calculated in the viewport we have no other option and must calculate coords once the viewport changes. However, if the object respects the viewport we can save on calculations by skipping
setCoords
in this case and sending the pointer to the relative object plane. The same applies to nested objects under groups.There are several bugs/issues this PR aims to fix.
absolute, calculate
Changes
Object origin has been REMOVED in favor of BBox. I did this because object origin respected only the containing plane so it was worthless. However the concept is brilliant so I adapted it into BBox, allowing us the work with origins on any plane.
BBox is basically a transform matrix that represents a plane. It accepts an origin point and transforms it into the point in the plane, making it possible to transform the context and work with origin etc. The motivation of BBox is double: we must compute coords and geometry in the viewport because of things like strokeUniform and padding and we don't want to continuously calculate and figure out hard non intuitive linear algebra stuff.
ObjectGeometry was split into:
setCoords
set
is not valid for the same reason ObjectOrigin isn't (and as a bonus it starts moving away from qrDecompose). Exposedscale
,skew
,rotate
,scaleBy
,skewBy
,rotateBy
,translate
and more. It will replace some control logic.I want to drop decomposition in favor of holding the own matrix and exposing setters and getters for the transition or forever. As I see it this is the reason the canvas API doesn't decompose, it exposes a lot of friction. But things like limiting scale etc will probably need decomposition anyways.
This PR should be considered part of the Group rewrite. I hacked controls by sending the pointer to the containing plane but it is wrong to do that (strokeUniform, vpt for example are not accounted for properly). I have decided to make controls work in the viewport because by definition they interact with the event which exists in the viewport but it is a design call. We need to discuss it. Because with BBox we could accurately send the points to the containing plane and set apply changes to object transformation in the vpt.
BBox will use stroke projection after we agree on how to move forward
Misc
fromObject
that consumes v5 data and outputs v6 data, since the version is present as part of the dataI am not touching tests yet because I don't want to waste time. First we need to decide and discuss.
#8767 (review)
Gist
Change of basis
In Action
fabric.js.sandbox.-.Google.Chrome.2023-03-17.07-46-00_Trim.mp4
This is a case that is broken on master:
https://codesandbox.io/s/fabric-vanillajs-sandbox-forked-3rykyj?file=/package.json
3D rotation
I have accidently managed to make the skewing control a 3d rotation 🤯😆
fabric.js.sandbox.-.Google.Chrome.2023-03-17.08-28-21_Trim.mp4
activeObject.rotate3D(1, -1, 1, true)
fabric.js.sandbox.-.Google.Chrome.2023-03-17.10-10-22_Trim.mp4
3D controls
fabric.js.sandbox.-.Google.Chrome.2023-03-17.12-29-47_Trim.mp4