Square brush sometimes draws incorrectly

mitchcurtis commented 5 years ago

When drawing with a thick square brush, there are sometimes what seems to be lines drawn which looks wrong:

thick-brush-bug

mitchcurtis commented 5 years ago

@not-surt do you know what would be causing this and how we could fix it?

not-surt commented 5 years ago

You mean the points where it draws a diamond instead of a square?

I figure that happens when the start and end points of the line segment are near enough that line angle cannot be calculated meaningfully. QPainter effectively rotates the pen to follow the path of the stroke. If the cursor move diagonally one pixel then the line with be angled 45 degrees and the pen will be rotated to follow it.

That's why I initially switched to using a round pen. That way pen orientation doesn't matter at all.

Ways to fix it:

use only round pen
store and process the stroke input points to discard any that are too close
switch to more traditional spaced brush dabs so brush doesn't rotate

I'm already working on storing the stroke and redrawing the area of the join so segments connect cleanly. That in itself may alleviate this problem some, but also make a fix easier to implement.

Most image editors don't bother with continuous strokes or rotating brush along stroke that QPainter paths allow, just drawing image-aligned copies of the brush spaced along the stroke.

QPainter paths are cool but impose considerable limitations:

QPainter can't draw to indexed images (indexed editing is very important for pixel art AFAIC)
can't change pen size mid stroke so can't scale brush in response to pressure without doing so for at least a whole line segment
effectively limited to circle or square brushes, can't use custom brushes, possibly could do basic brush transforms (rotation, skew, etc.) but would need to go about it in a rather convoluted way (draw stroke in brush transformed space)

mitchcurtis commented 5 years ago

Awesome, thanks for the info. Sounds like you really know your stuff!

not-surt commented 5 years ago

I've reworked the line drawing here: https://github.com/not-surt/slate/commit/7565b849f1fe00b51e6bcdf065e34dd3a1224cc3

It stores the points of the stroke and redraws the join area so segments connect smoothly.

It's got some very simple hard coded smoothing, just discarding any cursor sample points that aren't further than one pixel away but helps this issue quite a bit.

Just for kicks I also switched blend mode to source-over and allow transparent drawing without accumulation within a stroke so long as the width is greater than one as QPainter draws wide strokes without overdraw, but does overdraw on single pixel wide strokes.

Still hacky and needs more work, so not ready for merging yet.

Due to the way the undo system works it is currently first drawing an unconnected segment then undoing it to merge the undo command and redraw it connected. Will need a bit of fiddling to work around this.

There is a bugs with the occasional missing pixel in centre of stroke. It looks like a QPainter issue, but I haven't done any investigation to make sure I'm not doing something stupid.

mitchcurtis commented 5 years ago

I've reworked the line drawing here: not-surt@7565b84

It stores the points of the stroke and redraws the join area so segments connect smoothly.

It's got some very simple hard coded smoothing, just discarding any cursor sample points that aren't further than one pixel away but helps this issue quite a bit.

Awesome! The square brush drawing is looking a lot better!

Just for kicks I also switched blend mode to source-over and allow transparent drawing without accumulation within a stroke

That's really nice. I can't remember why I went with the current behaviour where it replaces instead of blending.. was probably just easier. It might be good to have a setting to toggle this though, just in case someone wants the old behaviour... would that be easy enough to do with your implementation?

so long as the width is greater than one as QPainter draws wide strokes without overdraw, but does overdraw on single pixel wide strokes.

Are you talking about the "accumulation" of transparent colour? I wonder why QPainter behaves that way. It would be good to have consistency between brush sizes, but I'm sure you already have it in mind.

Still hacky and needs more work, so not ready for merging yet.

Due to the way the undo system works it is currently first drawing an unconnected segment then undoing it to merge the undo command and redraw it connected. Will need a bit of fiddling to work around this.

I'm glad you mentioned the undo system - that was my first thought when I read about the transparency changes because I've had bugs there in the past. :)

There is a bugs with the occasional missing pixel in centre of stroke. It looks like a QPainter issue, but I haven't done any investigation to make sure I'm not doing something stupid.

I noticed that, but it seems like it could probably be hacked around if you don't end up finding the root cause?

I see in the diff that you added a QImage-based brush member... interesting. :D Will Slate get "texture" brushes like Photoshop? :o

By the way, just a heads up if you switch to Creator 4.8 - it seems that there is a bug with Qbs shipped with it that prevents building Slate. I added the workaround to the description of the report:

https://bugreports.qt.io/browse/QBS-1417

not-surt commented 5 years ago

Cleaned up on a new branch (https://github.com/not-surt/slate/tree/pen_tool2) and added a toolbar button to select between blend and replace blend modes.

Still using QPainterPath and got the issues of missing pixels and single pixel line overdraw.

Commented out the crude line smoothing as it was quite arbitrary.

Still intend to switch to image-based brush.

Failing a heap of tests, though don't have much clue why. I'm not a real programmer and have no experience with unit testing.

mitchcurtis commented 5 years ago

Don't worry too much about the tests, I can help with those when you're done. :)

not-surt commented 5 years ago

Got basic image-based brush working. Create a selection then use Edit -> Brush From Selection. With image-based brushes it's no longer naturally no overdraw so alpha blended painting acumulates again.

Added simple stylus pressure scaling (with 5.12 borked on Windows won't work there).

Added option to render brush preview onto canvas (View -> Show Brush Preview), in a similar manner to lines, but using the actual drawing command, then undoing after display. Working in all project types.

Are you targeting any particular OpenGL/ES version? Thinking about a GL-based stroke renderer and ES 3.1 equivalent would be nice for the compute shaders.

mitchcurtis commented 5 years ago

Sounds interesting! Looking forward to trying it out. Not sure if I still have a tablet to try the pressure stuff out with though...

Are you targeting any particular OpenGL/ES version?

No, hadn't even thought about it. :)

Thinking about a GL-based stroke renderer and ES 3.1 equivalent would be nice for the compute shaders.

What would this involve? Or what is it, exactly? :D

I guess it would be good if it worked with Angle, if that's even relevant in this context.

not-surt commented 5 years ago

GPU acceleration for drawing to the image (not just displaying it).

Looks like QPainter draws ellipses wonkily so had to dig out an ellipse scan converter from some old code to draw round brushes correctly. Only used once when first creating brush, so speed of software rendering isn't really an issue, but still feels wrong in this day-and-age. QPainter won't render to indexed images so if going to support indexed editing need to either write ugly, low-level software rendering code or simple shader code.

Looks like ES 3.1 support is still a ways off for ANGLE.

mitchcurtis commented 5 years ago

GPU acceleration for drawing to the image (not just displaying it).

Oh, nice.

Looks like QPainter draws ellipses wonkily so had to dig out an ellipse scan converter from some old code to draw round brushes correctly. Only used once when first creating brush, so speed of software rendering isn't really an issue, but still feels wrong in this day-and-age. QPainter won't render to indexed images so if going to support indexed editing need to either write ugly, low-level software rendering code or simple shader code.

I understand some of those words.. :x

Looks like ES 3.1 support is still a ways off for ANGLE.

The real problem is that I don't know how many users have drivers with poor OpenGL support. We could always just say that Slate requires a graphics card with decent OpenGL support. Depends if it's worth it or not. Do you see a lot of slowness in the current image drawing? Or does it also allow you to add new functionality?

glingy commented 5 years ago

@not-surt I was just recently trying the square brush and noticed that when I move diagonally, the brush still rotates. Does it ever need to be able to rotate, or can it always be straight? That would solve this issue, but I might me missing a use case.

not-surt commented 5 years ago

Try this branch: https://github.com/mitchcurtis/slate/tree/pen-tool-work-2

mitchcurtis / slate

Square brush sometimes draws incorrectly #96