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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Colorize query status results and highlight on change #7074

Closed
wants to merge 1 commit into from

Conversation

Nahor
Copy link
Contributor

@Nahor Nahor commented Nov 29, 2023

In the demo, when query the item and window statuses, colorize the background to make the boolean values more visible (green/red for true/false, blue for others).

Also, at high FPS, it's very hard to see when the value changes, especially when it's transient, like the return value or clicks. So make the changes more visible by flashing the background.

example:

Flash.mp4

image
...with the mouse hovering the child window, which Windows Snip tool doesn't capture 馃槥

@ocornut
Copy link
Owner

ocornut commented Nov 29, 2023

Hello,
Thanks for the PR!

In principle I like this idea but the amount of extra code (+116 lines to the demo) and use of coding-style not matching style of imgui_demo.cpp makes it something I am unlikely to integrate at the moment. Efforts should be made so that custom logic should be as local as possible to the code using it, and generally to reduce code to a minimum. I'm not entirely sure there is a better way to do that to that to be honest, but in the present condition it seems a little too noisy to be considered for inclusion. I'll keep this open either way to reevaluate later as the idea is good.

That, or we expand on this idea to provide an in-imgui debug tool that does something like that (e.g. something you can call manually after an item to output its state), which actually was something on my todo-list already.

@Nahor
Copy link
Contributor Author

Nahor commented Nov 29, 2023

+116 lines to the demo

108 but whose counting 馃槤.
There are 45 values/variables total to define/declare, update and display. The define/declare+update are new to this commit. Only display was already there. So that's about 2 extra-lines of code to do 2 extra operations plus a more advanced display. So all in all, I think the 108 lines of extra code is pretty efficient, if I may say so myself.

There is however still a lot of repetitive code. This could be simplified into loops but at the cost of using more C++11 code (lambdas) and dynamically allocation (example)

coding-style not matching style of imgui_demo.cpp

I just saw now that there are other "helper functions" and "structs" define just before the ShowDemoXxx functions that uses them. When I first saw the "[SECTION] Helpers" comment, I didn't think to search further if there was more to it.
I missed the extra spaces in IM_LERP.
The one place where I purposefully did not follow the style conventions was for the name of the fields inside the struct. Instead I used the name of the matching function so that it would be easier to update when new functions/flags are added by just copy/pasting those function calls (minus the redundant prefixes and the invalid characters for variables). (Note: if you use lambda as proposed above, this would become a non-issue)

custom logic should be as local as possible

That's why the structs are declared inside the functions. But yes, as mentioned above, I missed the fact that some of the ShowDemoXxx had their extra functions/struct outside the "Helpers" code section.

@Nahor
Copy link
Contributor Author

Nahor commented Nov 30, 2023

I've pushed an update to use lambdas. That removes a lot of redundant code (only 68 new lines 鉁岋笍). Adding new fields is now just one line per field, with the added benefit that the format string and the Is/GetFunction are together instead of separate lines.

The main issue is the use of inheritance and gasp virtual functions. The heap allocation of static variables sucks as well. However, the only alternatives I could think of was using some kind of variant/union, or to separate arrays for the bool vs ImVec2 values, or to use tuples. None felt compelling enough to me though.

I've also done another pass at trying to figure out what the coding style is (to be blunt though, it's not easy without some more complete documentation than the 2 lines mentioned in CONTRIBUTING.md, especially since I've seen so much code in my life that I've learned to ignore it 馃槢)

@ocornut
Copy link
Owner

ocornut commented Nov 30, 2023

I am afraid this is worse in term of coding style :(
Allocations are absolutely forbidden on the idle path everywhere in dear imgui.
Linking to 1e74fb0 as I'll use that for reference.

I'm not against this but honestly since it isn't an easy slam-dunk in term of merge I'd consider it not urgent. The real underlying problem is that those small requests or features, however legit, tends to be hogging resources away from bigger, more important features.

@Nahor
Copy link
Contributor Author

Nahor commented Nov 30, 2023

I see. Sorry to be a burden. I won't bother you again.

@ocornut
Copy link
Owner

ocornut commented Nov 30, 2023

Apologies if my answer was misleading: I appreciate the work going into PR like this.
It's just that since it isn't a trivial merge for me (and would take me several hours of work to turn it into a state I'm happy with), I'm forced to treat it as low priority. But I'm happy to keep this open as the idea is good.

@ocornut
Copy link
Owner

ocornut commented Dec 11, 2023

Not sure if you closed that accidentally, afaik since it used Nahor:master as source would it could easily break?
I cannot reopen closed PR unfortunately but keeping a note this is an interesting idea to eventually pursue.

@Nahor
Copy link
Contributor Author

Nahor commented Dec 11, 2023

It was accidental.
I've never really used Github for PR before and I didn't realized that they were linked to the branch name rather that the commit ID. So when I initially created the PR, I didn't bother with creating a new branch just for that one commit.
And even after finding out, I didn't expect it would close the PR when I reorganized everything 馃槥.

I do have a branch for it now if necessary (but then you said you liked that the 2nd commit even less than the first...)

@ocornut
Copy link
Owner

ocornut commented Dec 12, 2023

The idea is good i鈥檓 just not sure what the best strategy is to implement it neatly. I tend to like to keep ideas around, i鈥檒l remember this one and know where to find it (normally the first commit hash is still available).

In the demo, when query the item and window statuses, colorize
the background to make the boolean values more visible (green/red for
true/false, blue for others).

Also, at high FPS, it's very hard to see when the value changes,
especially when it's transient, like the return value or clicks. So
make the changes more visible by flashing the background.
@Nahor Nahor reopened this Dec 12, 2023
@Nahor
Copy link
Contributor Author

Nahor commented Dec 12, 2023

Hmm I could reopen the PR by re-pushing to master and I see a way to move the PR into another branch (via the Edit button in the PR), but I don't see a way to pull from another branch. I can't really keep my master on that commit though.

So, options:

  1. Reset my master and get the PR auto-close again, and you keep track of the change whichever way you prefer.
  2. Generate a PR from the colorize_status branch and see what that does (merge with this PR, since it's the same commit, or create a new PR and you start tracking that one instead,... assuming that I can try to push a PR in the first place)

Preference?

@Nahor
Copy link
Contributor Author

Nahor commented Dec 12, 2023

Closing in favor of #7127

@Nahor Nahor closed this Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants