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

perPixelTargetFind not working in nested group. #5287

Merged
merged 12 commits into from
Oct 13, 2018

Conversation

doouding
Copy link
Contributor

@doouding doouding commented Oct 1, 2018

perPixelTargetFind property is not working with object in nested group. When I replace isTargetTransparent methods params with pointer relative to canvas, everything works fine.

@asturur
Copy link
Member

asturur commented Oct 1, 2018

So we would search target with the normalized pointer for bounding box, but the per pixel target would work with the non normalized one.

That would make sense. I wonder if a nicer fix is possible. And tests, we need tests to ensure it works.

@doouding
Copy link
Contributor Author

doouding commented Oct 2, 2018

Would you please indicate which part of code can be improved. Thanks.

I'll add some test cases soon.

@@ -1196,13 +1199,14 @@
/**
* @private
*/
_checkTarget: function(pointer, obj) {
_checkTarget: function(pointer, obj, globalPointer) {
globalPointer = globalPointer || pointer;
Copy link
Member

Choose a reason for hiding this comment

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

is this check necessary? it looks like that we always pass in globalPointer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if someone might call this private method directly.
Should I remove this check or just document this method?

Copy link
Member

Choose a reason for hiding this comment

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

Document the method is enough, is a private method.
If someone call it directly is gonna have problems anyway since those methods get changed with no information to devs.
We can put this check in searchPossibleTargets that while being private too, is at least one level up of this.

what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That make sense to me.

@@ -1216,18 +1220,18 @@
/**
* @private
*/
_searchPossibleTargets: function(objects, pointer) {
Copy link
Member

Choose a reason for hiding this comment

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

even if those 2 methods are private, we could still document them

Function used to search inside objects an object that contains pointer in bounding box or that contains pointerOnCanvas when painted
@param [fabric.Object] objects array to look into
...

can you complete the JSDOC information for _searchPossibleTargets and checkTarget ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

@asturur
Copy link
Member

asturur commented Oct 2, 2018

If address the comments looks good.
Let me know how it goes for testing.

subTargetCheck: true
});

canvas.add(group2);
Copy link
Member

Choose a reason for hiding this comment

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

While this test is ok, the generated group is too simple.
can we use something like this:

var circle = new fabric.Circle({ radius: 30, top: 30, left: 30, fill: 'yellow'});

var circle2 = new fabric.Ellipse({ rx: 10, ry: 20, top: 40, left: 0, fill: 'blue'});

var circle3 = new fabric.Circle({ scaleX: 2, scaleY: 2, radius: 10, top: 120, left: -20, fill: 'red'});

var rect = new fabric.Rect({ width: 100, height: 80, top: 50, left: 60, fill: 'purple' });

var group = new fabric.Group([circle, circle2], { scaleX: 3, angle: 45, opacity: 0.5 });

var group2 = new fabric.Group([group, rect, circle3], { opacity: 0.5, top: 20, left: 20, scaleX: 0.8, scaleY: 1.2 });

canvas.add(group2);

that looks like this:

image

and test edge cases? we should be able to succesfully target all the shapes.

@doouding
Copy link
Contributor Author

doouding commented Oct 3, 2018

It seems that object in nested group should normalize the pointer just once or we will get wrong result when calling containsPoint.

Please check over the test cases I just added :).

assert.equal(canvas.targets.length, 2, 'Subtargets length should be 2');
assert.equal(canvas.targets[0], circle2, 'The deepest target should be circle2');
canvas.perPixelTargetFind = false;
canvas.remove(triangle);
Copy link
Member

Choose a reason for hiding this comment

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

this is group3, not triangle

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about that. I will fix it soon.

@asturur
Copy link
Member

asturur commented Oct 8, 2018

yes i think t his is because normalizePointer will normalize everything up to the canvas level, so everytime you normalize starting from absolute.

Those changes are less invasive, i need some time to download and test the branch, but i think we are good. We still avoided angle and skew in the test case, i do not understand why

@doouding
Copy link
Contributor Author

doouding commented Oct 8, 2018

Please let me know if there's any problem.

@asturur
Copy link
Member

asturur commented Oct 13, 2018

Hey, thank you.
This is a nice pr, a nice fix and thanks for the flexibility in rewriting the test.

@asturur asturur merged commit 839b54a into fabricjs:master Oct 13, 2018
@asturur asturur mentioned this pull request Oct 14, 2018
thiagocunha pushed a commit to thiagocunha/fabric.js that referenced this pull request Nov 18, 2019
* fix perPixelTargetFind in nested object

* fix: lint

* add methods doc

* doc: update _serachPossibleTarget doc

* test: add test case

* fix: remove pointer check

* fix: object in nested group should normalize just once

* test: add test case

* restore to previous code style

* fix: update test case descriptions

* fix: test case

* test: add skew and angle test case
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.

2 participants