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

Add custom cursors to scrollbars, buttons, sliders, drag widgets, etc. #6442

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Madman10K
Copy link

Hello, firstly, thanks for maintaining such a great GUI library.

How I found this problem

Since dear imgui has support for setting and using different cursors, I decided it would be a good idea to use the hand cursor for buttons. But after scrolling through the settings and the source code, I found that no such feature was present.

This is what cursors look like normally:

image

Now with the patch:

image

I am aware of the fact that I can use ImGui::SetMouseCursor(ImGuiMouseCursor_...) like how it is shown in the demo window, however, this would bloat too much of the code for such a small aesthetic feature and would require me to rewrite 1000s of lines of perfectly functioning dear imgui code.

I decided that the best course of action would be to integrate it into the backend.

Why make this PR

I think this would be beneficial to the dear imgui community, because by adding this, developers can provide better UX for their end users. Additionally, taking an example from the most used UI framework, the web, buttons and links on almost all sites use the Hand button, for example.

Furthermore, I have created a new IO variable ConfigUseDefaultMouseCursors, which can be used to enable/disable this feature with it being false by default, as to not break any existing behaviour. This way, we can still preserve the current imgui style, while allowing these mouse cursors to be turned on, for those who need them.

Contents of the PR

This PR adds the following:

  1. An IO variable ConfigUseDefaultMouseCursors, which can be used to enable/disable this feature, which is set to false by default, as to not break any existing behaviour
  2. The actual cursor change code
    1. Sets the cursor to resize all while dragging and dropping
    2. Sets the cursor to hand on any widget using ButtonBehaviour, except for InvisibleButton, since that widget is used by many non-button interfaces, and it's not that common, so it would be better to use the regular cursor
    3. Sets the cursor to "Resize_EW" on horizontal sliders and drag widgets
    4. Sets the cursor to "Resize_NS" on vertical sliders(this automatically includes scrollbars)
  3. Code related to IO in the demo file

Showcase

Horizontal slider:

image

Vertical slider:

image

Drag and drop:

image

Horizontal scrollbar:

image

Testing

I tested this patch with the latest dear imgui version and using the following examples:

  1. null
  2. glfw_opengl3
  3. glfw_opengl2
  4. glfw_vulkan
  5. sdl2_opengl3
  6. sdl2_opengl2
  7. sdl2_sdlrenderer
  8. sdl2_vulkan
  9. glut_opengl2

The rest are not tested for the following reasons:

  1. allegro - Allegro package is broken on my Linux distribution
  2. android - Android emulators are broken on my system
  3. apple - Don't have an apple system
  4. glfw_metal - Don't have an apple system
  5. sdl2_directx - Don't have a Windows system
  6. sdl2_metal - Don't have an apple system
  7. sdl3 - SDL3 is in the testing phase but still not included in my Linux distribution as of now
  8. win32 - Don't have a Windows system
  9. wgpu - WGPU is still not supported on Chrome or Firefox on Linux
@ocornut ocornut added the inputs label May 20, 2023
@ocornut
Copy link
Owner

ocornut commented May 21, 2023

Hello,

Thanks for the PR! This is a good reference.

Some feedback:

  • I am not sure it is ideal to modify the signature of ButtonBehavior(). Have you tried doing it from the caller function using the bool hovered info emitted from the function?
  • Explicitly setting ImGuiMouseCursor_Arrow in e.g. InvisibleButton() seems misleading and having side-effects, there should be a way to not make any change to the cursor there instead of setting back the expected "default" cursor, e.g. by using ImGuiMouseCursor_COUNT as a marker for no-op.
  • It doesn't seem desirable to be calling IsItemHovered() - a user facing function - so often when internal widgets usually track their hovering state via ButtonBehavior(). Also because that earlier function returns true on keyboard focused item when using keyboard.
  • if ((A & ImGuiItemFlags_ReadOnly) || (B & ImGuiItemFlags_ReadOnly)) can be reworded as if ((A | B) & ImGuiItemFlags_ReadOnly).

Design wise:

  • Windows 10-11 itself doesn't seem to change cursor on everything. So if anything I feel that with this PR and the feature enabled, my experience is actually more distant from what I feel is my standard of a desktop-productivity-app experience. Those apps and apps build with Dear ImGui tends to be more dense, with explicit clicking points and less graphical variety. As I am seeing it, menus would know show a hand cursor all the time. I think the reason the web uses Hand cursor so much is to cater for visual variety and/or visual clutter hindering affordance.
  • Whenever/if people start enabling it, I worry It will undoubtedly put more responsibility on both the UI system and the user. Cases where "I don't want this or this cursor" etc. Also if opt-in it means people writing custom code/widget may need to cater for a feature they haven't used nor enabled, leading to small lingering issues.
  • Stuff like EW arrows on horizontal sliders seems to go hand in hand with their look. If I look at my volume control in window, with a thin bar and large round thing to grab, it doesn't change cursor and I think it would be odd to do so. I suspect the righter solution to improve affordance is to be widgets that are a little more opinionated in their look.
  • I would be more cautious by enabling this on selected widgets with specific semantic e.g. Button() rather than across the board., but as soon as we start using the Hand one things needs to be consistent so it's not easy.

I'm not really sure what to do about this, writing my thoughts down for down.

@Madman10K
Copy link
Author

Hello,

Thanks for the feedback. I will comment on and fix the immediate technical issues and concerns within a couple of hours.

As for the design plan:

Hand cursor on buttons

I do agree that the behaviour of having the hand button on every button is non-standard, even on the web, buttons were not designed to have this type of cursor. For one reason or another, it has now become the unofficial standard to use hand buttons on the web. There are many arguments for and against it, which I will not get into.

I think that having this feature being optional, because of its unofficial nature, is the best way to handle it. However, it may be better, as you said, to have a type of setting that, for example, enables this only on ButtonXYZ and not on all widgets using ButtonBehaviour.

The same thing can be done for sliders, etc.

The "I don't want this cursor" issue

If we take the feature from above, why not make the user able to select the type of cursor for these widgets too

Third party widgets' implementation

I'm not sure how to handle this. I looked at the internals of the most popular libraries to calculate some type of rough impact:

  1. imgui-knobs - Uses invisible button and drag
  2. imspinner - Doesn't use any button/slider functionality, so no issues there
  3. implot
    1. Uses ButtonBehaviour on the legends, here it might actually be beneficial to have this
    2. Legend context menu uses a good number of buttons and radio buttons
    3. ButtonBehaviour in UpdateInput
    4. ButtonBehaviour in BeginSubplots to render splitters(setting the default to Hand here is a behaviour breakage)
    5. ButtonBehaviour in DragPoint, DragLine, DragRect
    6. Button in ColormapButton
    7. Buttons and SliderFloat in ShowStyleEditor - This is generally OK
    8. Buttons and Checkboxes in ShowMetricsWindow - This is generally OK
    9. Buttons in implot_demo - This is generally OK
    10. Buttons in ShowDatePicker and ShowTimePicker
  4. imgui_toggle
    1. ToggleBehaviour uses ButtonBehaviour
  5. ImGuizmo
    1. ImZoomSlider uses InvisibleButton and slider flags, this most definitely is a breakage
    2. ImSequencer uses InvisibleButton in multiple places and Selectable
    3. GraphEditor uses InvisibleButton, Buttons and more

Well... this is not ideal. There is basically no non-intervention way to fix this. I will add these libraries to a test project to make a more detailed impact report soon.

@Madman10K
Copy link
Author

  • I am not sure it is ideal to modify the signature of ButtonBehavior(). Have you tried doing it from the caller function using the bool hovered info emitted from the function?

Applied, signature returned to old state and called SetMouseCursor from every caller. This should also help as a prerequisite if we decide to have a mouse cursor set for each widget

  • Explicitly setting ImGuiMouseCursor_Arrow in e.g. InvisibleButton() seems misleading and having side-effects, there should be a way to not make any change to the cursor there instead of setting back the expected "default" cursor, e.g. by using ImGuiMouseCursor_COUNT as a marker for no-op.

Now we don't call SetMouseCursor at all. This makes it behave as the default cursor.

  • It doesn't seem desirable to be calling IsItemHovered() - a user facing function - so often when internal widgets usually track their hovering state via ButtonBehavior(). Also because that earlier function returns true on keyboard focused item when using keyboard.

Switched to checking with g.HoveredId == id

  • if ((A & ImGuiItemFlags_ReadOnly) || (B & ImGuiItemFlags_ReadOnly)) can be reworded as if ((A | B) & ImGuiItemFlags_ReadOnly).

Switched to recommended pattern

@Madman10K
Copy link
Author

By the way, SplitterBehavior calls the following code, when switching the mouse cursor:

    if (held || (hovered && g.HoveredIdPreviousFrame == id && g.HoveredIdTimer >= hover_visibility_delay))
        SetMouseCursor(axis == ImGuiAxis_Y ? ImGuiMouseCursor_ResizeNS : ImGuiMouseCursor_ResizeEW);

Should this also be applied to the other widgets?

@ocornut
Copy link
Owner

ocornut commented May 21, 2023

By the way, SplitterBehavior calls the following code, when switching the mouse cursor:
Should this also be applied to the other widgets?

This is designed to reduce visual noise when moving mouse around, especially since they are thin borders that are frequently crossed over with no intent of staying over.
I don’t think we need a similar thing as the default behavior.

@ocornut
Copy link
Owner

ocornut commented May 24, 2023

Merge branch 'ocornut:master' into master

May I suggest to occasionally rebase over master + force push instead of merging master + push, this will create less commit cruft in the history. (But it's not too important as normally if we adopt the commit, we can squash and rebase).

@Madman10K
Copy link
Author

I'm sorry, I was iterating over my private forks and this seems to have passed through. I will discard the commit right away

@Madman10K
Copy link
Author

Thinking about this, what is your opinion on making this into a style-var-like API? The benefits of this, in my eyes, are these:

  1. This API style is already familiar to most users of dear imgui
  2. The user has full control over what cursors apply to which widgets through enum members that apply to each widget type
  3. The ability to Push and Pop would be useful for fine-grain control over cursor style
  4. Third party libraries theoretically don't have to touch any of their code, because this is style. Thus, we shift responsibility for correct styling to the user.
  5. We can apply defaults that are identical to current day imgui cursor styling, which wouldn't break existing styles
@ocornut
Copy link
Owner

ocornut commented May 30, 2023

I am sorry can't validate or come up with a design when designing this is the main chunk of the work.
My intuition is that this path would add too much API surface for what's a non-essential feature.
I'm not against it but I need to focus on more important stuff presently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 participants