Skip to content

Paint rectangle drawing#2294

Merged
ghaerr merged 4 commits intoghaerr:masterfrom
Vutshi:master
Apr 12, 2025
Merged

Paint rectangle drawing#2294
ghaerr merged 4 commits intoghaerr:masterfrom
Vutshi:master

Conversation

@Vutshi
Copy link
Copy Markdown
Contributor

@Vutshi Vutshi commented Apr 11, 2025

  • Implemented rectangle drawing feature.
  • Refactored code to improve scalability and maintainability.
  • Added UI feedback to highlight the active drawing mode button.
Screenshot 2025-04-11 at 22 14 04

@ghaerr
Copy link
Copy Markdown
Owner

ghaerr commented Apr 12, 2025

Wow, super nice work. Thank you @Vutshi!

@ghaerr ghaerr merged commit 9b78149 into ghaerr:master Apr 12, 2025
@ghaerr
Copy link
Copy Markdown
Owner

ghaerr commented Apr 12, 2025

@Vutshi, if you look very closely when dragging the rectangle to a desired size, the corners of the rectangle are missing (but get painted when the mouse button is released). The reason for this is that you are drawing the each of the endpoints twice with the vertical lines, so the double XOR causes the corners to go back to the background color. This can be fixed by shortening the two vertical lines drawn by two pixels each, when the rectangle height > 3.

@Vutshi
Copy link
Copy Markdown
Contributor Author

Vutshi commented Apr 12, 2025

@ghaerr,
When this enum is compiled, is it stored as a 16-bit or 8-bit value?

typedef enum {
    mode_Brush,
    mode_Fill,
    mode_Circle,
    mode_Rectangle
} DrawingMode;

@Vutshi
Copy link
Copy Markdown
Contributor Author

Vutshi commented Apr 12, 2025

This can be fixed by shortening the two vertical lines drawn by two pixels each, when the rectangle height > 3.

I'm not sure it's worth adding an extra if statement to fix such a minor issue—especially since it almost feels like a feature, subtly indicating that the shape isn't finalized yet.

@ghaerr
Copy link
Copy Markdown
Owner

ghaerr commented Apr 12, 2025

When this enum is compiled, is it stored as a 16-bit or 8-bit value?

I believe by default they are stored as 'int', which is 16-bits here. I think there's an option to size enum's at 8-bits. The reason int is default is that usually allows for the simplest and fastest code to be generated: in some instances but not always the compiler will have to generate a CBW (convert byte to word) instruction after grabbing a byte before comparing or passing the value as a function argument.

@Vutshi
Copy link
Copy Markdown
Contributor Author

Vutshi commented Apr 12, 2025

I believe by default they are stored as 'int', which is 16-bits here.

so our Boolean variables are also 16-bit int?)

@ghaerr
Copy link
Copy Markdown
Owner

ghaerr commented Apr 12, 2025

so our Boolean variables are also 16-bit int?)

Yes. Are you thinking about the memory used? In general, none on disk unless initialized non-zero, and the resulting use when running very small. The code generated can differ though, and should be looked at via objdump to learn more exactly.

That said, the GCC compiler does support a "native" boolean using #include <stdbool.h>. These variables, I think declared using _Bool, are 8-bit variables and treated by the compiler code generator as a special case. Thus the code generated for them might be different than 8-bit enums. However, I have pushed back from including stdbool.h in any ELKS programs because of the compatibility problems with other compilers and the resulting dependency on another "standard" header file.

What is your concern about 8- vs 16-bits for enums and bools?

@Vutshi
Copy link
Copy Markdown
Contributor Author

Vutshi commented Apr 12, 2025

What is your concern about 8- vs 16-bits for enums and bools?

I’m just learning. When I started thinking how to store mode and state of drawing I came across these enums which didn’t say explicitly what they are. Then I found that Paint bools are also enums. Finally, if I understand correctly 8088 has 8-bit memory bus so it reads 1 byte at a time which doesn’t look good for a simple bool taking 2 cycles to read.

@Vutshi
Copy link
Copy Markdown
Contributor Author

Vutshi commented Apr 12, 2025

What I’m really concerned about is the structure of the paint program’s drawing logic. I suspect some of the conditions are redundant, but it’s hard for me to grasp the overall flow well enough to verify its correctness and efficiency. I feel like I need a Rust-like system — but for logic safety instead of memory safety :)

@ghaerr
Copy link
Copy Markdown
Owner

ghaerr commented Apr 12, 2025

if I understand correctly 8088 has 8-bit memory bus so it reads 1 byte at a time which doesn’t look good for a simple bool taking 2 cycles to read.

You make a very good point there - I hadn't added in the extra cycle for memory access on 8088 vs every other x86 chip out there. However, in general, this only matters in tight loops - trying to "optimize" in non-critical areas where only 10 or 20 lines of non-looping C code won't add anything to delays experienced by the user, and in some cases just generates extra code, which has to be carried around in the disk file.

Nonetheless, for boolean types, I usually use 'char' for no other reason that the program can clearly see it's not an int and is not usually used for arithmetic calculations/values. And in most cases, I don't think the compiler generates extra code (CBW, etc) since the value will only compared to zero or non-zero.

For floppy systems, every extra 1K read can be noticeable. One optimization I try to do is that, when the program is nearing completion (not now for Paint), I look at the executable size and if its 1-100 bytes over a 1K boundary, try to see what can be done to eliminate the over over a 1K boundary.

What I’m really concerned about is the structure of the paint program’s drawing logic.

I wouldn't worry too much about that - the program is still quite simple, and the recent modifications you've made are easy to follow. The overall efficiency in Paint's case will always be judged by its drawing speed while moving the mouse, I think.

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