Nuklear icon indicating copy to clipboard operation
Nuklear copied to clipboard

v 4.10.0: Multiple PRs merged

Open Nielsbishere opened this issue 5 years ago • 14 comments

Multiple PRs:

  • https://github.com/Immediate-Mode-UI/Nuklear/pull/50
  • https://github.com/Immediate-Mode-UI/Nuklear/pull/35
  • https://github.com/Immediate-Mode-UI/Nuklear/pull/15
  • https://github.com/Immediate-Mode-UI/Nuklear/pull/14
  • https://github.com/Immediate-Mode-UI/Nuklear/pull/13

Various changes such as:

  • Fixing clicking through multiple overlapping windows
  • Allowing subpixel rendering
  • Allowing add_window to return a Window pointer to read out information useful to an abstraction that isn't immediate mode over nuklear.
  • Fixed radio buttons style
  • Fixed compile issues with C++17 (-Werr -W3)
  • Trees can now have 'child' nodes, which are supposed to be the final stop for childless nodes. This isn't fully true, but it can be used to have another tree node type. It doesn't have a symbol as a file should as opposed to a folder.

Nielsbishere avatar Jul 06 '20 15:07 Nielsbishere

Looks like there's a few git conflicts :eyes:

RobLoach avatar Dec 14 '21 04:12 RobLoach

I'm sorry for us not getting earlier to this. I think this PR is quite valuable and was a lot of work to put it all together. We should merge (parts of?) it after a thorough review. Would anyone be willing to update it to fix the merge conflicts? I'm constantly fighting my tighter and tighter schedule...

dumblob avatar Dec 14 '21 09:12 dumblob

There we go

Nielsbishere avatar Dec 15 '21 19:12 Nielsbishere

I've skimmed it (really no review yet) and it actually looks surprisingly good to me. @RobLoach could you please thoroughly review it? @Hejsil would you also find some time to take a look at this rather sensitive PR? The more eyes the better.

dumblob avatar Dec 15 '21 20:12 dumblob

@Hejsil I agree, but it was requested I merge this a long time ago iirc.

Nielsbishere avatar Dec 15 '21 22:12 Nielsbishere

@Nielsbishere yep, we thought that merging small changes will make it more appropriate. As it turns out, not all the changes are as small as anticipated. Do you think you could help us with splitting the PR (at least the major bump versus other changes) and cleaning it as discussed above?

dumblob avatar Dec 16 '21 09:12 dumblob

I double checked this branch to see if all works well in case it ends up being pulled in. There's only one thing that bugs me, I'm not sure if it's on purpose but the color picker widget isn't following my mouse cursor anymore.

You see when I click, I'm holding the mouse, but the color cursor isn't following the mouse cursor: GIF 12-24-2021 9-35-00 PM

This is the expected behaviour: GIF 12-24-2021 9-36-15 PM

aganm avatar Dec 25 '21 02:12 aganm

Oh yeah I will get to this PR in a bit, just busy. @aganm I'm guessing the event is being consumed before it reaches it for some reason. @dumblob I think I should probably apply those fixes. I'm not sure how to split it in different branches without rewriting it.

Nielsbishere avatar Dec 25 '21 02:12 Nielsbishere

@aganm thanks for the quick test! That's very important to do. Feel free to continue your endeavor if you find some time for it. We'll be thankful for that!

@dumblob I think I should probably apply those fixes. I'm not sure how to split it in different branches without rewriting it.

Yeah, different fixes will be necessary. Regarding splitting, what do you mean by "rewriting it"? Just take few related commits into a separate branch and then consolidate the result with one new commit in that new branch (fixing the discrepancies which are to be expected). Do this until you run out of all the commits from this PR :wink:. This PR will then get closed as superseeded by all the new PRs (for each new branch). Sounds tedious but in this case it's really valuable and we'll be greateful if you managed. Take your time.

Thanks!

dumblob avatar Dec 28 '21 13:12 dumblob

I double checked this branch to see if all works well in case it ends up being pulled in. There's only one thing that bugs me, I'm not sure if it's on purpose but the color picker widget isn't following my mouse cursor anymore.

Found a fix that worked for me. I'm not sure what else this may affect but it seemed to retain the click through fix

diff --git a/src/nuklear_input.c b/src/nuklear_input.c
index 6bcb97d..a8f5e05 100644
--- a/src/nuklear_input.c
+++ b/src/nuklear_input.c
@@ -154,7 +154,7 @@ nk_input_has_mouse_click_in_rect(const struct nk_input *i, enum nk_buttons id,
     struct nk_rect b)
 {
     const struct nk_mouse_button *btn;
-    if (!i || i->mouse.clicked) return nk_false;
+    if (!i) return nk_false;
     btn = &i->mouse.buttons[id];
     if (!NK_INBOX(btn->clicked_pos.x,btn->clicked_pos.y,b.x,b.y,b.w,b.h))
         return nk_false;

mecwerks avatar Nov 22 '22 15:11 mecwerks

@mecwerks Thanks for that, I have looked into it and this change comes from commit

8e89387e2d946545a19bbe61bd1bafaa6da90c7d Merge branch 'fix_click' of https://github.com/Nielsbishere/Nuklear into fix_radio_buttons

It seems like the reason for this change was to fix click related to radio buttons, but, I have tested your proposed fix and it seems to work, both color pickers and radio buttons seem to work perfectly with this fix!

Fix which is a revert of this one specific line https://github.com/Immediate-Mode-UI/Nuklear/blob/8e89387e2d946545a19bbe61bd1bafaa6da90c7d/src/nuklear_input.c#L157 back to what it was before this PR https://github.com/Immediate-Mode-UI/Nuklear/blob/3a0aafb9cc117b66d6f1cfcc36bb6d0c8dc2ea08/src/nuklear_input.c#L151

With this fix I would be happy if we could merge this PR.

aganm avatar Nov 22 '22 16:11 aganm

Oh hey it's my old branch. @aganm @mecwerks the reason for this change was quite important; if you have a dropdown window over a radio button, it'll push through the window into the radio button. The proper fix would be to figure out why the color picker has this behavior. Maybe this check is in the wrong place; preventing that behavior? But letting the radio button be clicked while the input event is already consumed is not good imo

Nielsbishere avatar Nov 22 '22 16:11 Nielsbishere

Oh hey it's my old branch. @aganm @mecwerks the reason for this change was quite important; if you have a dropdown window over a radio button, it'll push through the window into the radio button. The proper fix would be to figure out why the color picker has this behavior. Maybe this check is in the wrong place; preventing that behavior? But letting the radio button be clicked while the input event is already consumed is not good imo

It seems both color picker and radio button rely on the same underlying nk_button_behavior function. But the difference is that the radio button calls nk_button_behavior with a behavior parameter of NK_BUTTON_DEFAULT and the color picker calls nk_button_behavior with a behavior parameter of NK_BUTTON_REPEATED. The function nk_button_behavior seems to not handle the repeated mode correctly.

aganm avatar Nov 22 '22 17:11 aganm

@aganm sounds like a solution is to only check if it's not repeated then. I'm not continuing this branch as my current project doesn't use it. Maybe if I ever use it again, but I don't have any time left to do this rn

Nielsbishere avatar Nov 22 '22 17:11 Nielsbishere