Conversation
Merged
Contributor
I didn’t say the new circles look nice, I said they are the raster circles closest to the ideal one :) There are no better circles centered on a pixel. I’ll make an illustration later. The previous circles were not, like e.g. 2x2 block of pixels. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Fixes error in IA16 vga_drawhline found in #2278 (comment).
The problem was a single ASM line with reversed arguments (you have to thank Intel vs AT&T for that)!!
@Vutshi, I had to remove the mouse clipping (when drawing) you added into event.c, as when selecting various colors one after another in the control panel, after a bit of doing just that the entire screen would get filled with the color selected. It seems that this is the same problem you were trying to fix when moving the cursor off the canvas when drawing, when using the new R_DrawDisk function.
I think this is a new bug that has to do with R_DrawDisk, as I'd never seen it before, but not sure.
In any case, we don't want to/can't keep the mouse clipping code in event.c anyways, as it contains app.h and application globals (drawing, altduawing). The event.c code is meant to be a library routine, so I'd like to ask that you clip the mouse coordinates and fix the problem in input.c during mouse down processing, instead of in the shared app main event code. I haven't figured out exactly why the problem is, but just commented it out for the time being. With it commented out, the buttons never fail but you can get in trouble when mouse down drawing going off the canvas.
On another note, I am closely comparing the circle code and there are definitely differences. TBH, I'm not sure the new code looks better - the 4th pixel size in particular was very round in the before version, and now seems to have points on it and kind of looks like a diamond (see screenshots below, you may want download them and quickly switch between them with mac Preview "spacebar" in order to really check them out). I can see that the single pixel circle is now correct whereas in the old version it was a bit larger. Nonetheless, the new code draws a circle that is a little bit larger (you need to quickly switch between them to see it).
The new circle looks are fine with me, I just wanted to bring this up, in case you handn't already noticed it. I did notice that the new draw code is much faster, undoubtedly from drawing using drawhline, is that the main benefit of this rewrite?
I had already written the above when I realized that there must be a strict way that circles have to be rasterized if they are to easily use drawhline to draw them, rather than the very slow original drawpixel method.
Before

After

Thank you for your enhancements!