Skip to content
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
wants to merge 220 commits into from
Closed

Redo coords #8767

wants to merge 220 commits into from

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Mar 7, 2023

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 of positionHandler 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 of group). 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 or padding 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.

  • flipping + controls (and under group)
  • stroke uniform and padding bbox
  • isOnScreen under group
  • removing geometry signatures absolute, calculate
  • proofing geometry for 100% of cases
  • make coords respect the viewport (skewing, rotation)
  • complex transforms under group (https://codesandbox.io/s/fabric-vanillajs-sandbox-forked-3rykyj)

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:

    • ObjectLayout - purely layout logic, not related to bbox/coords. There was a lot of confusion about this in the code. stroke/padding do not belong here. This is the breaking change of the PR. Positioning is not affected by stroke any longer. Think of strokeUniform - how can layout know the size of stroke beforehand? And it is IMO the expected behavior. If I change stroke I don't expect my object to move from its center.
    • ObjectBBox - bbox/coords logic, strokeUniform and padding go here, the bbox is created in setCoords
    • ObjectPosition - exposing getters/setters for x,y instead of ObjectOrigin (sme methods might be redundant)
    • ObjectTransformation - new class mimicking canvas transform API for objects because using set is not valid for the same reason ObjectOrigin isn't (and as a bonus it starts moving away from qrDecompose). Exposed scale, skew, rotate, scaleBy, skewBy, rotateBy, translate and more. It will replace some control logic.
    • ObjectGeometry - pure geometry methods, all use bbox now, so most of them are aliases
  • 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

    • This PR has made me aware that rendering doesn't respect strokeUniform as well. Skewing an object or the vpt will distort the stroke and shouldn't. The stroke should be rendered in a post effect hook IMO
    • We can expose a util or option in fromObject that consumes v5 data and outputs v6 data, since the version is present as part of the data

I 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

@ShaMan123 ShaMan123 mentioned this pull request May 8, 2023
9 tasks
@stale
Copy link

stale bot commented Jun 18, 2023

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.

@ShaMan123
Copy link
Contributor Author

Ported to ShaMan123#4
Contributions are welcome

@ShaMan123 ShaMan123 closed this May 7, 2024
@ShaMan123 ShaMan123 deleted the redo-coords branch May 7, 2024 17:50
@asturur asturur restored the redo-coords branch May 7, 2024 19:32
@nounder
Copy link

nounder commented Sep 5, 2024

@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.

@ShaMan123
Copy link
Contributor Author

Disagreements
I left fabric

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment