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

update(AligningGuidelines): Fix some bugs and add custom features. #10120

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

zhe-he
Copy link
Contributor

@zhe-he zhe-he commented Sep 2, 2024

Resolve the issue where when the user scales using scaleX, scaleY is not updated simultaneously. The same applies to scaleY.

Copy link

codesandbox bot commented Sep 2, 2024

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

@zhe-he
Copy link
Contributor Author

zhe-he commented Sep 2, 2024

2024-09-02.14.23.03.mov

test

@zhe-he zhe-he force-pushed the fix-aligning-guidelines-uniformScaling branch from 5e1acc1 to 326e756 Compare September 10, 2024 09:09
@zhe-he zhe-he force-pushed the fix-aligning-guidelines-uniformScaling branch from 326e756 to 9523d69 Compare September 10, 2024 09:19
@zhe-he
Copy link
Contributor Author

zhe-he commented Sep 10, 2024

When the angle of the operated graphic is not 0, there is a bug with the 4-corner scaling alignment.

I will solve it at noon tomorrow.

@zhe-he
Copy link
Contributor Author

zhe-he commented Sep 11, 2024

fixed

2024-09-11.10.19.10.mov

demo

When the angle of the operated graphic is not 0, there is a bug with the 4-corner scaling alignment.
I will solve it at noon tomorrow.

@zhe-he
Copy link
Contributor Author

zhe-he commented Sep 23, 2024

In practical use, a common issue has been identified: when there are too many shapes, the problem of excessive reference line hits occurs because each shape is compared with the current shape. It is suggested to change the approach to count all points and use those points for judgment. (The same issue occurs in V5; if there are related requirements for V5, please refer to the modifications.)

The following is a video comparison before and after the modifications.

a.mov
b.mov

@asturur
Copy link
Member

asturur commented Sep 23, 2024

I m sorry i didn't have time for this and i won't for a bit.
All the implementation needs to be checked and verified a bit imho.
When there are too many lines there must be a way for the developer to choose the one they care about or expose an interaction key to manually disable them. Is a common problem

@zhe-he
Copy link
Contributor Author

zhe-he commented Sep 24, 2024

Okay, I will take some time to write the test cases, while also adding the corresponding listener functions and disabling the relevant feature switches.

@zhe-he
Copy link
Contributor Author

zhe-he commented Sep 24, 2024

@asturur
I made a lot of updates, and the final result is significantly different from the initial version. You can ignore the logic of the initial version and directly look at all the final code. Compared to the initial version, I added comments to each line, making it easier to understand.
Additionally, I added user customization options, which are documented in the README. Users can manually disable horizontal lines and customize the alignment of graphics. It also supports user-defined names for controllers.
If you have any questions about any line of code, feel free to tag me, and I will explain once I see it.
This is the final version. If you have time, please download the code locally to review all files; it will be faster than reviewing changes on GitHub. Thank you.
Regarding the test cases for simulating user manual dragging, I haven't written any before. If needed, I will try to add them.
Here is a test demo where you can test by modifying parameters or customizing them.
demo

@zhe-he zhe-he changed the title fix(AligningGuidelines): Align guidless changes aspect ratio on snapping when scaling update(AligningGuidelines): Fix some bugs and add custom features. Sep 24, 2024
// tr <-> tl、 bl <-> br、 mb <-> mt、 ml <-> mr
if (target.flipX) corner = corner.replace('l', 'r').replace('r', 'l');
if (target.flipY) corner = corner.replace('t', 'b').replace('b', 't');
const coords = target.getCoords();
Copy link

@Smrtnyk Smrtnyk Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't see properly but is this constant used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When executing the flip, its value becomes true, which is set internally by Fabric. The video below shows the bug that occurs when it is turned off.

2024-11-07.10.01.35.mov

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean that const coords = target.getCoords(); seems unused

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right, it should be deleted.


// You can customize the return graphic, and the example will only compare it with sibling elements
initAligningGuidelines(myCanvas, {
getObjectsByTarget: function (target) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this PR and tried using this function to filter out objects that have properties isGridLine on them
like if (!object.isGridLine) { set.add(object) }
but then no guidelines happened at all
was I using it wrong maybe?

Copy link
Contributor Author

@zhe-he zhe-he Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Smrtnyk
Can you provide a simple demo to reproduce your issue? This will help me locate the problem. Regarding this function, if customized, it means that when moving the target element, it only processes the reference line for the graphical representation of the return value
The input parameter of this function is the current shape being operated on, not a loop representing all shapes.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is what I tried

 initAligningGuidelines(this.canvas, {
         getObjectsByTarget: () => {
             const set = new Set<FabricObject>();
             for (const obj of this.canvas.getObjects()) {
                 if (obj.isGridLine) {
                     continue;
                 }
                 set.add(obj);
             }
             return set;
         },
     });

I have a grid that I toggle, used as a helper when adding shapes
for aligning guidelines I want to ignore all lines in that grid
and I thought I can ignore them if I don't add them to the set that is being returned

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Smrtnyk
I wrote the custom code according to your method and found that it works. You can compare it with the demo to see where the differences are.
custom demo

2024-11-08.10.37.59.mov

@Smrtnyk
Copy link

Smrtnyk commented Nov 7, 2024

I have downloaded locally this PR and use it in the project, it is better than the current version for my needs
Would be good to see progress on this being merged into the fabric

@Smrtnyk
Copy link

Smrtnyk commented Nov 13, 2024

I have been testing this patch for a while now in production in my fabric app.
It works much better than the current one in master tree.
Not sure if it means something, but would be nice to see some progress on this getting merged so I can get rid of my patch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants