Skip to content

Paint add flood fill button#2283

Merged
ghaerr merged 2 commits intoghaerr:masterfrom
Vutshi:master
Apr 6, 2025
Merged

Paint add flood fill button#2283
ghaerr merged 2 commits intoghaerr:masterfrom
Vutshi:master

Conversation

@Vutshi
Copy link
Copy Markdown
Contributor

@Vutshi Vutshi commented Apr 6, 2025

  • Add flood fill button
  • rename SCREEN_WIDTH to CANVAS_WIDTH
  • automated buttons generation code

TODO: add indication of brush/fill either in icons (XOR it?) or in cursor

paint_fill_new

@ghaerr
Copy link
Copy Markdown
Owner

ghaerr commented Apr 6, 2025

Wow, you're going to town on this! Nice, thank you! :)

@ghaerr ghaerr merged commit 753bf6a into ghaerr:master Apr 6, 2025
Comment thread elkscmd/gui/app.c
int indices[] = {10, 11, 12, 13, 14};
char* names[] = {"SaveButton", "FillButton", "BrushButton", "QuitButton", "Cls"};
int offsetX[] = {0, 32, 0, 32 * 3, 32 * 3};
int offsetY[] = {0, -32, -32, 0, -32};
Copy link
Copy Markdown
Owner

@ghaerr ghaerr Apr 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you aware how expensive this is, to declare auto initialized array variables? The initializations have to be saved in the data segment at compile time, but then at runtime copied via an internal call to memcpy to get them on the stack; IMO lots of extra code for nothing.

I recommend adding 'static' to the declarations which achieves the same result with lots less code. Same for the variables on lines 81-85.

(You can use ia16-elf-objdump -D -r -Mi8086 app.oaj to see code generated).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, no I was not aware of this. I was just looking for a way to simplify new buttons addition but I couldn’t achieve it with macros only.
I will rewrite it on next PR

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the new coding and ease of inspection of the initializers for each case, I don't think any rewrite is necessary except just applying static prefix to the declarations of the arrays (only).

@ghaerr
Copy link
Copy Markdown
Owner

ghaerr commented Apr 6, 2025

add indication of brush/fill either in icons (XOR it?) or in cursor

I still kind of like the idea of a bucket cursor, (and not sure if we need two sizes of it, one would likely do), but I can add XOR drawing if you'd like to play with it. XOR drawing will be handled by the VGA hardware using set_op(0x18) before the draw code and then set_op(0) afterwards. There's a version of that macro in drawxcanline.c for ia16.c. I need to know just a little more to know how to best implement it.

Are you thinking of indicating this via XOR drawing of the brush or bucket icon? That might result in funny colors, given that the VGA will do a physical XOR of the actual color number, rather than say, reversing colors. Another idea would be drawing a white line around the icon. I still think the simplest and most obvious for users would be changing the cursor when the fill mode is pending a mouse down.

@Vutshi
Copy link
Copy Markdown
Contributor Author

Vutshi commented Apr 6, 2025

That might result in funny colors, given that the VGA will do a physical XOR of the actual color number, rather than say, reversing colors.

Yeah, maybe for icons it is not optimal. I was also thinking how to implement a circle (not disk) drawing. I’d like to have circle updating itself on the fly while user changes its radius. The cheapest way is probably using XOR.

@ghaerr
Copy link
Copy Markdown
Owner

ghaerr commented Apr 6, 2025

I'll put together a test drawpixel_xor() for you to play with for now.

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