fix large square brush

[gschweden]

fixes #163

Hey, I hope I'm not stepping on your toes here. No worries if you have a better fix in mind for this (re your comments on the issue). I learned a lot about your code just fiddling with this so it won't feel like wasted effort if the fix doesn't work for you.

I will squash the commits before merging but I thought I'd keep them for the review as the progression kinda shows where I'm coming from with this fix.

Problem

The larges (8pxl) square brush appears hollow when quickly moving horizontally. Other brush shapes and sizes are not affected. Eraser is not affected.

screen shot


Cause

The bitmap array can not differentiate between 0-1,2 and 7,1. So is identifies the left edge of the square as being adjacent to the right edge of the square.

visualization because this hurt my brain haha


So the conditional in the function for finding the circumference (outline) of the brush shape can not be met for values at the left and right edges of the shape.

https://github.com/1j01/jspaint/blob/f7fea8b12d7900d4aaffab3a5875849cb499dbf6/src/image-manipulation.js#L164-L169

screen shot of square brush circumference rendered without brush stamp


This leaves the observable gap in between the filled shape stamps at either end of the line segment.

Solution

If the canvas is 1 bigger than the brush shape get_circumference_points_for_brush can always find the edge of the shape on the canvas.

The best place for increasing the canvas shape seemed to be the function responsible for calculating canvas size based on shape. This increases the canvass shape on both the x and y axis, even though only x would be necessary. I think the minor performance impact is worth the separation of concerns in the code. (Alternative would be to increase the canvas size in the get_circumference_points_for_brush function.)

Conveniently this also fixes the issue with rendering odd sized circles. I also tested with a custom circle of size 8 and it had the same gap issue as the size 8 square. Handling both sqares and circles the same seems reasonable.

Interrupted circle outline


QA

Tested all brush shapes and sizes at different speeds and directions. There no longer is a gap.

Performance and Testing

My local set up could be better. I don't have cypress set up yet but will figure that out if the change is desirable and tests are needed. Performance seems to be same as before change, but my set up is pretty slow so it's hard to tell.

[1j01]

Nice investigation, and nice graphics! I didn't know the circumference points calculation was failing. I think a simpler solution would be to add bounds checking to the at function.

[1j01]

I've fixed this by changing the at function. Thanks for your work on this though!

    const at = (x, y) => (
        // coordinate checking is important so it doesn't wrap (if the brush abuts the edge of the canvas)
        x >= 0 && y >= 0 &&
        x < image_data.width && y < image_data.height &&
        image_data.data[(y * image_data.width + x) * 4 + 3] > 127
    );