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

Line Retool #682

Merged
merged 5 commits into from
May 17, 2017
Merged

Line Retool #682

merged 5 commits into from
May 17, 2017

Conversation

smiegrin
Copy link
Contributor

@smiegrin smiegrin commented May 9, 2017

This is the last tool (as far as I'm aware) that needed a tune-up after adding larger sizes for drawing tools, and it was a bit of a bugger. Lines still aren't very uniform at varying angles, and the ends of the lines aren't square with how I set this one up. If there's anything that would be useful for the end user that's missing in this one, please don't hesitate to let me know, and I'll crack on it!

@juliandescottes
Copy link
Collaborator

Thanks for the PR! Quickly tested and it's indeed much faster.

As you said there are still a few weird artifacts, and endings should really be square. Reading your comment I'm not sure if it was a deliberate choice, or if you could not find a way to achieve square endings?

@smiegrin
Copy link
Contributor Author

The un-squareness of the endpoints was a deliberate solution to the difficulty I had in finding one that left square endings. In order to improve performance, I changed the method of resizing the line pixels from expanding them as a square (n^2) to expanding them as a cross (2n). This had the side effect, though, of making the lines thinner in body (rather than thicker) when drawn diagonally. As a result, the square caps on the ends became noticeably knobby.

At first, I adjusted the size expansion to be from corner to corner, but this resulted in dithered gaps in-between the footprints of the line, so instead I changed the caps. By making them diamonds, the caps fit the profile of the line when at a diagonal.

Attached are some illustrations of the results I was getting. The top left is the original footprint of the pixels that formed the line, the top right is the new footprint. Below each in the middle row is what each looks like when stacked in a line that moves down one and right one for each step. The bottom left is what a diagonal cross footprint looks like with square ends. Note that the boundaries are correct, but the insides have missing pixels. The bottom right is the cross footprint using square endings. Note that the squares are visible apart from the line drawn.
new piskel 1

I'm afraid I'm not familiar with any artifacts resulting from the current method, though... any illustration/demonstrations of such would be helpful.

Thank you so much for the feedback! I'll see if I can rework the footprint to accommodate square endings.

...also, let me know if this was too much exposition on my thought/work process. I'm not yet familiar with what information is useful and what bits are obvious/useless.

@@ -97,8 +97,35 @@
linePixels = pskl.PixelUtils.getLinePixels(col, this.startCol, row, this.startRow);
}

pskl.PixelUtils.resizePixels(linePixels, penSize).forEach(function (point) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, I exchange the square-style resize for a cross-shape resize in lines 124-129.

@@ -97,8 +97,35 @@
linePixels = pskl.PixelUtils.getLinePixels(col, this.startCol, row, this.startRow);
}

pskl.PixelUtils.resizePixels(linePixels, penSize).forEach(function (point) {
targetFrame.setPixel(point[0], point[1], color);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Below, I implement a (potentially verbose) procedural diamond at each end of the line. It is accomplished by conditionally coloring a square on the basis of the taxicab distance from the center of a square.

@juliandescottes
Copy link
Collaborator

The new version works really well! Thanks a lot for updating this.
I'm embarrassed I didn't check it before doing the v0.11.0 tag, but we can release it next week :)

...also, let me know if this was too much exposition on my thought/work process. I'm not yet familiar with what information is useful and what bits are obvious/useless.

Your explanations were great, keep doing that!

I'm afraid I'm not familiar with any artifacts resulting from the current method, though... any illustration/demonstrations of such would be helpful.

image

Trying your precedent version, when using an even pen size (8px, 12px, 16px etc...) and drawing a line at 45degrees, you can see the "inner" part of the line is visibly 1 pixel thinner than the edges. FYI you can hold shift while using the line pen to stick to precise orientations.

The new version is not affected by a similar issue, so all good!

Copy link
Collaborator

@juliandescottes juliandescottes left a comment

Choose a reason for hiding this comment

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

That's a great fix, thanks @smiegrin !

If you want to add some of your comments in the source directly, feel free to do so. Otherwise I can merge as is :)

@smiegrin
Copy link
Contributor Author

Oooh, comments seem like a good idea. Thanks for the feedback! I'll have those in just a few minutes.

@juliandescottes
Copy link
Collaborator

Merging!

Thanks again for doing this. The implementation is really clever and it's a pleasure to read :)

@juliandescottes juliandescottes merged commit cd56001 into piskelapp:master May 17, 2017
@juliandescottes juliandescottes mentioned this pull request Jun 21, 2017
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