Fix the line draw choppy on fast mouse movements issue

[Viet281101]

Hi,
I had the idea of using the Bresenham algorithm to calculate the points between two mouse positions, we can ensure that the drawing line with pencil or delete with eraser will not be interrupted when moving the mouse too fast. And I successfully implemented them after dividing the EventHandlers functions from script.js into eventHandlers.js file for easier reading and editing. I hope this helps improve this project, thanks for your patience.

Fixes the following issue(s)

• Fixes Line is drawn choppy on fast mouse movements #8

[Kully]

Hi,
I had the idea of using the Bresenham algorithm to calculate the points between two mouse positions, we can ensure that the drawing line with pencil or delete with eraser will not be interrupted when moving the mouse too fast. And I successfully implemented them after dividing the EventHandlers functions from script.js into eventHandlers.js file for easier reading and editing. I hope this helps improve this project, thanks for your patience.

Woah, this looks awesome! 🎉 Nice work @Viet281101 !!

I'm going to give this a spin when I have some time on my hands. I have a very busy week but will aim to look at this PR by the end of the week.

Also good call on implementing for the erase tool as well; it makes sense to apply to that tool as well.

@Viet281101 I'm curious, did you consider other algorithms besides Bresenham? What made you go with that? Just curious :)

[Viet281101]

In fact, the results above are only experimental results with linear interpolation, not with Bresenham. I am trying to modify them to be successful with Bresenham, linear interpolation is simpler and easier to implement but in terms of performance it is not as good as bresenham because the way it connects points in the line is by dividing the distance between two points into multiple steps and plot points along those steps. So with the current edit, there are still some errors in connecting drawing lines when hovering quickly.
The reason I want to choose Bresenham is because it is more optimal in the job of calculating the points that need to be connected on the drawing line, and it is very accurate in drawing straight lines that are not sloping or slightly sloping.
And because this Bresenham algorithm exercise is the first thing I learned from the graphics algorithm course at Paris 8 university, so I am quite familiar with it to use in this case, although not successful yet. I tested it many times and found it works very well in some of my other OpenGL projects.

[Kully]

there are still some errors in connecting drawing lines when hovering quickly.

What do you mean by hovering? Do you mean just in drawing a line quickly?

The reason I want to choose Bresenham is because it is more optimal in the job of calculating the points that need to be connected on the drawing line, and it is very accurate in drawing straight lines that are not sloping or slightly sloping.

Hmm I checked out your Pull Request and tested it but it looked pretty good to me.

There was one unexpected behaviour that I noticed. When you start a line, move your cursor off the screen then come back, there was a line interpolated from the exit point and to the entry point.

error.with.interpolation.mov

We would want to make sure that once you leave the canvas, we stop the interpolation and pick it up again when you re-enter, so there is no vertical line. Makes sense?

[Viet281101]

Interpolation issue when move the cursor off the canvas and return has been resolved. However, it creates a small problem that comes with solving it, which is that when we move the cursor too quickly from inside the canvas outward, the line still breaks near the border of the canvas. I tried to fix it by using bresenham again to connect the canvas boundary point where the mouse pointer last appeared with the following code:

canvasDiv.addEventListener("mouseleave", function (e) {
	if (STATE["brushDown"]) {
		const canvasRect = canvasDiv.getBoundingClientRect();
		const x = Math.max(0, Math.min(e.clientX - canvasRect.left, canvasRect.width - 1));
		const y = Math.max(0, Math.min(e.clientY - canvasRect.top, canvasRect.height - 1));
		const targetCell = document.elementFromPoint(x + canvasRect.left, y + canvasRect.top);

		if (previousCursorX !== null && previousCursorY !== null && targetCell && targetCell.classList.contains('canvasCell')) {
			const targetX = targetCell.offsetLeft / CELL_WIDTH_PX;
			const targetY = targetCell.offsetTop / CELL_WIDTH_PX;

			Bresenham_Line_Algorithm(previousCursorX, previousCursorY, targetX, targetY, function (intermediateCell) {
				if (STATE["activeTool"] === "eraser") {
					intermediateCell.style.backgroundColor = CANVAS_INIT_COLOR;
				} else if (STATE["activeTool"] === "pencil") {
					intermediateCell.style.backgroundColor = STATE[ACTIVE_COLOR_SELECT];
				}
			});
		}
	}
	isDrawingOutside = true;
});

But unfortunately, in most cases, retesting does not show good results. Please tell me if you have any suggestions

[Viet281101]

@Kully
So do you want to add or edit anything else to this Pull Request? Syntax style, or reposition functions or move them into ES6 classes for easier maintenance?
Because I think the rest is all done after I fixed the problem of broken lines when moving the mouse quickly.
Anyway, I'm very happy to be working on this project. This project really has a lot of features that can be added in the future such as zoom function, animation layer, transparent background, sharing results online, or testing connecting the server with socket.io for many people to draw on various devices, etc

[Kully]

Just gave you a round of review.

I looked at the Bresenham_Line_Algorithm implementation not in detail and was not able to figure out why it draws the line choppy when you come into the canvas. If I have time tomorrow, I can have a look again. If you want to continue seeing if you can fix that would be great.

Regarding some of the syntactic changes you made (tabs into spaces), if it's easy enough to revert these changes I would prefer this :) I am open to doing this across the repo eventually, but it would be cleaner if we did in a separate PR.

[Kully]

So do you want to add or edit anything else to this Pull Request? Syntax style, or reposition functions or move them into ES6 classes for easier maintenance?

Just gave you a review.

Anyway, I'm very happy to be working on this project. This project really has a lot of features that can be added in the future such as zoom function, animation layer, transparent background, sharing results online, or testing connecting the server with socket.io for many people to draw on various devices, etc

This is open-source at its best! 😍

And cool ideas. Zoom, animation, transparent backgrounds, and socket.io all sound really interesting. If you are willing, I'm happy for you to open an issue for each of these feature requests.

(PS. I am working on game independently right now and am locally refactoring the code to support custom colors ... I will probably push this to a PR at some point)

[Viet281101]

Hi @Kully, is there any other syntax style that you want to change in this Pull Request ?

[Kully]

No nothing else syntax related. If you’ve retverted the tabs/spaces commits, then no.

…

On Sun, May 19, 2024 at 1:35 PM Viet Nguyen ***@***.***> wrote: Hi @Kully <https://github.com/Kully>, is there any other syntax style that you want to change in this Pull Request ? — Reply to this email directly, view it on GitHub <#20 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACPDQR2WMCLG4FKVFSVNNPTZDBW7HAVCNFSM6AAAAABHWYKIICVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJZGE3TAMBQGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[Viet281101]

So, can I close this Pull Request to start the new one?
I already have an idea for transparent background to draw and save.

[Kully]

So, can I close this Pull Request to start the new one?
I already have an idea for transparent background to draw and save.

@Viet281101 Wait, were we going to merge this PR? I'm confused why you closed it?

Did you manage to fix the bug with your Bresenham implementation?

[Viet281101]

Sorry, it's my first time doing a Pull Request so I don't know what the final process is, and I've already fixed the error applying bresenham before.

[Kully]

Sorry, it's my first time doing a Pull Request so I don't know what the final process is, and I've already fixed the error applying bresenham before.

All good. So usually the flow I'm used to is that the PR author would ping the reviewer to say that they have addressed the review, the reviewer (me) would take a look, then I would either:

• approve the changes
• request more changes / discuss changes

I will now check out the branch and test it out 👍

[Kully]

@Viet281101 Okay so I tried out the app and I am still getting the issue with the algo. Here's a video.

Did you say you solved this one or was there another one you solved?

issue.with.algo.mov

[Viet281101]

What I mean is that when you quickly move the mouse out of the canvas the line breaks before reaching the border of the canvas, this problem I have solved, but if you want to talk about the problem is after you move the mouse out and rotate returning to the canvas in another location at a fast speed will have a break at the beginning of the entry point, which I haven't fixed yet because I'm not sure if it's a bug or not.
Because it can be a normal phenomenon when you move your cursor quickly into the canvas after leave it and the canvas detects your cursor slower than its actual position.
If you don't want that little break when you quickly hover back to the canvas then maybe I can also use the same method with when you quickly hover outside the canvas which was fixed before.

[Kully]

Because it can be a normal phenomenon when you move your cursor quickly into the canvas after leave it and the canvas detects your cursor slower than its actual position.

Hmm I see what you are saying (I've experienced this kind of behaviour in other apps before) but it still feels like a bug to me. It's still within reason to expect that if you are holding down the mouse and dragging in, that it should continue the line - especially if you are drawing a line and darting in and out of the canvas.

If you don't want that little break when you quickly hover back to the canvas then I can also use the same method with when you quickly hover outside the canvas which was fixed before.

Yes this would be great! 🙂

If you can do this, then the PR should be ready to merge.
PS There is also another comment I made that I don't think you have responded to. Did you see it? It's here: https://github.com/Kully/pixel-paint/pull/20/files#r1606034897

[Viet281101]

I fixed it according to your request, and it actually required more math than I thought, I had to use a ''cheat'' to position the cursor movement vector with mousemove event and the rest was using two mouse coordinates inside and outside of the canvas to calculate the coordinates of entryPoint and finally connect entryPoint to the starting point of the line appearing in the canvas as target using Bresenham. But anyway, the problem has been resolved and it doesn't seem to have created any new problems.
PS: I don't really have much experience in using eventListeners in JS like mousemove so it's not too clear what the performance optimization is in this case except adding removeEventListener at the end like i did.

[Viet281101]

Hi @Kully , Does this make it ready to merge?
Please let me know if there is anything else that needs fixing, This week I will probably be very busy with my final project due to graduating soon.

[Kully]

Does this make it ready to merge?
Please let me know if there is anything else that needs fixing, This week I will probably be very busy with my final project due to graduating soon.

Hi @Viet281101 I just gave it a try. Thanks for fixing the the issue as we discussed. However, I did notice another inconsistency in behaviour.

Look at what happens when you draw like this:

new.error.mov

Could you fix this issue?

[Viet281101]

Well, sorry for not readjusting the filter for entryPoint values ​​so that they were sometimes greater than 31 when they should have been 0 or 31 in all cases, but now it has been resolved.

[Kully]

Well, sorry for not readjusting the filter for entryPoint values ​​so that they were sometimes greater than 31 when they should have been 0 or 31 in all cases, but now it has been resolved.

It works, amazing! Okay we are good to go now. Thank you so so much for the contribution to the project. I will find time to eventually read over the algo and understand your code, but from a user perspective it works very well.