Fix decimate deleting end of a freedrawing line

[BinaryMoon]

Stop decimate from deleting the last point on the line, whilst ensuring the line is always > 1 point in length (so that it actually displays)

Fixes this ticket: #6951

[asturur]

this could work, but can you check what happens for single click points with drawing brush? We should still get a circle.

[asturur]

or couldn't we just check in the loop

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

?

[asturur]

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

[BinaryMoon]

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.

[BinaryMoon]

this could work, but can you check what happens for single click points with drawing brush? We should still get a circle.

Yes - a single click still works. Previously (the code I removed) added the last point again. Since there was only one point point[0] is the last point. Essentially all this code does is remove the check for line length and always add the last point to the end. In my tests it worked well on a variety of drawing speeds, just clicking, and different decimate amounts.

[BinaryMoon]

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. :)

[asturur]

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

[asturur]

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

[BinaryMoon]

I was able to run the test. That's great. Thanks!

I've never done any JavaScript testing before.

[asturur]

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.

[asturur]

@BinaryMoon can you add a test for this change? so we can merge the pr

[stale]

This issue 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.

[BinaryMoon]

Hi there - Sorry for the very delayed reply. Work and real life got in the way.

I'd really like to work on this and add a test but I have no idea what I'm doing. I've never written a unit test and I don't know how to generate the data.

Sorry :(

[asturur]

No worries @BinaryMoon this is a free open project and people dedicate the time they can.
I'll guide you to write a test, is very important for open software to have tests, and i want your contrbution to come with one.
When is ready, is ready.

[BinaryMoon]

Hi - thanks for your understanding! :)

I had some time to go through this today and it was easier than I thought it would be. I've added the test you mentioned and added a couple of additional tests.

Hopefully that works ok.

[BinaryMoon]

Also - do you have any coding standards to follow? The formatting standards I use for work are very different to what I can see here so it makes it harder to edit things without reformatting the entire file. I've been working out the changes locally then applying the changes on Github directly - but that's not the niceset workflow :)

[asturur]

Hi @BinaryMoon sorry for not answering, i moved fabricJS emails out of my inbox and i m still getting adjusted to it.
In the repo you find a lint command and a lint command for tests.
If you run npm run lint or npm run lint_tests you get the check for the only code standards that are catched as regressions.
Other than that we still use ES5, so that is the other coding standard you have to follow.
We still keep ie11 working with no transpiler ( i think this is the last version that does it )

[asturur]

In theory your IDE should pick up the eslint file from the project and adjust itslef to respect the rules.
What ide do you use?

[BinaryMoon]

Thanks for merging this! :)

I have been using Atom to edit this. When I saved the file(s) it reformatted the same thing. I don't know if that's because of an extension or something else but I suspect that's not what you want.

I'll see if I can work out how to get the eslint file to be recognised.