Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Checkbox Appearance #2 #733

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

RobLoach
Copy link
Contributor

@RobLoach RobLoach commented Nov 17, 2024

Follow up on #681 .

@awschult002 @riri I'm unsure if this fixes all the issues we had considered, but it may be onto something. Touches some of the base style settings, which may be what we were looking for.

SDL Renderer

Screenshot from 2024-11-17 18-29-28

RawFB X11

Screenshot from 2024-11-17 18-30-11

Note the rectangle misalignment on x11 raw

RobertLemmens and others added 15 commits June 17, 2022 13:03
Verified that toggle boxes and buttons look good for all SDL, X11, and
GLFW demos. Also respects different styles, which is the biggest change
with the recent merge with master.
The new checkbox toggle look was off by a couple of pixels. This commit fixes that by removing the "line thickness" operation performed on its width before drawing.
The new checkbox toggle look was off by a couple of pixels. This commit fixes that by removing the "line thickness" operation performed on its width before drawing.
the rounded corners on the checkboxes looked terrible with the X11 and rawfb demos. This was also in-congruent with the previous look. Reverting to sharp corners keeps the previous look while also removing the issue of the X11 looking terrible. 

This commit also shrinks the select toggle box inside the checkbox by a couple pixels to better match the radio button's look and feel
@riri
Copy link
Contributor

riri commented Nov 18, 2024

@awschult002 @riri I'm unsure if this fixes all the issues we had considered, but it may be onto something. Touches some of the base style settings, which may be what we were looking for.

Agreed, that was the goal of my comment: it's not perfect but leading to improvements for a nice change with checkboxes and options.

I admit I get lost with all the PRs stacking for this feature.
I let you unroll the mess with opened PRs and issues once it's merged lol

@RobLoach
Copy link
Contributor Author

Yeah, it's tricky because I think we're so close to getting it right across the renderers, but there's still something wrong, and I'm not 100% sure what the original cause is. Apologies for the PR spam.

@RobLoach RobLoach marked this pull request as draft November 18, 2024 17:28
@awschult002
Copy link
Contributor

i think we should make an issue that RAWFB demos have strange checkbox, link that here for tracking; then merge the PR.

That will clean up the PR mess that i started; but keep some tracking around of the problem so that we can keep working it. I like the idea of letting the issue pages balloon, while keeping the PR list small and clean

@riri
Copy link
Contributor

riri commented Nov 18, 2024

Agreeing with that, making an explicit issue denoting the backends on which draw issues persist.
Letting @RobLoach deciding.

@awschult002
Copy link
Contributor

i am going to post this here, but will migrate it to issue when that is created.

I spent some time really trying to figure out the issue of this rawfb checkbox in my PR #681 , but couldn't figure it out. HOWEVER! I believe I found a significant clue.

The shape of the internal box changes depending on whether you have rounded corners or not. What I mean is the offset problem we are seeing changes. Here is what the offset looks like in the "normal" execution path where in the internal rectangle is filled without rounding.
rect_fill_no_rounding

notice that the inner rectangle is bias into the upper left corner (0,0).

now take a look at the exact same thing, but with the do_toggle rectangle draw function asking for the same rectangle with rounding set to 1 instead of 0.
x11_with_rounding

it is a little difficult to see, but the box has shifted down to something more like (0,+1). This has lead me to investigate how the rectangles are actually drawn. If you don't use rounding, then a scissor algorithm is used, and if rounding is added, then arcs and lines are used. I played around with this a little bit and... as of this moment, I believe the issue to be with how we are drawing the scissor and basic primitives. basically. i think the issue is something like

/* draw rectangle no rounding */
for( i = 0; i < h; i++)
{
      stroke_line(i++)
}

Where in we loop to h EXCLUSIVE; but a height and width number might be considered an inclusive value.

I haven't solved the problem; but I believe the bug is something very akin to what I have noted above and it exists in nuklear_rawfb.h which is why all of the rawfb have the issue and nothing else.

@RobLoach
Copy link
Contributor Author

RobLoach commented Nov 18, 2024

Definitely onto something! Could it be that, in some renderers, lines are drawn differently depending on line thickness? Like you touched on at...

        } break;
        case NK_COMMAND_RECT: {
            const struct nk_command_rect *r = (const struct nk_command_rect *)cmd;
-            nk_xsurf_stroke_rect(surf, r->x, r->y, NK_MAX(r->w -r->line_thickness, 0),
-                NK_MAX(r->h - r->line_thickness, 0), (unsigned short)r->rounding,
+            nk_xsurf_stroke_rect(surf, r->x, r->y, r->w,
+                r->h, (unsigned short)r->rounding,
                r->line_thickness, r->color);
        } break;
        case NK_COMMAND_RECT_FILLED: {

Perhaps if there's a line thickness of 2, it should be * 0.5 the line thickness x position?

@awschult002
Copy link
Contributor

i would love to continue the discussion, but am afraid to keep posting here if in we get an issue where this investigation could continue.

@RobLoach RobLoach mentioned this pull request Nov 21, 2024
@riri
Copy link
Contributor

riri commented Nov 23, 2024

I just checked last version and it seems to work pretty well @RobLoach, on x11, x11_xft, xcb_cairo and rawfb/x11.

I didn't realize rounded corner disappeared in a previous commit, but this fixed it's not an issue.
Maybe a future PR could give the option to style the roundness or checkboxes ;)

With an updated patch version for nuklear (toggle changed) in clib.json and CHANGELOG, all seems ok to finally merge this so awaited evolution!

@awschult002
Copy link
Contributor

Yes. The rounded corners were removed because it made the issue look much worse. Rob and I discussed it in our other PR.

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.

4 participants