Supporting TextHovered and TextActive colors #511
Conversation
| { | ||
| va_list args; | ||
| va_start(args, fmt); | ||
| TextDisabledV(fmt, args); |
There was a problem hiding this comment.
Wrong call here. What are those functions really useful for anyway? (did you feel you needed them as user?)
There was a problem hiding this comment.
I do not need them at all.
It was a replication of the other ones.
I totally agree they are not useful.
There was a problem hiding this comment.
Want me to remove them and make another pull-request ?
(I'm not very familiar with github...)
There was a problem hiding this comment.
The pitfall with GIT that everyone falls into at least once, if that your pull-request is linked to your branch. So anything you push into this branch will be added to the PR.
This also mean you need to be mindful of your local branching. In theory you need to commit to the PR branch and then merge the PR branch in your working branch. Git is a little work..
|
Let's remove those helpers from now. I just found the TextDisabled pattern to be a little more common so it was a convenience helper. On the rest of the PR, it is a little difficult to take a stance and we may need to consider things in a more holistic manner. This isn't a bad step but I've been increasingly concerned about making subtle short-term changes for the styling and was hoping to take larger steps considering more things. The fact that you are applying TextActive/TextHovered over ButtonActive/ButtonHovered and HeaderActive/HeaderHovered and FrameBg is very arbitrary (and some widgets aren't using the new code). Does it imply that the new text color must be a fit over those 3 pairs of color? I love your theme, could you post the colors here? (How about existing user code changing the color of _Text ? Potential minor breakage, may be acceptable.) |
|
Sure I can share the theme, it is just color-picked from OSX theme for my personal project. About the other Hovered/Active colors, it doesn't make sense to modify the related widgets to use TextHovered/TextActive because they don't write any text which collides with coloured rectangles (ImGuiCol_ScrollbarGrabXXX, ImGuiCol_ColumnXXX, ImGuiCol_ResizeGripXXX, ImGuiCol_CloseButtonXXX). Ok, let's put these modifications away. But there is clearly a need for text colouring.
Mmmmh.. I don't see how to automatically change text colors depending on the state of the widgets. Especially if widgets are writing several texts with different colors (a DragFloat for example) |
|
Just to clarify, I am very interested in this patch. I'm just saying I may need to think about it along with other styling improvements rather than an isolated change.
But e.g. your patch doesn't modify DragFloat function but it could?
Not sure what you mean here. DragFloat render 1 piece of text inside the frame, like most other widgets. (But it also uses FrameBgActive, FrameBgHovered) |
|
Yes, DragFloat was modified, but it not anymore ;) For example, in the following capture, the number "3" should have been half-black and half-white. |
|
In your screenshot you are showing a Slider, not a Drag (drags don't have this issue). We could by using CPU-side clipping which is supported by ImFont with multples call to the text render (and/or modifying the text render code) but it may be too much hassle and too heavy. Another solution would be to draw a subtle transparent background (that is choosen to be an "opposite" of the text color) between those labels that are within a frame. Much simpler, may look odd ? Maybe making it super subtle. An idea maybe worth exploring. |
|
Another case where text colouring is important : This widget is a fork of your Histogram widget, taking raw values in input, calculating the histogram values, drawing the bins and other stuff (units, axis, average, deviation, etc), and obviously this is immediate and dynamic. |
Sorry, DragFloat is modified (but its label is not modified). And Slider is not. |
|
Nice graph (keep bribing me with cool screenshots!) Even if CPU-side clipping with multiple renders is possible I think it would be a little overkill in term of maintenance and performances. If you want to generalize this recoloring scheme it may lead to use of stencil buffer or different shaders. Much easier to have a soft background but a little uglier. I don't know what is a satisfying solution. Your graph example isn't looking so bad to be fair. |
|
Sure, I'm happy with this modified histogram widget. CPU clipping is not a solution.. definitely. Partial text coloring could be done by splitting the text in more vertex-buffers with different colors. I mean, not only splitting the text in characters having their own colors, but splitting a single character in two (having different colors). This would be quite easily possible horizontally and vertically. Having complex shapes would make it a mess. |
This is exactly what the CPU clipping does for you. For a specify text call you can request it to be clipped. You'd have to do multiple pass on the text (which isn't a big deal as this is only happen for visible items anyway, coarse clipping gets rid of widget when they are out of visibility). |
|
Ok, I will look at the code tomorrow evening to see what CPU clipping really is. Thx |
* commit '62fe0b59bfb879dd3b2b857139f1acd4c5f23781': Updated todo list ImDrawList: fixed index overflow check broken by AddText(). Added extra assert. (#514) Fixed removal of trailing draw command if it is a callback command InputTex(): Comment (#512) InputText() fixed clipping bounds of contents (#512)
* 'master' of https://github.com/ocornut/imgui: (27 commits) Comments DragFloat(): always apply value when mouse is held/widget active, so that can use a drag over an always-reseting value Examples: OpenGL: Fix early return on zero-sized framebuffer breaking GL state (#486, #547) Comment (#544) InputText() ImGuiInputTextFlags_CallbackAlways event set the EventFlag field of ImGuiTextEditCallbackData (#541) Removed extraneous comma for pedantic compilers (#516) Minor tidying up following (#516) - renamed ImGuiSelectableFlags_HandleDoubleClick to ImGuiSelectableFlags_AllowDoubleClick + comments CaptureKeyboardFromApp() / CaptureMouseFromApp(): allow to enforce clearing the capture flag (#533) + demo + made code a little less messy InputText: Added BufTextLen in ImGuiTextEditCallbackData. Requesting user to maintain it. Zero-ing structure properly before use. (#541) BeginChild()/EndChild() fixed incorrect layout to allow widgets submitted after an auto-fit child wnidow (#540) Demo: plot code doesn't use ImVector to avoid heap allocation + comment (#538) TextUnformatted: Fixed rare crash bug with large blurb of text (2k+) not finishing with a '\n' and fully above the clipping Y line. (#535) Demo: Fixed malloc/free mismatch and leak when destructing demo console (if it has been used) (#536) Exposed FindWindowByName() in imgui_internal.h (missing chunk following 339b67c) Update README.md Update README.md Exposed FindWindowByName() in imgui_internal.h Examples: OpenGL: skip rendering and calling glViewport() if we have a zero-fixed buffer (#486) Examples: SDL/OpenGL: Tabs to spaces Examples: OpenGL3: Fix BindVertexArray/BindBuffer order (#527) ...
|
Pacome, So I went back to this PR in more details tonight. My understanding is that the changes are totally arbitrary here and don't really make sense in the wider theming context. It currently just assume that text color XXX is a good fit over multiple arbitrary background colors, leaving little room to adjust settings as it becomes a bit of puzzle to satisfy readability. Have you played with theming further? I believe the righter way would be that all "background/filling" color entries may have an optional Text color (defaulting to the main Text color?). We could possibly reorganize the style structure to carry 2 colors per value of the enum so we don't have to pollute the enum further. This would however be a much wider change which we can take to another PR. While doing it I would also like to pre-encode colors as U32 to save a little CPU (we still have an alpha multiplier to apply but it is best applied with integer multiple / masking). ( I am sort of relieved to notice that other people have been using your theme without this change, making it less of an issue to drop it! ). |
* 'master' of https://github.com/ocornut/imgui: (65 commits) Silence borderline warning with -Werror=strict-overflow Update README.md Examples: Apple: Readme tweaks (#575 #247) Remove local glfw3 lib for osx. (+1 squashed commit) Squashed commits: [34cc3b7] Adds osx example. (+6 squashed commits) Squashed commits: [20330f2] Uses glfw by brew install. [0427861] Renames imguiex folder name to imguiex-ios [f9e27e5] Renames ios_example to apple_example. [44f8fe3] Updates the glfw header/library path. [919f279] Renames target from imguiex to imguiex-ios since there is already a imguiex-osx target now. [24395f5] Adds osx example. Trivial format string fix in demo Style: removed WindowFillAlphaDefault which was confusing and redundant, baked into WindowBg color. Renamed TooltipBg > PopupBG. (#337) Fixed InputTextMultiLine(), ListBox(), BeginChildFrame(): outer frame not honoring bordering (following #462) Added ImGuiWindowFlags_AlwaysUseWindowPadding flag to ensure non-border child window uses window padding (#462) Renamed ImGuiWindowFlags_Force**Scrollbar to ImGuiWindowFlags_Always**Scrollbar (#476) Examples: Libs: Update glfw binaries to glfw master. Examples: Vulkan: Coding style tweaks. Comments Examples: DirectX10,DirectX11 : Minor renaming Examples: DirectX10: Save/restore state + minor cleanups (#570) Examples: DirectX11: Shallow massaging to make the code more consistent/readable (following #570) Examples: DX11: Cleanup state backup/restore code (#570) capture and restore all state Updated todo list and comments Moved EndFrame() back to imgui_internal.h + comments. Undo cfbf06e Update README.md ... # Conflicts: # imgui.cpp # imgui.h
…w showing borders.
|
@itamago FYI you just f*cked your PR by committing more to the same branch (everybody does that same mistake all the time, in Git 1 PR = 1 branch). Not a problem tho, don't worry or delete anything. We can always cherry-pick commits. I am keeping this thread alive because I want to solve this problem. Also on twitter
Essentially the same problem.
|
|
What is the motivation behind groups? Is it just to allow backwards compatibility, or are they are more general feature (e.g. do you plan to add groups for other attribute such as widget background colors or roundings)? If it's just for backwards compatibility, I think it needlessly adds clutter to the API... How would groups play with global styles? I.e. would Col_Text global style var behave differently than pushing Col_Text (which would be group behavior)? Personally I would just add text colors ImGuiCol_XXXText for every widget that has a customizable background color. Yes, it would introduce a minor backwards compatibility problem but easy to fix in user code. From my POV it's ok that creating new styles in a little hard, after all it's pretty rare occasion. |
|
Good question, idea initially came to me for backward compatibility but I think it may be a convenient thing as well. If you are going to be manipulating colors in code then second-guessing enumeration values is tricky, particularly if we are introducing new ones from time to time. It's not a fleshed idea and may be wrong, need to spend more time with real code (looking at bigger codebases). I also liked idea that you could say "I want super-tight packed widgets" and it would affect various padding/spacing values at once. Again not fleshed at all.
Yes-ish, we could consider that there's no "Col_Text" stored variable at all. That would perhaps break compatibility which defeat a bit of the intent. Standardizing styling to add a Text entry for each Background etc. may just be the way to go. My main concern is that if we are going to break styling somehow let's break it once and not three times. Perhaps a "group" is just a way to collect those Text/Background/Active/Hovered info in one place, so we make those enums more consistent. Perhaps manipulating values in HSV places, with less parameters, e.g. Active or Hovered could be only 2 style-wide h/s/v delta values applied other values, leading to simplified styling (like #438). How about the Disabled state? There's currently a TextDisabled entry which is meant to lead toward a full "Disabled" state (#211) that you can set and turn all the widget in scope unusable, with grey text. The combinatory is huge, there's potentially a time-sink to open with styling so I am progressing very cautiously. We could accept that "creating new styles in a little hard, after all it's pretty rare occasion." if we accept a reduction of value of manipulating style in code (pushing many values may be confusing/cumbersome/slow). Sorry this is just dumping random ideas and rambling at this point. One needs to sit a few days to resolve the styling situation! All those discussions are helping. |
|
It's good to think about this thoroughly before rushing in. One thing tho, I don't like the idea of HSV if it's the only way -- limits flexibility too much in my opinion. I think at the lowest level the API should allow customizing everything. On top of that, an optional styling layer would make sense, so that the user could choose between explicitly setting up colors, or using an easier to use, less flexible styling layer. Another concern of mine is that if you try hard to think about the perfect styling system before implementing and test driving it on users, it may never happen. Sometimes designing the perfect system on paper just does not work or you get stuck with analysis paralysis :-) Just trying to say that the requirement of "breaking styling only once" may be too much to ask. Besides even if you find the perfect solution now, an even better solution may come to mind later. Re: "pushing many values may be confusing/cumbersome/slow." Maybe I'm missing something but I rarely push style vars (I mainly use global styles). Sometimes I have customized a single widget but that's it. So maybe I'm not seeing the bigger picture and the benefit of the push/pull color groups idea. But yeah, now I'm rambling. Bottom line, take your time and keep up the great work you're doing! |
|
Motivational screen shot of the button style I have in mind:
|
|
<3 |
Hello,
These are minor changes in order to support TextHovered and TextActive colors.
It could be seen as nothing, but it is mandatory to have these 2 new colors when using high-contrast GUIs.
A capture of the usual test case :
A capture of a contrasted GUI. Please look at the "Layout" text which is white instead of black, which makes it more readable. The same for Buttons, Selectables, etc.
