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

Fix decimate deleting end of a freedrawing line #6966

Merged
merged 3 commits into from
May 16, 2021
+16 −4
Merged

Fix decimate deleting end of a freedrawing line #6966

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
62355e8
Fix decimate deleting end of a freedrawing line
BinaryMoon Mar 30, 2021
778c9f7
Update pencil_brush.class.js
BinaryMoon Mar 31, 2021
b89ce10
Add tests for brush.decimate
BinaryMoon May 9, 2021
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
10 changes: 6 additions & 4 deletions src/brushes/pencil_brush.class.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,16 +246,18 @@
var zoom = this.canvas.getZoom(), adjustedDistance = Math.pow(distance / zoom, 2),
i, l = points.length - 1, lastPoint = points[0], newPoints = [lastPoint],
cDistance;
for (i = 1; i < l; i++) {
for (i = 1; i < l - 1; i++) {
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 went a slightly different way to your suggestion and changed the loop to ignore the last point, and then always add it at the end.

With this method the line will always be at least 2 points long, because newPoint has the first point added when it's initialized, and the last point added after the decimation. The decimation then only happens to the points in the middle of the line.

This removes the need to check the line length, and reduces the amount of checks made when decimating the line.

Admittedly very small improvements but I like it :)

I have tested this with single clicks and long lines.

If you're not happy with this I'd be happy to use your suggestion. I don't mind what the solution is as long as it works. :)

Copy link
Member

Choose a reason for hiding this comment

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

this looks good to me.
Way better than my try.
Now the last step. Can we write a test that will fail if this commit is removed?

Let me see if i can find a semi ready test

Copy link
Member

Choose a reason for hiding this comment

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

no we haven't, so i started one

        QUnit.test('decimate points', function(assert) {
          var brush = new fabric.PencilBrush(canvas);
          var points = [{ x: 1, y: 0 }, { x: 2, y: 0 }, { x: 3, y: 0 }, { x: 4, y: 0 }, { x: 5, y: 0 }];
          var distance = 6;
          var newPoints = brush.decimatePoints(points, distance);
          assert.equal(newPoints[0], points[0], 'first point is always present');
          // add more conditions... or more tests.
        });

this test would go at the end of last test in this file:

test/unit/brushes.js

and provided you npm installed fabric locally, you can execute the tests runing:

npm run test:single test/unit/brushes.js

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 was able to run the test. That's great. Thanks!

I've never done any JavaScript testing before.

Copy link
Member

Choose a reason for hiding this comment

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

Please add a test so we merge the PR.
What is important to cover in the test is your change.
So give a list of points in which the last point is distat from the previous once less than distance, it should be still available in the output.

Any PR to any public repo should generally, as a rule of thumb, ask you for tests.
Is too much easy to forget features and delete previous fixes with changes otherwise.
And our coverage is not even perfect.

cDistance = Math.pow(lastPoint.x - points[i].x, 2) + Math.pow(lastPoint.y - points[i].y, 2);
if (cDistance >= adjustedDistance) {
lastPoint = points[i];
newPoints.push(lastPoint);
}
}
if (newPoints.length === 1) {
newPoints.push(new fabric.Point(newPoints[0].x, newPoints[0].y));
}
/**
Copy link
Member

Choose a reason for hiding this comment

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

or couldn't we just check in the loop

if (cDistance >= adjustedDistance || i === l - 1) {

?

Copy link
Member

Choose a reason for hiding this comment

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

Because with your solution we risk to duplicate the last point in case is already there? or am i reading something wrong?

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 is also an option. I liked the simplicity of my solution, which actually removed code, but I can see that it may sometimes duplicate the last point. I shall have another think and try some other options.

* Add the last point from the original line to the end of the array.
* This ensures decimate doesn't delete the last point on the line, and ensures the line is > 1 point.
*/
newPoints.push(points[l]);
return newPoints;
},

Expand Down
10 changes: 10 additions & 0 deletions test/unit/brushes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,16 @@
assert.deepEqual(brush._points, [], 'points is an empty array');
});

QUnit.test('decimate points', function(assert) {
var brush = new fabric.PencilBrush(canvas);
var points = [{ x: 1, y: 0 }, { x: 2, y: 0 }, { x: 3, y: 0 }, { x: 4, y: 0 }, { x: 5, y: 0 }];
var distance = 6;
var newPoints = brush.decimatePoints(points, distance);
assert.equal(newPoints[0], points[0], 'first point is always present');
assert.equal(newPoints[1], points[points.length-1], 'last point is always present');
assert.equal(newPoints.length, 2, 'All points removed except first and last');
});

[true, false].forEach(function(val) {
QUnit.module('fabric.BaseBrush with canvas.enableRetinaScaling = ' + val, function(hooks) {
hooks.beforeEach(function() {
Expand Down