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

fix #101 - diagonal line #102

Merged
merged 1 commit into from
May 19, 2024
Merged

fix #101 - diagonal line #102

merged 1 commit into from
May 19, 2024

Conversation

nichoth
Copy link
Contributor

@nichoth nichoth commented May 18, 2024

I don't know the details of how the algorithm works, but this fixes the issue.

@jakubfiala
Copy link
Owner

@nichoth thanks for catching this one, too! It didn't occur to me to try double clicking :P i'll merge this now

@jakubfiala jakubfiala merged commit c9ab1b5 into jakubfiala:main May 19, 2024
@jakubfiala
Copy link
Owner

@nichoth Just to add some context, it turns out the culprit with both issues you found, was that I originally covered for cases where previous X/Y is 0 in pointerMove but didn't do so in pointerUp.

Looking at your fix, I realised that correcting for when prevX/Y == 0 should be done inside the draw method itself. This does have the unfortunate side effect, that drawing a line from exactly (0,0) to any arbitrary (x,y) is not possible. However, in practice I think it's unlikely to impact users, because it's usually very hard to click exactly on (0,0), and even if the user does, only the first line segment will be missed. I think that's an acceptable tradeoff.

Another option would be to set previous X/Y to null when the stroke ends, rather than 0. I remember thinking about this some months ago and there was a reason I didn't end up doing it, if only I could remember :D

In any case, thank you for flagging up these issues. Atrament v4.4.0 is now published with your fix, I hope you enjoy using the library :)

@nichoth
Copy link
Contributor Author

nichoth commented May 19, 2024

Cool thank you. I noticed that also, that starting at 0,0 was impossible, but thought it would basically never happen in practice, so an ok solution.

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