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.

Sign up for GitHub

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
Merged

perPixelTargetFind not working in nested group. #5287

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
34afb37
fix perPixelTargetFind in nested object
doouding Oct 1, 2018
02cfb33
fix: lint
doouding Oct 1, 2018
5985429
add methods doc
doouding Oct 2, 2018
940a9f2
doc: update _serachPossibleTarget doc
doouding Oct 2, 2018
b0b0eb0
test: add test case
doouding Oct 2, 2018
f453fd3
fix: remove pointer check
doouding Oct 3, 2018
cc81ea5
fix: object in nested group should normalize just once
doouding Oct 3, 2018
9865efd
test: add test case
doouding Oct 3, 2018
4c92474
restore to previous code style
doouding Oct 8, 2018
b7b8a26
fix: update test case descriptions
doouding Oct 8, 2018
1d22391
fix: test case
doouding Oct 8, 2018
9bb43a5
test: add skew and angle test case
doouding Oct 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 22 additions & 8 deletions src/canvas.class.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1158,6 +1158,7 @@

var ignoreZoom = true,
pointer = this.getPointer(e, ignoreZoom),
globalPointer = pointer,
activeObject = this._activeObject,
aObjects = this.getActiveObjects(),
activeTarget, activeTargetSubs;
Expand All @@ -1167,15 +1168,17 @@
// if active group just exits.
this.targets = [];

if (aObjects.length > 1 && !skipGroup && activeObject === this._searchPossibleTargets([activeObject], pointer)) {
if (aObjects.length > 1 &&
!skipGroup &&
activeObject === this._searchPossibleTargets([activeObject],pointer, globalPointer)) {
return activeObject;
}
// if we hit the corner of an activeObject, let's return that.
if (aObjects.length === 1 && activeObject._findTargetCorner(pointer)) {
return activeObject;
}
if (aObjects.length === 1 &&
activeObject === this._searchPossibleTargets([activeObject], pointer)) {
activeObject === this._searchPossibleTargets([activeObject], pointer, globalPointer)) {
if (!this.preserveObjectStacking) {
return activeObject;
}
Expand All @@ -1185,7 +1188,7 @@
this.targets = [];
}
}
var target = this._searchPossibleTargets(this._objects, pointer);
var target = this._searchPossibleTargets(this._objects, pointer, globalPointer);
if (e[this.altSelectionKey] && target && activeTarget && target !== activeTarget) {
target = activeTarget;
this.targets = activeTargetSubs;
Expand All @@ -1194,15 +1197,21 @@
},

/**
* Checks point is inside the object.
* @param {Object} [pointer] x,y object of point coordinates we want to check.
* @param {fabric.Object} obj Object to test against
* @param {Object} [globalPointer] x,y object of point coordinates relative to canvas used to search per pixel target.
* @return {Boolean} true if point is contained within an area of given object
* @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.

if (obj &&
obj.visible &&
obj.evented &&
this.containsPoint(null, obj, pointer)){
if ((this.perPixelTargetFind || obj.perPixelTargetFind) && !obj.isEditing) {
var isTransparent = this.isTargetTransparent(obj, pointer.x, pointer.y);
var isTransparent = this.isTargetTransparent(obj, globalPointer.x, globalPointer.y);
if (!isTransparent) {
return true;
}
Expand All @@ -1214,20 +1223,25 @@
},

/**
* Function used to search inside objects an object that contains pointer in bounding box or that contains pointerOnCanvas when painted
* @param {Array} [objects] objects array to look into
* @param {Object} [pointer] x,y object of point coordinates we want to check.
* @param {Object} [pointerOnCanvas] x,y object of point coordinates relative to canvas used to search per pixel target.
* @return {fabric.Object} object that contains pointer
* @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

_searchPossibleTargets: function(objects, pointer, pointerOnCanvas) {

// Cache all targets where their bounding box contains point.
var target, i = objects.length, normalizedPointer, subTarget;
// Do not check for currently grouped objects, since we check the parent group itself.
// until we call this function specifically to search inside the activeGroup
while (i--) {
if (this._checkTarget(pointer, objects[i])) {
if (this._checkTarget(pointer, objects[i], pointerOnCanvas)) {
target = objects[i];
if (target.subTargetCheck && target instanceof fabric.Group) {
normalizedPointer = this._normalizePointer(target, pointer);
subTarget = this._searchPossibleTargets(target._objects, normalizedPointer);
subTarget = this._searchPossibleTargets(target._objects, normalizedPointer, pointerOnCanvas);
subTarget && this.targets.push(subTarget);
}
break;
Expand Down
30 changes: 30 additions & 0 deletions test/unit/canvas.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -790,6 +790,36 @@
canvas.remove(triangle);
});

QUnit.test('findTarget with perPixelTargetFind in nested group', function(assert) {
assert.ok(typeof canvas.findTarget === 'function');
var triangle = makeTriangle({ left: 0, top: 0 }),
target,
group1 = new fabric.Group([triangle], {
subTargetCheck: true
}),
group2 = new fabric.Group([group1], {
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.

target = canvas.findTarget({
clientX: 5, clientY: 5
});
assert.equal(target, group2, 'Should return the triangle by bounding box');
canvas.perPixelTargetFind = true;
target = canvas.findTarget({
clientX: 5, clientY: 5
});
assert.equal(target, null, 'Should return null because of transparency checks');
target = canvas.findTarget({
clientX: 15, clientY: 15
});
assert.equal(target, group2, 'Should return the group2 now');
assert.equal(canvas.targets[0], group1, 'First subTargets should be group1');
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.

});

QUnit.test('findTarget on activegroup', function(assert) {
var rect1 = makeRect({ left: 0, top: 0 }), target;
var rect2 = makeRect({ left: 20, top: 20 });
Expand Down