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

Fix horizontal scrolling with touchpad devices #3795

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

Conversation

folays
Copy link

@folays folays commented Feb 9, 2021

Description

Fix scroll-x on touchpad

The standalone imgui "demo" exhibit the faulty behavior.
Table & columns -> Advanced ; Open Options

  • reduce item count to a few (~5), so the Table is not "full" y-wise and thus y-scrollbar disappear
  • raise the inner width to go beyond the width so that x-scrollbar appear

You are now in a situation with a Table having x-scrollbar but not y-scrollbar.

You can of course easily recreate the same situation in homemade code. Just fill a Table with large lignes (number of columns), small number of items.

With a touchpad (at least on Mac), the x-scrollbar is 99% unusable.

The x-scrollbar can be moved left/right ONLY IF we very very carefully (and with difficulty) touch the touchpad so that the operating system only issue a +/- on the x-axis and 0 on the y-axis.

Because the Table is not interested for scroll-y, the event is being "bubbled up" to the parent window.
It mess with the code which would have taken interest in the x-scroll on behalf of the Table.

Expected behavior

I guess that when the user has a mouse with two scroll wheels, the users can easily choose to "scroll only on X and be neutral (0) on Y".

But I have a hard time figuring out any situation where an user would really wish to AT THE SAME TIME scrolling on two different widgets (parent and children).
I guess that the expected behavior are rather:

  • scroll only on X and ignore the other axis
  • scroll only on Y and ignore the other axis
  • scroll both on X and Y (diagonally) but only if inside the same widget

FIX of the pull request

If children window has interest for scroll-x, the fix prevent bubbling scroll-y events and lock further wheeling events to parent window.

The code changes of the pull request are including 3 components ;
The first 2 changes below should not change the "logic" of the code, they are for readability only.
The 3 change below should fix the bug.

1) Simplify the 2 lines calculating const float: wheel_y and wheel_x.

The two artimethic operations are being factorized to their simpler form.
The results should be 100% the same than before, if I'm right, it's only some boolean arithmetics.

Those two calculations are also moved at the top of the diff, the patch needing those resuls early.

2) Replace a frequent calculus by its own boolean

I replaced:

  • BEFORE : (window->Flags & ImGuiWindowFlags_NoScrollWithMouse) && !(window->Flags & ImGuiWindowFlags_NoMouseInputs) everywhere
  • AFTER : the bool noScroll in replacements

3) The bug fix

The patch aim to stop bubbling the event as soon as a widget has interest for either scroll-x or scroll-y.
If the widget has interest for only one of the two scrolling axes, the second one will be ignored, which is probably the better situation.

I do not pretend to have inside knowledge of the internal of imgui.
I think I still have respected the intention of the original code.

In the case where the widget has no interest for one of the two scrolling axes, I guess a flag could also be introduced which would enable/disable bubbling up the ignored event to the parent window.
But it would probably require to split the StartLockWheelingWindow subsystem for the two axes.

…urther wheeling events to parent window, if children window has interest for scroll-x)
@ocornut ocornut changed the title fix scroll-x on touchpad Fir horizontal scrolling with touchpad devices Feb 10, 2021
@ocornut ocornut changed the title Fir horizontal scrolling with touchpad devices Fix horizontal scrolling with touchpad devices Feb 24, 2021
@ocornut
Copy link
Owner

ocornut commented Apr 6, 2021

Hello,

Thanks for reporting this. I initially misunderstood the issue on my first read but now I realize that the scrolling events being dispatched to two windows is indeed wrong. Will fix this :)

  1. Replace a frequent calculus by its own boolean

The purpose of the test is to climb up the parent hierarchy until we are stopped by those flags. Your change locks the flags to the first immediately hovered window which seems like a very different behavior.

Don't worry about reworking/fixing it, we'll be looking at this very shortly (along with other mouse-wheel related changes). Thanks a lot!

@folays
Copy link
Author

folays commented Dec 22, 2021

Hi, I have read recent discussions in #4559 ;

My most recent comment (of today) in #4559 should be read, before this one comment here.

My #3795 above contained three "proposals"/"fixes" :

1) "Simplify the 2 lines calculating const float: wheel_y and wheel_x"...

...You seem to already have figured out this, by "factoring" the "?:" operation and removed the duplicate "g.IO.MouseWheel" which occured at the version I then had.
-> Thus, my request is deprecated on this point.

2) "Replace a frequent calculus by its own boolean"...

...Which I think would still be relevant today, because I think it would really simplify readability of the code still exhibing the problem.

This proposal is to simply "sed" replacing, without changing applied "logic":

replace :

(window->Flags & ImGuiWindowFlags_NoScrollWithMouse) && !(window->Flags & ImGuiWindowFlags_NoMouseInputs)

with its own boolean, like :

bool noScroll = 
(window->Flags & ImGuiWindowFlags_NoScrollWithMouse) && !(window->Flags & ImGuiWindowFlags_NoMouseInputs);

(new-lined just so that you see that the boolean contained the same thing than the original code).
In the current "master" branch :

imgui/imgui.cpp

Lines 3871 to 3876 in 612b787

if (wheel_y != 0.0f)
{
StartLockWheelingWindow(window);
while ((window->Flags & ImGuiWindowFlags_ChildWindow) && ((window->ScrollMax.y == 0.0f) || ((window->Flags & ImGuiWindowFlags_NoScrollWithMouse) && !(window->Flags & ImGuiWindowFlags_NoMouseInputs))))
window = window->ParentWindow;
if (!(window->Flags & ImGuiWindowFlags_NoScrollWithMouse) && !(window->Flags & ImGuiWindowFlags_NoMouseInputs))

Applying this would really simplify the "readability" of the condition tested in the while Loop at line 3874.

I agree that my original proposal may have skip the fact that the "boolean evaluation" (evaluate the expression) should be eval'ed (set) at each run of the loop, so probably something like:

# example of code for Line 3874 (the first : "if (wheel_y != 0.0f)")
bool noScroll;
while (1) { // Line 3874
   noScroll = 
       (window->Flags & ImGuiWindowFlags_NoScrollWithMouse) && !(window->Flags & ImGuiWindowFlags_NoMouseInputs);
     IF ((window->Flags & ImGuiWindowFlags_ChildWindow) && ((window->ScrollMax.y == 0.0f) || (noScroll))) {
        window = window->ParentWindow;
        continue;
     }
     break;
}
if (!noScroll) // line 3876

So above I just :

  • replaced an overly long "set anonymous boolean and test it" in the while loop, by decomposing it in a explicit named boolean before (having a relevant name)
  • replace the "WHILE ... window = parent" with a "assign + IF ... { window = parent ; continue } else break".

This is ugly but it's an example just to show that it's just a "sed replace" of some "text" (and the WHILE loop modified to continue/break to be sure to correctly set the boolean which was previously set "anonymously" in the condition of the loop itself.

This uglyness is easily fixable : just negate the proposed "IF" and make it break if the negated == true, otherwise just "window = parent" and then "naturally" (implicitly) "continue" (by not breaking...) :

# example of code for Line 3874 (the first : "if (wheel_y != 0.0f)")
bool noScroll;
while (1) { // Line 3874
   noScroll = 
       (window->Flags & ImGuiWindowFlags_NoScrollWithMouse) && !(window->Flags & ImGuiWindowFlags_NoMouseInputs);
     IF NEGATION((window->Flags & ImGuiWindowFlags_ChildWindow) && ((window->ScrollMax.y == 0.0f) || (noScroll))) {
        break;
    window = window->ParentWindow;
}
if (!noScroll) // line 3876

(yeah, replace "IF NEGATION" above by "if !", that was for emphasis.)

I think that would reaaally simplify intention of the code at Line 3874 and 3876

imgui/imgui.cpp

Lines 3871 to 3876 in 612b787

if (wheel_y != 0.0f)
{
StartLockWheelingWindow(window);
while ((window->Flags & ImGuiWindowFlags_ChildWindow) && ((window->ScrollMax.y == 0.0f) || ((window->Flags & ImGuiWindowFlags_NoScrollWithMouse) && !(window->Flags & ImGuiWindowFlags_NoMouseInputs))))
window = window->ParentWindow;
if (!(window->Flags & ImGuiWindowFlags_NoScrollWithMouse) && !(window->Flags & ImGuiWindowFlags_NoMouseInputs))

And of course it would also be applied at the same test for X below :

imgui/imgui.cpp

Lines 3888 to 3890 in 612b787

while ((window->Flags & ImGuiWindowFlags_ChildWindow) && ((window->ScrollMax.x == 0.0f) || ((window->Flags & ImGuiWindowFlags_NoScrollWithMouse) && !(window->Flags & ImGuiWindowFlags_NoMouseInputs))))
window = window->ParentWindow;
if (!(window->Flags & ImGuiWindowFlags_NoScrollWithMouse) && !(window->Flags & ImGuiWindowFlags_NoMouseInputs))

This would really for code readability (factoring) of this section, which currently somewhat hampers fixing the code issue of the behavior described in #3795.

3) The bug fix...

...Following up on this one in another comment below..

@folays
Copy link
Author

folays commented Dec 22, 2021

3) The bug fix...

Regarding

imgui/imgui.cpp

Lines 3871 to 3882 in 612b787

if (wheel_y != 0.0f)
{
StartLockWheelingWindow(window);
while ((window->Flags & ImGuiWindowFlags_ChildWindow) && ((window->ScrollMax.y == 0.0f) || ((window->Flags & ImGuiWindowFlags_NoScrollWithMouse) && !(window->Flags & ImGuiWindowFlags_NoMouseInputs))))
window = window->ParentWindow;
if (!(window->Flags & ImGuiWindowFlags_NoScrollWithMouse) && !(window->Flags & ImGuiWindowFlags_NoMouseInputs))
{
float max_step = window->InnerRect.GetHeight() * 0.67f;
float scroll_step = ImFloor(ImMin(5 * window->CalcFontSize(), max_step));
SetScrollY(window, window->Scroll.y - wheel_y * scroll_step);
}
}

It still has the same issue.
As said on #4559, on a MacBook with the internal touchpad, EVEN if you try REALLY HARD to move your finger "only on X", it superhard to do and if you have success, it's only for 10 pixels maximum, then you will inevitably also move your finger "on Y".

As soon as you "inevitably" move your finger on Y, the "wheel locked window" (StartLockWheelingWindow(window)) will bubble up to the first parent having interest for Y.

You will as such be "locked out" of your capability to "scroll on X", and you will recover only if you raise your finger and wait the amount of time "unlocking" the "wheel locked window" as done by ImGui.

Concretely, this is really nearly impossible to scroll on X on a touchpad.
Maybe the hardware/driver of some MacBook have some small differences, but on mine (MBP Pro 13" August 2020) it's really super super hard to scroll on X, that's not usable at all really :(

The Patch

Well, the core of the problem is that the logic is first applied to Y, then to X.

It has some senses when, as @ocornut said, there is "two streams" of independent values, like when you for real have 2 physical scrolls, the human user could be sufficiently intelligent to understand to not scroll both at the same time when this problem occurs.

fdbce6c

My original patch proposal above (fdbce6c), which should be read in "diff mode" of "side-by-side" so as not to go crazy,
"changes" and "merges" the behavior of the 2 loops:

  • BEFORE : If scrollY != 0, Loop on window=parent on some condition, and then do the same on scrollX != 0
  • AFTER (my proposal) : "merge" the 2 loops to figure out the "bubbling up" of the event.

My patch aimed to:

stop bubbling the event as soon as a widget has interest for either scroll-x or scroll-y.

If the widget has interest for only one of the two scrolling axes, the second axe will be ignored, which is probably the better situation. (it will NOT be bubbled up to the parent).
Of course if the widget has interest for BOTH axes, it will correctly receive both events.

I'm not sure that it would be a bug, I think that maybe it's a feature.
Regarding "scrolling two axes both at the same time" (being on 2 reals physicals wheels, or on a touchpad) :
-> I'm not sure any sane user would like to have those two "concurrent" events (at the same time, a scroll both on X and Y) to HAPPEN on TWO DIFFENTS window/widget (selected by the while/if "window = parent").

My patch does not seem far behind "master" and the logic change (merging the 2 loops) seems to be possibly applied easily.

@folays
Copy link
Author

folays commented Dec 22, 2021

I should also add that, as one of the other issues explained, there is a quicker way to test the behavior with the "demo":

  • run the demo
  • do not go for "Tables & Columns". More quicker, go for the "menu" and chose the "Examples" menu then "Log" in the drop-down (the 3rd item).
  • Click to "add 5 entries"
  • resize the window so that there is a scrollbar on X (horizontal) and no scrollbar on Y (it should probably not, since there is only 5 lines...)

On a MBP Pro 13" August 2020, this is nearly impossible to scroll on X.

You somewhat just achieve it by luck with ~10 pixels max at each try, by trying to have a steady hand/finger, and moving your finger only on X, which is superhard.

@folays
Copy link
Author

folays commented Sep 27, 2022

Hi, I updated my branch :)

Please consider merging this request, I beg you.

Scrolling does not work AT ALL in some conditions on MacBooks with touchpad. Since at the very least February 2021 (my original request), but I guess since way more time.

Just try on a Macbook scrollpad the following :

run the demo (I use examples/example_glfw_opengl3)
Run from the menu, the "Examples / Log" item in the drop-down (the 3rd item).
Click to "add 5 entries"
resize the window so that there is a scrollbar on X (horizontal) and NO (VERY important) scrollbar on Y (it should probably not, since there is only 5 lines...)
On a MBP Pro 13" August 2020, this is nearly IMPOSSIBLE (99% difficulty) to scroll on X.

You somewhat just achieve it by luck with ~10 pixels max at each try, by trying to have a steady hand/finger, and moving your finger only on X, which is superhard.

As soon as you add many more "5 entries" up to to the a scrollbar on Y appears, the problem disappear.

My branch (and the following patch) fixes that.

The caveat is both event (scroll Y AND scroll X) will STOP bubbling up as soon as a window has interest / utility.
(and this is probably NOT a caveat, because scrolling BOTH AT THE SAME TIME on TWO DIFFERENT WIDGETS would probably be super weird)

The diff is :

--- a/imgui.cpp
+++ b/imgui.cpp
@@ -4296,27 +4296,23 @@ void ImGui::UpdateMouseWheel()
         wheel_y = 0.0f;
     }

-    // Vertical Mouse Wheel scrolling
-    if (wheel_y != 0.0f)
+    if (wheel_y != 0.0f || wheel_x != 0.0f)
     {
         StartLockWheelingWindow(window);
-        while ((window->Flags & ImGuiWindowFlags_ChildWindow) && ((window->ScrollMax.y == 0.0f) || ((window->Flags & ImGuiWindowFlags_NoScrollWithMouse) && !(window->Flags & ImGuiWindowFlags_NoMouseInputs))))
+        bool noScroll = (window->Flags & ImGuiWindowFlags_NoScrollWithMouse) && !(window->Flags & ImGuiWindowFlags_NoMouseInputs);
+        while ((window->Flags & ImGuiWindowFlags_ChildWindow) && ((window->ScrollMax.y == 0.0f && window->ScrollMax.x == 0.0f) || noScroll))
             window = window->ParentWindow;
-        if (!(window->Flags & ImGuiWindowFlags_NoScrollWithMouse) && !(window->Flags & ImGuiWindowFlags_NoMouseInputs))
+
+        // Vertical Mouse Wheel scrolling
+        if (!noScroll && wheel_y != 0.0f)
         {
             float max_step = window->InnerRect.GetHeight() * 0.67f;
             float scroll_step = ImFloor(ImMin(5 * window->CalcFontSize(), max_step));
             SetScrollY(window, window->Scroll.y - wheel_y * scroll_step);
         }
-    }

-    // Horizontal Mouse Wheel scrolling, or Vertical Mouse Wheel w/ Shift held
-    if (wheel_x != 0.0f)
-    {
-        StartLockWheelingWindow(window);
-        while ((window->Flags & ImGuiWindowFlags_ChildWindow) && ((window->ScrollMax.x == 0.0f) || ((window->Flags & ImGuiWindowFlags_NoScrollWithMouse) && !(window->Flags & ImGuiWindowFlags_NoMouseInputs))))
-            window = window->ParentWindow;
-        if (!(window->Flags & ImGuiWindowFlags_NoScrollWithMouse) && !(window->Flags & ImGuiWindowFlags_NoMouseInputs))
+        // Horizontal Mouse Wheel scrolling, or Vertical Mouse Wheel w/ Shift held
+        if (!noScroll && wheel_x != 0.0f)
         {
             float max_step = window->InnerRect.GetWidth() * 0.67f;
             float scroll_step = ImFloor(ImMin(2 * window->CalcFontSize(), max_step));

But the diff is somewhat un-readable unless in "split view".

The result of applying it is :

    if (swap_axis)
    {
        wheel_x = wheel_y;
        wheel_y = 0.0f;
    }

    // UNCHANGED CODE ABOVE, NEW CODE BELOW...

    if (wheel_y != 0.0f || wheel_x != 0.0f)
    {
        StartLockWheelingWindow(window);
        bool noScroll = (window->Flags & ImGuiWindowFlags_NoScrollWithMouse) && !(window->Flags & ImGuiWindowFlags_NoMouseInputs);
        while ((window->Flags & ImGuiWindowFlags_ChildWindow) && ((window->ScrollMax.y == 0.0f && window->ScrollMax.x == 0.0f) || noScroll))
            window = window->ParentWindow;

        // Vertical Mouse Wheel scrolling
        if (!noScroll && wheel_y != 0.0f)
        {
            float max_step = window->InnerRect.GetHeight() * 0.67f;
            float scroll_step = ImFloor(ImMin(5 * window->CalcFontSize(), max_step));
            SetScrollY(window, window->Scroll.y - wheel_y * scroll_step);
        }

        // Horizontal Mouse Wheel scrolling, or Vertical Mouse Wheel w/ Shift held
        if (!noScroll && wheel_x != 0.0f)
        {
            float max_step = window->InnerRect.GetWidth() * 0.67f;
            float scroll_step = ImFloor(ImMin(2 * window->CalcFontSize(), max_step));
            SetScrollX(window, window->Scroll.x - wheel_x * scroll_step);
        }
    }
}

@ocornut ocornut added the bug label Sep 27, 2022
@ocornut
Copy link
Owner

ocornut commented Sep 27, 2022

Hello,

The PR doesn't address the reported issue that the noScroll was not locked to initial window, which probably always made me consider it as "look again later".

The caveat is both event (scroll Y AND scroll X) will STOP bubbling up as soon as a window has interest / utility.
(and this is probably NOT a caveat, because scrolling BOTH AT THE SAME TIME on TWO DIFFERENT WIDGETS would probably be super weird)

The problem I see presently is that most/many windows will have (ScrollMax.x > 0.0f), which is not very noticeable in most case, and not often taken advantage of, and the change effectively disable the bubbling back to parent when using a regular mouse wheel.

So imagine this situation:

  • Window A has a visible vertical scrollbar
  • Child B (inside Window A) has no visible scrollbar, but (ScrollMax.x > 0.0f) which is common.
ImGui::Begin("Window A");
for (int n = 0; n < 10; n++)
    ImGui::Text("%*s some contents", n, "");
ImGui::BeginChild("Child B", ImVec2(0.0f, 200.0f), true);
ImGui::Text("this is a long line of text. this is a long line of text. this is a long line of text. this is a long line of text. ");
ImGui::EndChild();
for (int n = 0; n < 10; n++)
    ImGui::Text("%*s some contents", n, "");
ImGui::End();

image

With the proposed change, vertical wheeling while over Child B doesn't bubble back Window A anymore.

While it's not as problematic as your current issue, it is problematic and I was hoping this would get resolved.

@ocornut
Copy link
Owner

ocornut commented Sep 27, 2022

Could you add in UpdateMouseWheel()

IMGUI_DEBUG_LOG("MouseWheel X:%.3f Y:%.3f\n", wheel_x, wheel_y);

And record logs of:

  • Trying to do a few vertical swipe.
  • Trying to do a few horizontal swipe.
  • Trying to do a diagonal swipe and round gesture.

I am trying to understand is MacOS does some filtering/axis locking at all.
If you try to do slow or fast vertical/horizontal swipe, do you notice if osx filters things on fast swipes?

When you try to have a steady hand and try to "only move X", well, after some tries, sometimes:
you "successfully" scroll ONLY X on some pixels
then farther the scrolling you do, the fastest you "lose your luck" (your finger move a little "vertically")...
...and so OS X will also emit a Y change, and then the problem exhibits.

OK I now understand another aspect/subtly of this bug which I previously didn't see.
On the frame where suddenly both wheel are != 0.0f, bubbling happen for wheel_y, and that final "bubbled" value of window is used in the wheel_x codepath to call StartLockWheelingWindow() on the window bubbled by wheel_y, which now incorrectly locks again higher up. This is one aspect of this issue. I'm pushing some debugging stuff to look into that.

@folays
Copy link
Author

folays commented Sep 27, 2022

Trying to do a diagonal swipe

# Below a slight diagonal swipe (from top-left to bottom-right)
[00170] MouseWheel X:0.100 Y:0.000
[00171] MouseWheel X:0.400 Y:0.600
[00172] MouseWheel X:0.800 Y:1.100
[00173] MouseWheel X:0.400 Y:0.300
[00174] MouseWheel X:0.500 Y:0.700
[00177] MouseWheel X:0.100 Y:0.100
[00178] MouseWheel X:2.500 Y:3.000
[00179] MouseWheel X:1.700 Y:2.400
[00180] MouseWheel X:2.900 Y:4.300
[00182] MouseWheel X:0.300 Y:0.500
[00183] MouseWheel X:0.800 Y:1.000
[00184] MouseWheel X:0.600 Y:0.800
[00185] MouseWheel X:0.400 Y:0.800
[00186] MouseWheel X:0.400 Y:0.600
[00187] MouseWheel X:0.400 Y:0.500
[00188] MouseWheel X:0.400 Y:0.600
[00189] MouseWheel X:0.300 Y:0.400
[00190] MouseWheel X:0.300 Y:0.400
[00191] MouseWheel X:0.200 Y:0.300
[00192] MouseWheel X:0.200 Y:0.200
[00193] MouseWheel X:0.200 Y:0.200
[00194] MouseWheel X:0.200 Y:0.200
[00195] MouseWheel X:0.100 Y:0.200
[00196] MouseWheel X:0.000 Y:0.100
[00197] MouseWheel X:0.100 Y:0.200
[00202] MouseWheel X:0.100 Y:0.100

# Below a huge diagonal swipe, with force and virtual inertial intended (from top-left to bottom-right)
[00066] MouseWheel X:0.200 Y:0.300
[00067] MouseWheel X:3.500 Y:5.400
[00068] MouseWheel X:15.400 Y:26.300
[00069] MouseWheel X:13.600 Y:22.500
[00070] MouseWheel X:26.800 Y:40.500
[00071] MouseWheel X:26.300 Y:34.700
[00072] MouseWheel X:24.000 Y:30.900
[00073] MouseWheel X:22.300 Y:28.400
[00074] MouseWheel X:20.800 Y:26.400
[00075] MouseWheel X:19.600 Y:24.800
[00076] MouseWheel X:18.400 Y:23.400
[00077] MouseWheel X:16.300 Y:20.400
[00078] MouseWheel X:15.800 Y:19.700
[00079] MouseWheel X:15.300 Y:19.100
[00080] MouseWheel X:14.700 Y:18.500
[00081] MouseWheel X:14.200 Y:17.900
[00082] MouseWheel X:13.700 Y:17.400
[00083] MouseWheel X:13.200 Y:16.700
[00084] MouseWheel X:12.700 Y:16.200
[00085] MouseWheel X:12.100 Y:15.700
[00086] MouseWheel X:11.700 Y:15.200
[00087] MouseWheel X:11.200 Y:14.700
[00088] MouseWheel X:10.800 Y:14.100
[00089] MouseWheel X:10.200 Y:13.600
[00090] MouseWheel X:9.900 Y:13.100
[00091] MouseWheel X:9.300 Y:12.600
[00092] MouseWheel X:8.900 Y:12.200
[00093] MouseWheel X:8.400 Y:11.600
[00094] MouseWheel X:7.900 Y:11.100
[00095] MouseWheel X:7.500 Y:10.600
[00096] MouseWheel X:7.000 Y:10.300
[00097] MouseWheel X:6.600 Y:9.700
[00098] MouseWheel X:6.100 Y:9.300
[00099] MouseWheel X:5.700 Y:8.900
[00100] MouseWheel X:5.300 Y:8.400
[00101] MouseWheel X:4.900 Y:7.900
[00102] MouseWheel X:4.500 Y:7.400
[00103] MouseWheel X:4.300 Y:7.000
[00104] MouseWheel X:3.900 Y:6.400
[00105] MouseWheel X:3.600 Y:6.000
[00106] MouseWheel X:3.300 Y:5.700
[00107] MouseWheel X:3.000 Y:5.300
[00108] MouseWheel X:2.800 Y:4.900
[00109] MouseWheel X:2.600 Y:4.500
[00110] MouseWheel X:2.400 Y:4.100
[00111] MouseWheel X:2.100 Y:3.900
[00112] MouseWheel X:1.900 Y:3.600
[00113] MouseWheel X:1.700 Y:3.300
[00114] MouseWheel X:1.600 Y:3.000
[00115] MouseWheel X:1.400 Y:2.800
[00116] MouseWheel X:1.300 Y:2.500
[00117] MouseWheel X:1.300 Y:2.200
[00118] MouseWheel X:1.100 Y:2.100
[00119] MouseWheel X:1.000 Y:1.900
[00120] MouseWheel X:0.900 Y:1.700
[00121] MouseWheel X:0.900 Y:1.600
[00122] MouseWheel X:0.800 Y:1.400
[00123] MouseWheel X:0.700 Y:1.300
[00124] MouseWheel X:0.700 Y:1.200
[00125] MouseWheel X:0.600 Y:1.100
[00126] MouseWheel X:0.600 Y:1.000
[00127] MouseWheel X:0.500 Y:0.900
[00128] MouseWheel X:0.500 Y:0.900
[00129] MouseWheel X:0.500 Y:0.800
[00130] MouseWheel X:0.400 Y:0.700
[00131] MouseWheel X:0.400 Y:0.700
[00132] MouseWheel X:0.400 Y:0.600
[00133] MouseWheel X:0.400 Y:0.600
[00134] MouseWheel X:0.300 Y:0.500
[00135] MouseWheel X:0.300 Y:0.500
[00136] MouseWheel X:0.300 Y:0.500
[00137] MouseWheel X:0.300 Y:0.400
[00138] MouseWheel X:0.200 Y:0.400
[00139] MouseWheel X:0.200 Y:0.400
[00140] MouseWheel X:0.200 Y:0.400
[00141] MouseWheel X:0.200 Y:0.300
[00142] MouseWheel X:0.200 Y:0.300
[00143] MouseWheel X:0.200 Y:0.300
[00144] MouseWheel X:0.200 Y:0.300
[00145] MouseWheel X:0.200 Y:0.200
[00146] MouseWheel X:0.100 Y:0.200
[00147] MouseWheel X:0.100 Y:0.200
[00148] MouseWheel X:0.100 Y:0.200
[00149] MouseWheel X:0.100 Y:0.200
[00150] MouseWheel X:0.100 Y:0.100
[00151] MouseWheel X:0.100 Y:0.100
[00152] MouseWheel X:0.100 Y:0.100
[00153] MouseWheel X:0.100 Y:0.100
[00154] MouseWheel X:0.100 Y:0.100
[00155] MouseWheel X:0.100 Y:0.100
[00157] MouseWheel X:0.100 Y:0.100
[00158] MouseWheel X:0.000 Y:0.100
[00159] MouseWheel X:0.100 Y:0.000
[00160] MouseWheel X:0.000 Y:0.100

and round gesture.

# below a full clock-around in normal clock direction (going from 00:00 to 03:00 to 06:00 to 09:00 to 12:00 again)
[00084] MouseWheel X:0.200 Y:-0.500
[00085] MouseWheel X:0.200 Y:-0.400
[00086] MouseWheel X:0.400 Y:-0.600
[00087] MouseWheel X:0.400 Y:-0.600
[00088] MouseWheel X:0.600 Y:-0.800
[00089] MouseWheel X:0.200 Y:-0.200
[00090] MouseWheel X:0.400 Y:-0.400
[00091] MouseWheel X:0.200 Y:-0.200
[00092] MouseWheel X:0.400 Y:-0.300
[00093] MouseWheel X:0.400 Y:-0.200
[00094] MouseWheel X:0.400 Y:-0.200
[00095] MouseWheel X:0.800 Y:-0.300
[00096] MouseWheel X:0.500 Y:0.000
[00097] MouseWheel X:0.500 Y:0.000
[00098] MouseWheel X:0.500 Y:0.000
[00100] MouseWheel X:0.600 Y:0.200
[00101] MouseWheel X:0.600 Y:0.200
[00102] MouseWheel X:0.500 Y:0.200
[00103] MouseWheel X:0.800 Y:0.300
[00104] MouseWheel X:0.500 Y:0.200
[00105] MouseWheel X:0.600 Y:0.200
[00106] MouseWheel X:0.600 Y:0.300
[00107] MouseWheel X:0.600 Y:0.400
[00108] MouseWheel X:0.600 Y:0.400
[00109] MouseWheel X:0.600 Y:0.400
[00110] MouseWheel X:0.600 Y:0.400
[00111] MouseWheel X:0.500 Y:0.400
[00112] MouseWheel X:0.400 Y:0.400
[00113] MouseWheel X:0.400 Y:0.500
[00114] MouseWheel X:0.300 Y:0.500
[00115] MouseWheel X:0.300 Y:0.600
[00116] MouseWheel X:0.300 Y:0.600
[00117] MouseWheel X:0.200 Y:0.600
[00118] MouseWheel X:0.200 Y:0.700
[00119] MouseWheel X:0.200 Y:0.800
[00120] MouseWheel X:0.000 Y:0.800
[00121] MouseWheel X:-0.100 Y:1.200
[00122] MouseWheel X:-0.100 Y:0.300
[00123] MouseWheel X:-0.200 Y:0.600
[00124] MouseWheel X:-0.400 Y:0.600
[00125] MouseWheel X:-0.300 Y:0.500
[00126] MouseWheel X:-0.400 Y:0.400
[00127] MouseWheel X:-0.400 Y:0.400
[00128] MouseWheel X:-0.400 Y:0.400
[00129] MouseWheel X:-0.700 Y:0.600
[00130] MouseWheel X:-0.300 Y:0.200
[00131] MouseWheel X:-0.500 Y:0.400
[00132] MouseWheel X:-0.500 Y:0.300
[00133] MouseWheel X:-0.300 Y:0.100
[00134] MouseWheel X:-0.600 Y:0.200
[00135] MouseWheel X:-0.600 Y:0.200
[00136] MouseWheel X:-0.600 Y:0.200
[00137] MouseWheel X:-0.800 Y:0.200
[00138] MouseWheel X:-0.800 Y:0.200
[00139] MouseWheel X:-0.600 Y:0.100
[00140] MouseWheel X:-0.600 Y:0.000
[00141] MouseWheel X:-0.600 Y:-0.200
[00142] MouseWheel X:-0.500 Y:-0.200
[00143] MouseWheel X:-0.500 Y:-0.300
[00144] MouseWheel X:-0.600 Y:-0.400
[00145] MouseWheel X:-0.500 Y:-0.400
[00146] MouseWheel X:-0.400 Y:-0.400
[00147] MouseWheel X:-0.400 Y:-0.400
[00148] MouseWheel X:-0.400 Y:-0.500
[00149] MouseWheel X:-0.400 Y:-0.600
[00150] MouseWheel X:-0.400 Y:-0.600
[00151] MouseWheel X:-0.400 Y:-0.500
[00152] MouseWheel X:-0.400 Y:-0.500
[00153] MouseWheel X:-0.300 Y:-0.600
[00154] MouseWheel X:-0.200 Y:-0.400
[00155] MouseWheel X:-0.400 Y:-0.800
[00156] MouseWheel X:-0.200 Y:-0.600
[00157] MouseWheel X:-0.200 Y:-0.600
[00158] MouseWheel X:-0.200 Y:-0.800
[00159] MouseWheel X:0.000 Y:-0.900
[00160] MouseWheel X:0.000 Y:-0.900
[00161] MouseWheel X:0.000 Y:-0.800
[00162] MouseWheel X:0.100 Y:-0.800
[00163] MouseWheel X:0.200 Y:-0.800
[00164] MouseWheel X:0.200 Y:-0.800
[00165] MouseWheel X:0.200 Y:-0.800
[00166] MouseWheel X:0.300 Y:-0.600
[00167] MouseWheel X:0.300 Y:-0.600
[00168] MouseWheel X:0.400 Y:-0.600
[00169] MouseWheel X:0.400 Y:-0.600
[00170] MouseWheel X:0.400 Y:-0.600
[00171] MouseWheel X:0.400 Y:-0.500
[00172] MouseWheel X:0.400 Y:-0.400
[00173] MouseWheel X:0.600 Y:-0.400
[00174] MouseWheel X:0.600 Y:-0.500
[00175] MouseWheel X:0.600 Y:-0.400
[00176] MouseWheel X:0.500 Y:-0.300
[00177] MouseWheel X:0.500 Y:-0.300
[00178] MouseWheel X:0.600 Y:-0.200
[00179] MouseWheel X:0.500 Y:-0.200
[00180] MouseWheel X:0.600 Y:-0.200
[00181] MouseWheel X:0.400 Y:-0.200
[00182] MouseWheel X:0.400 Y:-0.100
[00183] MouseWheel X:0.400 Y:-0.100
[00184] MouseWheel X:0.600 Y:-0.100
[00185] MouseWheel X:0.500 Y:0.000
[00186] MouseWheel X:0.500 Y:-0.100
[00187] MouseWheel X:0.500 Y:-0.100
[00188] MouseWheel X:0.400 Y:0.000
[00189] MouseWheel X:0.400 Y:0.000
[00190] MouseWheel X:0.400 Y:0.000
[00191] MouseWheel X:0.400 Y:0.000
[00192] MouseWheel X:0.400 Y:0.000
[00193] MouseWheel X:0.400 Y:0.000
[00194] MouseWheel X:0.200 Y:0.200
[00195] MouseWheel X:0.200 Y:0.100
[00196] MouseWheel X:0.200 Y:0.100
[00197] MouseWheel X:0.200 Y:0.000
[00198] MouseWheel X:0.000 Y:0.100
[00200] MouseWheel X:0.000 Y:0.100
[00202] MouseWheel X:0.000 Y:0.100

@ocornut
Copy link
Owner

ocornut commented Sep 27, 2022

Thanks for additional thoughts.

Hint : imgui could MAYBE be missing something : a configuration toggle to tell imgui that both axis are on the SAME physical "wheel" (or on a trackpad) so that imgui could do those heuristics only when needed.
(and maybe the different examples/ could auto-set this flag depending on the hardware...)

Maybe backends could handle that indeed.
Another possibility is that we detect pure "single axis" swipe (which would typically happen with a mouse), so at least we prevent regression with a mouse, while applying your swipe for trackpad but with imperfect bubbling up.

The fact is that the heuristic will inevitably delay the beginning of the events.
Some games will want to deactivate the heuristics.
Maybe the heuristics will have to be dynamically actionable ;
A game would maybe want to disable them when the mouse pointer is over only the "background" (the 3d world view),
and enable heuristics when the mouse pointer is over some child window.

If we end up needing to use an heuristic, the heuristic will be at the level of how/where to decide scrolling based on raw wheel events but should never affect raw wheel events, so I believe this particular thing is not an issue.

(PS: I'm currently installing Xcode on my brother's Macbook hoping to be to be able to test things. Last year I did buy a Trackpad following discussions, but it ended up being terrible at doing any form of scrolling, and definitively doesn't do both axis simultaneously)

@folays
Copy link
Author

folays commented Sep 27, 2022

Okay I found some webpage I tested one year ago to try to figure out some stuff :
Please check https://gromo.github.io/jquery.scrollbar/demo/basic.html

Here are my observations on macOS / Safari :
(I make the window large enough so that there is no scrollbar on X)

  1. "Simple Inner" (one scrollable-Y, inside the webpage)
    If I started to scroll inside, and I continue to keep changing VERY RAPIDLY my scroll-Y direction
    (to top, or to bottom),
    As long as I don't let the trackpad "alone" (without my fingers on it) not too much time,
    it will keep only scrolling INSIDE the "Simple Inner".

IF I'm already at the bottom of the "Simple Inner" AND I left the trackpad alone roughly ~200-300ms,
(both condition), then a further "scroll-Y down" impulse will make the WEBPAGE scroll down.

That's not some advanced heuristics, only that the scroll-Y is bubbled up if were are already at the bottom
of the scrollable children AND that the scrollable children axis has been unlocked after a small timeout.

  1. "Simple Outer" (two scrollables-Y, one inside the other, inside the webpage)
    Same observation than above, translated in the intuitive consequences...
    If we're at the bottom of the 1st scrollable-Y and the timeout occurred (axis unlocked), then the parent will get the event,
    and when the parent also is at the bottom, and after it also get past unlocking, then the webpage will get the event.

NOW, when I shrink Safari on X so that scrollbars appear on X ;
I observe that it's nearly impossible to scroll both on X and on the Y at the same time.
It seems really well done, I did not noticed that before.
It seems that some heuristics will try every 100ms to "elect" the "main" axis of your fingers movement.

To change the "elected" main axis (the only one receiving movement), you :

  • do NOT have to lift you finger from the trackpad
  • do NOT have to pause your movement

In a single, continuous movement, if you were staring to move vertically (and it scrolled only on Y), and then started (in the SAME continuous movement) to now move primarily horizontally, then the scrollable region, after maybe 100-200ms will now stop ALL vertical movement to only do horizontal movement.

I observe the same behaviour with macOS + Chrome with no noticeable difference.
Maybe that's WekKit behaviour and not macOS...

Maybe I should try with a non-WebKit browser this webpage to figure out if this behaviour is WebKit-specific or some macOS per-application configuration...

@folays
Copy link
Author

folays commented Sep 27, 2022

What was the approximate frame rate for the application? (to derive wall-clock time from frame counter, just in case we need it later).

I re-enabled the "demo window" inside the main.cpp.
It display nearly 60 FPS (99.9% of that I think)

Indeed after 10 seconds, there is 600 "MouseWheel" log lines.

fangshun2004 added a commit to fangshun2004/imgui that referenced this pull request Sep 29, 2022
commit e74a50f
Author: Andrew D. Zonenberg <[email protected]>
Date:   Wed Sep 28 08:19:34 2022 -0700

    Added GetGlyphRangesGreek() helper for Greek & Coptic glyph range. (ocornut#5676, ocornut#5727)

commit d17627b
Author: ocornut <[email protected]>
Date:   Wed Sep 28 17:38:41 2022 +0200

    InputText: leave state->Flags uncleared for the purpose of backends emitting an on-screen keyboard for passwords. (ocornut#5724)

commit 0a7054c
Author: ocornut <[email protected]>
Date:   Wed Sep 28 17:00:45 2022 +0200

    Backends: Win32: Convert WM_CHAR values with MultiByteToWideChar() when window class was registered as MBCS (not Unicode). (ocornut#5725, ocornut#1807, ocornut#471, ocornut#2815, ocornut#1060)

commit a229a7f
Author: ocornut <[email protected]>
Date:   Wed Sep 28 16:57:09 2022 +0200

    Examples: Win32: Always use RegisterClassW() to ensure windows are Unicode. (ocornut#5725)

commit e0330c1
Author: ocornut <[email protected]>
Date:   Wed Sep 28 14:54:38 2022 +0200

    Fonts, Text: Fixed wrapped-text not doing a fast-forward on lines above the clipping region. (ocornut#5720)

    which would result in an abnormal number of vertices created.

commit 4d4889b
Author: ocornut <[email protected]>
Date:   Wed Sep 28 12:42:06 2022 +0200

    Refactor CalcWordWrapPositionA() to take on the responsability of minimum character display. Add CalcWordWrapNextLineStartA(), simplify caller code.

    Should be no-op but incrementing IMGUI_VERSION_NUM just in case.
    Preparing for ocornut#5720

commit 5c4426c
Author: ocornut <[email protected]>
Date:   Wed Sep 28 12:22:34 2022 +0200

    Demo: Fixed Log & Console from losing scrolling position with Auto-Scroll when child is clipped. (ocornut#5721)

commit 12c0246
Author: ocornut <[email protected]>
Date:   Wed Sep 28 12:07:43 2022 +0200

    Removed support for 1.42-era IMGUI_DISABLE_INCLUDE_IMCONFIG_H / IMGUI_INCLUDE_IMCONFIG_H. (ocornut#255)

commit 73efcec
Author: ocornut <[email protected]>
Date:   Tue Sep 27 22:21:47 2022 +0200

    Examples: disable GL related warnings on Mac + amend to ignore list.

commit a725db1
Author: ocornut <[email protected]>
Date:   Tue Sep 27 18:47:20 2022 +0200

    Comments for flags discoverability + add to debug log (ocornut#3795, ocornut#4559)

commit 325299f
Author: ocornut <[email protected]>
Date:   Wed Sep 14 15:46:27 2022 +0200

    Backends: OpenGL: Add ability to #define IMGUI_IMPL_OPENGL_DEBUG. (ocornut#4468, ocornut#4825, ocornut#4832, ocornut#5127, ocornut#5655, ocornut#5709)

commit 56c3eae
Author: ocornut <[email protected]>
Date:   Tue Sep 27 14:24:21 2022 +0200

    ImDrawList: asserting on incorrect value for CurveTessellationTol (ocornut#5713)

commit 04316bd
Author: ocornut <[email protected]>
Date:   Mon Sep 26 16:32:09 2022 +0200

    ColorEdit3: fixed id collision leading to an assertion. (ocornut#5707)

commit c261dac
Author: ocornut <[email protected]>
Date:   Mon Sep 26 14:50:46 2022 +0200

    Demo: moved ShowUserGuide() lower in the file, to make main demo entry point more visible + fix using IMGUI_DEBUG_LOG() macros in if/else.

commit 51bbc70
Author: ocornut <[email protected]>
Date:   Mon Sep 26 14:44:26 2022 +0200

    Backends: SDL: Disable SDL 2.0.22 new "auto capture" which prevents drag and drop across windows, and don't capture mouse when drag and dropping. (ocornut#5710)

commit 7a9045d
Author: ocornut <[email protected]>
Date:   Mon Sep 26 11:55:07 2022 +0200

    Backends: WGPU: removed Emscripten version check (currently failing on CI, ensure why, and tbh its redundant/unnecessary with changes of wgpu api nowadays)

commit 83a0030
Author: ocornut <[email protected]>
Date:   Mon Sep 26 10:33:44 2022 +0200

    Added ImGuiMod_Shortcut which is ImGuiMod_Super on Mac and ImGuiMod_Ctrl otherwise. (ocornut#456)

commit fd408c9
Author: ocornut <[email protected]>
Date:   Thu Sep 22 18:58:33 2022 +0200

    Renamed and merged keyboard modifiers key enums and flags into a same set:. ImGuiKey_ModXXX -> ImGuiMod_XXX and ImGuiModFlags_XXX -> ImGuiMod_XXX. (ocornut#4921, ocornut#456)

    Changed signature of GetKeyChordName() to use ImGuiKeyChord.
    Additionally SetActiveIdUsingAllKeyboardKeys() doesn't set ImGuiKey_ModXXX but we never need/use those and the system will be changed in upcoming commits.

# Conflicts:
#	imgui_demo.cpp
fangshun2004 added a commit to fangshun2004/imgui that referenced this pull request Sep 29, 2022
commit cc5058e
Author: ocornut <[email protected]>
Date:   Thu Sep 29 21:59:32 2022 +0200

    IO: Filter duplicate input events during the AddXXX() calls. (ocornut#5599, ocornut#4921)

commit fac8295
Author: ocornut <[email protected]>
Date:   Thu Sep 29 21:31:36 2022 +0200

    IO: remove ImGuiInputEvent::IgnoredAsSame (revert part of 839c310), will filter earlier in next commit. (ocornut#5599)

    Making it a separate commit as this leads to much indentation change.

commit 9e7f460
Author: ocornut <[email protected]>
Date:   Thu Sep 29 20:42:45 2022 +0200

    Fixed GetKeyName() for ImGuiMod_XXX values, made invalid MousePos display in log nicer.  (ocornut#4921, ocornut#456)

    Amend fd408c9

commit 0749453
Author: ocornut <[email protected]>
Date:   Thu Sep 29 19:51:54 2022 +0200

    Menus, Nav: Fixed not being able to close a menu with Left arrow when parent is not a popup. (ocornut#5730)

commit 9f6aae3
Author: ocornut <[email protected]>
Date:   Thu Sep 29 19:48:27 2022 +0200

    Nav: Fixed race condition pressing Esc during popup opening frame causing crash.

commit bd2355a
Author: ocornut <[email protected]>
Date:   Thu Sep 29 19:25:26 2022 +0200

    Menus, Nav: Fixed using left/right navigation when appending to an existing menu (multiple BeginMenu() call with same names). (ocornut#1207)

commit 3532ed1
Author: ocornut <[email protected]>
Date:   Thu Sep 29 18:07:35 2022 +0200

    Menus, Nav: Fixed keyboard/gamepad navigation occasionally erroneously landing on menu-item in parent when the parent is not a popup. (ocornut#5730)

    Replace BeginMenu/MenuItem swapping g.NavWindow with a more adequate ImGuiItemFlags_NoWindowHoverableCheck.
    Expecting more subtle issues to stem from this.
    Note that NoWindowHoverableCheck is not supported by IsItemHovered() but then IsItemHovered() on BeginMenu() never worked: fix should be easy in BeginMenu() + add test is IsItemHovered(), will do later

commit d5d7050
Author: ocornut <[email protected]>
Date:   Thu Sep 29 17:26:52 2022 +0200

    Various comments

    As it turns out, functions like IsItemHovered() won't work on an open BeginMenu() because LastItemData is overriden by BeginPopup(). Probably an easy fix.

commit e74a50f
Author: Andrew D. Zonenberg <[email protected]>
Date:   Wed Sep 28 08:19:34 2022 -0700

    Added GetGlyphRangesGreek() helper for Greek & Coptic glyph range. (ocornut#5676, ocornut#5727)

commit d17627b
Author: ocornut <[email protected]>
Date:   Wed Sep 28 17:38:41 2022 +0200

    InputText: leave state->Flags uncleared for the purpose of backends emitting an on-screen keyboard for passwords. (ocornut#5724)

commit 0a7054c
Author: ocornut <[email protected]>
Date:   Wed Sep 28 17:00:45 2022 +0200

    Backends: Win32: Convert WM_CHAR values with MultiByteToWideChar() when window class was registered as MBCS (not Unicode). (ocornut#5725, ocornut#1807, ocornut#471, ocornut#2815, ocornut#1060)

commit a229a7f
Author: ocornut <[email protected]>
Date:   Wed Sep 28 16:57:09 2022 +0200

    Examples: Win32: Always use RegisterClassW() to ensure windows are Unicode. (ocornut#5725)

commit e0330c1
Author: ocornut <[email protected]>
Date:   Wed Sep 28 14:54:38 2022 +0200

    Fonts, Text: Fixed wrapped-text not doing a fast-forward on lines above the clipping region. (ocornut#5720)

    which would result in an abnormal number of vertices created.

commit 4d4889b
Author: ocornut <[email protected]>
Date:   Wed Sep 28 12:42:06 2022 +0200

    Refactor CalcWordWrapPositionA() to take on the responsability of minimum character display. Add CalcWordWrapNextLineStartA(), simplify caller code.

    Should be no-op but incrementing IMGUI_VERSION_NUM just in case.
    Preparing for ocornut#5720

commit 5c4426c
Author: ocornut <[email protected]>
Date:   Wed Sep 28 12:22:34 2022 +0200

    Demo: Fixed Log & Console from losing scrolling position with Auto-Scroll when child is clipped. (ocornut#5721)

commit 12c0246
Author: ocornut <[email protected]>
Date:   Wed Sep 28 12:07:43 2022 +0200

    Removed support for 1.42-era IMGUI_DISABLE_INCLUDE_IMCONFIG_H / IMGUI_INCLUDE_IMCONFIG_H. (ocornut#255)

commit 73efcec
Author: ocornut <[email protected]>
Date:   Tue Sep 27 22:21:47 2022 +0200

    Examples: disable GL related warnings on Mac + amend to ignore list.

commit a725db1
Author: ocornut <[email protected]>
Date:   Tue Sep 27 18:47:20 2022 +0200

    Comments for flags discoverability + add to debug log (ocornut#3795, ocornut#4559)

commit 325299f
Author: ocornut <[email protected]>
Date:   Wed Sep 14 15:46:27 2022 +0200

    Backends: OpenGL: Add ability to #define IMGUI_IMPL_OPENGL_DEBUG. (ocornut#4468, ocornut#4825, ocornut#4832, ocornut#5127, ocornut#5655, ocornut#5709)

commit 56c3eae
Author: ocornut <[email protected]>
Date:   Tue Sep 27 14:24:21 2022 +0200

    ImDrawList: asserting on incorrect value for CurveTessellationTol (ocornut#5713)

commit 04316bd
Author: ocornut <[email protected]>
Date:   Mon Sep 26 16:32:09 2022 +0200

    ColorEdit3: fixed id collision leading to an assertion. (ocornut#5707)

commit c261dac
Author: ocornut <[email protected]>
Date:   Mon Sep 26 14:50:46 2022 +0200

    Demo: moved ShowUserGuide() lower in the file, to make main demo entry point more visible + fix using IMGUI_DEBUG_LOG() macros in if/else.

commit 51bbc70
Author: ocornut <[email protected]>
Date:   Mon Sep 26 14:44:26 2022 +0200

    Backends: SDL: Disable SDL 2.0.22 new "auto capture" which prevents drag and drop across windows, and don't capture mouse when drag and dropping. (ocornut#5710)

commit 7a9045d
Author: ocornut <[email protected]>
Date:   Mon Sep 26 11:55:07 2022 +0200

    Backends: WGPU: removed Emscripten version check (currently failing on CI, ensure why, and tbh its redundant/unnecessary with changes of wgpu api nowadays)

commit 83a0030
Author: ocornut <[email protected]>
Date:   Mon Sep 26 10:33:44 2022 +0200

    Added ImGuiMod_Shortcut which is ImGuiMod_Super on Mac and ImGuiMod_Ctrl otherwise. (ocornut#456)

commit fd408c9
Author: ocornut <[email protected]>
Date:   Thu Sep 22 18:58:33 2022 +0200

    Renamed and merged keyboard modifiers key enums and flags into a same set:. ImGuiKey_ModXXX -> ImGuiMod_XXX and ImGuiModFlags_XXX -> ImGuiMod_XXX. (ocornut#4921, ocornut#456)

    Changed signature of GetKeyChordName() to use ImGuiKeyChord.
    Additionally SetActiveIdUsingAllKeyboardKeys() doesn't set ImGuiKey_ModXXX but we never need/use those and the system will be changed in upcoming commits.

# Conflicts:
#	imgui.cpp
#	imgui.h
#	imgui_demo.cpp
fangshun2004 added a commit to fangshun2004/imgui that referenced this pull request Sep 30, 2022
commit 5884219
Author: cfillion <[email protected]>
Date:   Wed Sep 28 23:37:39 2022 -0400

    imgui_freetype: Assert if bitmap size exceed chunk size to avoid buffer overflow. (ocornut#5731)

commit f2a522d
Author: ocornut <[email protected]>
Date:   Fri Sep 30 15:43:27 2022 +0200

    ImDrawList: Not using alloca() anymore, lift single polygon size limits. (ocornut#5704, ocornut#1811)

commit cc5058e
Author: ocornut <[email protected]>
Date:   Thu Sep 29 21:59:32 2022 +0200

    IO: Filter duplicate input events during the AddXXX() calls. (ocornut#5599, ocornut#4921)

commit fac8295
Author: ocornut <[email protected]>
Date:   Thu Sep 29 21:31:36 2022 +0200

    IO: remove ImGuiInputEvent::IgnoredAsSame (revert part of 839c310), will filter earlier in next commit. (ocornut#5599)

    Making it a separate commit as this leads to much indentation change.

commit 9e7f460
Author: ocornut <[email protected]>
Date:   Thu Sep 29 20:42:45 2022 +0200

    Fixed GetKeyName() for ImGuiMod_XXX values, made invalid MousePos display in log nicer.  (ocornut#4921, ocornut#456)

    Amend fd408c9

commit 0749453
Author: ocornut <[email protected]>
Date:   Thu Sep 29 19:51:54 2022 +0200

    Menus, Nav: Fixed not being able to close a menu with Left arrow when parent is not a popup. (ocornut#5730)

commit 9f6aae3
Author: ocornut <[email protected]>
Date:   Thu Sep 29 19:48:27 2022 +0200

    Nav: Fixed race condition pressing Esc during popup opening frame causing crash.

commit bd2355a
Author: ocornut <[email protected]>
Date:   Thu Sep 29 19:25:26 2022 +0200

    Menus, Nav: Fixed using left/right navigation when appending to an existing menu (multiple BeginMenu() call with same names). (ocornut#1207)

commit 3532ed1
Author: ocornut <[email protected]>
Date:   Thu Sep 29 18:07:35 2022 +0200

    Menus, Nav: Fixed keyboard/gamepad navigation occasionally erroneously landing on menu-item in parent when the parent is not a popup. (ocornut#5730)

    Replace BeginMenu/MenuItem swapping g.NavWindow with a more adequate ImGuiItemFlags_NoWindowHoverableCheck.
    Expecting more subtle issues to stem from this.
    Note that NoWindowHoverableCheck is not supported by IsItemHovered() but then IsItemHovered() on BeginMenu() never worked: fix should be easy in BeginMenu() + add test is IsItemHovered(), will do later

commit d5d7050
Author: ocornut <[email protected]>
Date:   Thu Sep 29 17:26:52 2022 +0200

    Various comments

    As it turns out, functions like IsItemHovered() won't work on an open BeginMenu() because LastItemData is overriden by BeginPopup(). Probably an easy fix.

commit e74a50f
Author: Andrew D. Zonenberg <[email protected]>
Date:   Wed Sep 28 08:19:34 2022 -0700

    Added GetGlyphRangesGreek() helper for Greek & Coptic glyph range. (ocornut#5676, ocornut#5727)

commit d17627b
Author: ocornut <[email protected]>
Date:   Wed Sep 28 17:38:41 2022 +0200

    InputText: leave state->Flags uncleared for the purpose of backends emitting an on-screen keyboard for passwords. (ocornut#5724)

commit 0a7054c
Author: ocornut <[email protected]>
Date:   Wed Sep 28 17:00:45 2022 +0200

    Backends: Win32: Convert WM_CHAR values with MultiByteToWideChar() when window class was registered as MBCS (not Unicode). (ocornut#5725, ocornut#1807, ocornut#471, ocornut#2815, ocornut#1060)

commit a229a7f
Author: ocornut <[email protected]>
Date:   Wed Sep 28 16:57:09 2022 +0200

    Examples: Win32: Always use RegisterClassW() to ensure windows are Unicode. (ocornut#5725)

commit e0330c1
Author: ocornut <[email protected]>
Date:   Wed Sep 28 14:54:38 2022 +0200

    Fonts, Text: Fixed wrapped-text not doing a fast-forward on lines above the clipping region. (ocornut#5720)

    which would result in an abnormal number of vertices created.

commit 4d4889b
Author: ocornut <[email protected]>
Date:   Wed Sep 28 12:42:06 2022 +0200

    Refactor CalcWordWrapPositionA() to take on the responsability of minimum character display. Add CalcWordWrapNextLineStartA(), simplify caller code.

    Should be no-op but incrementing IMGUI_VERSION_NUM just in case.
    Preparing for ocornut#5720

commit 5c4426c
Author: ocornut <[email protected]>
Date:   Wed Sep 28 12:22:34 2022 +0200

    Demo: Fixed Log & Console from losing scrolling position with Auto-Scroll when child is clipped. (ocornut#5721)

commit 12c0246
Author: ocornut <[email protected]>
Date:   Wed Sep 28 12:07:43 2022 +0200

    Removed support for 1.42-era IMGUI_DISABLE_INCLUDE_IMCONFIG_H / IMGUI_INCLUDE_IMCONFIG_H. (ocornut#255)

commit 73efcec
Author: ocornut <[email protected]>
Date:   Tue Sep 27 22:21:47 2022 +0200

    Examples: disable GL related warnings on Mac + amend to ignore list.

commit a725db1
Author: ocornut <[email protected]>
Date:   Tue Sep 27 18:47:20 2022 +0200

    Comments for flags discoverability + add to debug log (ocornut#3795, ocornut#4559)

commit 325299f
Author: ocornut <[email protected]>
Date:   Wed Sep 14 15:46:27 2022 +0200

    Backends: OpenGL: Add ability to #define IMGUI_IMPL_OPENGL_DEBUG. (ocornut#4468, ocornut#4825, ocornut#4832, ocornut#5127, ocornut#5655, ocornut#5709)

commit 56c3eae
Author: ocornut <[email protected]>
Date:   Tue Sep 27 14:24:21 2022 +0200

    ImDrawList: asserting on incorrect value for CurveTessellationTol (ocornut#5713)

commit 04316bd
Author: ocornut <[email protected]>
Date:   Mon Sep 26 16:32:09 2022 +0200

    ColorEdit3: fixed id collision leading to an assertion. (ocornut#5707)

commit c261dac
Author: ocornut <[email protected]>
Date:   Mon Sep 26 14:50:46 2022 +0200

    Demo: moved ShowUserGuide() lower in the file, to make main demo entry point more visible + fix using IMGUI_DEBUG_LOG() macros in if/else.

commit 51bbc70
Author: ocornut <[email protected]>
Date:   Mon Sep 26 14:44:26 2022 +0200

    Backends: SDL: Disable SDL 2.0.22 new "auto capture" which prevents drag and drop across windows, and don't capture mouse when drag and dropping. (ocornut#5710)

commit 7a9045d
Author: ocornut <[email protected]>
Date:   Mon Sep 26 11:55:07 2022 +0200

    Backends: WGPU: removed Emscripten version check (currently failing on CI, ensure why, and tbh its redundant/unnecessary with changes of wgpu api nowadays)

commit 83a0030
Author: ocornut <[email protected]>
Date:   Mon Sep 26 10:33:44 2022 +0200

    Added ImGuiMod_Shortcut which is ImGuiMod_Super on Mac and ImGuiMod_Ctrl otherwise. (ocornut#456)

commit fd408c9
Author: ocornut <[email protected]>
Date:   Thu Sep 22 18:58:33 2022 +0200

    Renamed and merged keyboard modifiers key enums and flags into a same set:. ImGuiKey_ModXXX -> ImGuiMod_XXX and ImGuiModFlags_XXX -> ImGuiMod_XXX. (ocornut#4921, ocornut#456)

    Changed signature of GetKeyChordName() to use ImGuiKeyChord.
    Additionally SetActiveIdUsingAllKeyboardKeys() doesn't set ImGuiKey_ModXXX but we never need/use those and the system will be changed in upcoming commits.

# Conflicts:
#	docs/CHANGELOG.txt
#	imgui.h
#	imgui_demo.cpp
ocornut added a commit that referenced this pull request Oct 4, 2022
… also gets reset whenever scrolling again (#2604) + small refactor

Somehow interesting for (#3795, #4559). sorry this will break PR for 3795 but we got the info.
ocornut added a commit that referenced this pull request Oct 4, 2022
…ly from touch pad events) are incorrectly locking scrolling in a parent window. (#4559, #3795, #2604)
@ocornut
Copy link
Owner

ocornut commented Oct 4, 2022

Sorry for late reply and thanks for the careful investigation and details.

Maybe I should try with a non-WebKit browser this webpage to figure out if this behaviour is WebKit-specific or some macOS per-application configuration...

Any luck with that?

OK I now understand another aspect/subtly of this bug which I previously didn't see.
On the frame where suddenly both wheel are != 0.0f, bubbling happen for wheel_y, and that final "bubbled" value of window is used in the wheel_x codepath to call StartLockWheelingWindow() on the window bubbled by wheel_y, which now incorrectly locks again higher up. This is one aspect of this issue. I'm pushing some debugging stuff to look into that.

This part of fixed with 80a870a (slight refactor) + c7d3d22 (mitigation/fix for #4559), but this main issue remains totally open. Unfortunately PR as this will conflict (rebase/fix if you want to further investigate, but not strictly needed).

Also unfortunately, this issue is tricky enough it's not easy to get back to it after some time.

IF I'm already at the bottom of the "Simple Inner" AND I left the trackpad alone roughly ~200-300ms,
(both condition), then a further "scroll-Y down" impulse will make the WEBPAGE scroll down.

(1) I tweaked the logic in 80a870a. Notice it's not only a timer change: previously timer was not reset on further wheeling (which seemed very weird) now it is reset. TBH maybe now that it is reset on wheeling we should lower down that value much more (intuitively maybe 0.5f~0.8f may be better).

(2) Bubbling-up for same axis (when already at min/max) is something we should add indeed. Not the core of this issue but would be good. Can be developed separately.


Based on the discussions and data points above here's what I think we should do:

- WindowA
  - ChildB (hovered)

When needing to determine locked window for wheeling.

  • (1) do a bubble-up scan to check the first window which could use ScrollX and ScrollY.
  • (2) if only one is claimed, or both are claimed by the same window -> we have no problem.
  • (3) select closest axis (one used by ChildB). if the other axis value if 0.0f we can lock on ChildB > no lag. based on your logs/samples I suspect we'll have no false positive. (the issue would be e.g. user expecting to do a Y scroll, and Frame N has X != 0.0f, Y = 0.0f. If we later have reports of this we can decide to skip step 3 - see 3B - but honestly I expect trackpad drivers to have a say before we even have access to that data, and would prevent that).
  • (4). if we couldn't lock on step 3, prevent scrolling on Frame N+0. On Frame N+1 calculate the movement magnitude for both axises for both frames, select best axis and lock from here.

Once locked:

  • (5) if same window wants both axis: implement a heuristic to update "main" axis. e.g. update two sets of compact moving average of magnitude over 100 ms. except we probably want to consider supporting a "clearly both axis" mode based on a moving average of a longer period that needs an initial unlocking. So that heuristic would have 3 output: axis X, axis Y, axis XY.

Additional heuristic to detect mouse type:

  • (3.B) As pointed out, majority of mouse will never have this issues. If we implement Step 3 as is, regular mouses will never have a frame delay. However if we end up needing to disable 3 due to false positive reports, we should develop an additional heuristic to guess if the user is on trackpad device that emits dual axis. I don't want to create logic/heuristic that will forever "lock" the system to either as some user may be alternating between both, but as long as we develop a heuristic that merely "hitches" at the time of switching devices we good.

Same axis bubbling-up

  • (6) As pointed above we can also bubble-up on same axis when reaching min/max, that's merely an alteration of (1) that we can develop and test before or after all other points.

What do you think?

Going to start writing a little test bed for this (I'll have to write it blind as I don't have access to the Mac right now).

@ocornut
Copy link
Owner

ocornut commented Oct 6, 2022

WIP branch
https://github.com/ocornut/imgui/tree/features/mouse_wheeling_target_3795

Will update branch over time but first push has:

  • Shallow refactor (to simplify diff of second commits) + timer tweak 4529abf
  • Main work a1fad4d

This aims to implement (1) (2) (3) (4) but not well tested yet.
There's a commented line which should implement (6) but not tested at all.
I think (5) is quite desirable otherwise trackpad will often perform accidental X scroll. The commit adds a unused ImExponentialMovingAverage() helper which will be helpful for (5).

ocornut added a commit that referenced this pull request Oct 6, 2022
… misc refactor (#2604, #3795, #4559)

The refactor are designed to minimize the diff for changes needed for #3795
@ocornut
Copy link
Owner

ocornut commented Oct 6, 2022

I reworked this as fe07694 (same branch), it now handles selecting a main axis.

  • I guess it'll work well for "intending a single axis"
  • But changing axis while scrolling is not tuned and currently depends on a frame count (should use a wallclock time thing?)
  • But essentially disable 2D scrolling

Would appreciate test impressions and perhaps a review. I haven't run that on a Mac yet.

@ocornut
Copy link
Owner

ocornut commented Oct 6, 2022

I worry that a long trail of small/inertia mouse-wheel events may unnecessary prevent changing axis when the axis change leads to a target window change, so may need to rework that. If the axis change is on a same window it'll however work fast.

Unfortunately that means another refactor..

@folays
Copy link
Author

folays commented Oct 23, 2022

Hello, thanks for all your work, and I'm really sorry to not have answered quicker :(

I tested against upstream/features/mouse_wheeling_target_3795 c41f267

  1. The WheelingWindowWheelRemainder seems to do its job, or at least does not introduce unwanted behavior.
    Even when I'm trying to be "ambiguous" / "false pretend", I have the impression that your code choose the good axis.

    • In FindBestWheelingWindow() where you're writing "Subsequent frames:", (plural form) maybe replace ? :
      (before__) : .WheelingWindowWheelRemainder = wheel;
      (proposal) : .WheelingWindowWheelRemainder += wheel; // ACCUMULATE instead of replacing ?
  2. //// - a child window doesn't need scrolling because it is already at the edge for the direction we are going in (FIXME-WIP) : indeed it does not work since it Not Yet Implemented

  3. scrolling on both axis at the same time (inside one unique child), when it was clearly the intention :
    Not Yet Implemented as you said. Real probable usage sensical cases :

    • In a "text" child : of course it's probably NOT wanted because it's confusing and not readable pleasurably.
    • In a "image" child : (like a in "image viewer / editor ") it could be usefull. Your eyes would be okay in scrolling both axis.
  4. Timer :

    • 0.8s of locking the axes seems way too much. I must now go, but I will do some tests of seemlingly acceptable timeouts.
    • Trashed inertia : When you scroll quickly with the touchpad and "lift" your fingers, the "virtual" wheel still has inertial, which should probably NOT "reset" the timer to full 0.8s WHEN the "inertial" did NOT have any visible effect (scrolling past limit) :
      because when an user did scroll, if the inertia "continues" to ... "non"-scrolling, then the user probably expects a quick "scrolling on another axe" to execute. which will not if the previous axis was still locked.

Regarding (2) the "edge direction WIP"/ "scrolling_past_limits" :

  • are you sure you would want to activate it for every one ?
  • Or should it be _Window_Flags_Enablable ?
    • An user could be "prison-locked" inside a child window, if a child window were to disable it on both axis AND the child "displayed" X/Y area exceeded whole parent display area.
      (if whole displayed surface of the parent is used by the child, and the user cannot "get out" of the child if the child scrolling event "exceeding limits" does not bubble up to the parent, so that the parent cannot scroll the child out...)

Kind Regards,

8<---8<---8<---

BTW it seems that you "adopted" the terminology of naming things "bubbling" / "bubble up".
That's not really mine and I reused from other GUI frameworks, but if you find it cool, you can name :

  • sinking phase : when you find the deepest child
  • bubbling phase : when you "bubble up" to find which parent is interested in the event

BramblePie pushed a commit to BramblePie/imgui that referenced this pull request Oct 26, 2022
… also gets reset whenever scrolling again (ocornut#2604) + small refactor

Somehow interesting for (ocornut#3795, ocornut#4559). sorry this will break PR for 3795 but we got the info.
BramblePie pushed a commit to BramblePie/imgui that referenced this pull request Oct 26, 2022
…ly from touch pad events) are incorrectly locking scrolling in a parent window. (ocornut#4559, ocornut#3795, ocornut#2604)
BramblePie pushed a commit to BramblePie/imgui that referenced this pull request Oct 26, 2022
… misc refactor (ocornut#2604, ocornut#3795, ocornut#4559)

The refactor are designed to minimize the diff for changes needed for ocornut#3795
@ocornut
Copy link
Owner

ocornut commented Nov 15, 2022

(1)

0.8s of locking the axes seems way too much. I must now go, but I will do some tests of seemlingly acceptable timeouts.

The same timer is currently used to lock the window itself (in a situation where e.g. you are vertically scrolling in parent and temporarily have mouse over a child which could accept vertical scrolling). I don't think that timer can be reduced to significantly lower amount (e.g. 0.2 seconds).

  • Ideally we can refactor to have one timer for "same axis lock" and another timer for "axis change lock".

(2)

Trashed inertia : When you scroll quickly with the touchpad and "lift" your fingers, the "virtual" wheel still has inertial, which should probably NOT "reset" the timer to full 0.8s WHEN the "inertial" did NOT have any visible effect (scrolling past limit) :
because when an user did scroll, if the inertia "continues" to ... "non"-scrolling, then the user probably expects a quick "scrolling on another axe" to execute. which will not if the previous axis was still locked.

What I currently did was to modulate the time increase for small (sub-pixel) wheel amounts: (committed a582d92)

static void LockWheelingWindow(ImGuiWindow* window, float wheel_amount)
{
    ImGuiContext& g = *GImGui;
    if (window)
        g.WheelingWindowReleaseTimer = ImMin(g.WheelingWindowReleaseTimer + ImAbs(wheel_amount) * WINDOWS_MOUSE_WHEEL_SCROLL_LOCK_TIMER, WINDOWS_MOUSE_WHEEL_SCROLL_LOCK_TIMER);
    else
        g.WheelingWindowReleaseTimer = NULL

The axis is still locked during that inertia but this avoid the extra +WINDOWS_MOUSE_WHEEL_SCROLL_LOCK_TIMER delay.

  • I think this needs to be reworked more to allow an axis-change to unlock earlier, which is consistent with the need in (1) to have two timers.
  • Or simpler, gating the logic on an arbitrary threshold may lower the amount of inertia, barely noticeable for user but I suppose we can halve the time this way.

(3) I initially reworked the per-axis average to not be framerate dependent, changing:

// Maintain a rough average of moving magnitude on both axises
// FIXME: should by based on wall clock time rather than frame-counter
g.WheelingAxisAvg.x = ImExponentialMovingAverage(g.WheelingAxisAvg.x, ImAbs(wheel.x), 30);
g.WheelingAxisAvg.y = ImExponentialMovingAverage(g.WheelingAxisAvg.y, ImAbs(wheel.y), 30);

To a rough equivalent:

// Maintain a rough average of moving magnitude on both axises
g.WheelingAxisAvgAccum += ImVec2(ImAbs(wheel.x), ImAbs(wheel.y));
g.WheelingAxisAvgAccumTimer += g.IO.DeltaTime;
while (g.WheelingAxisAvgAccumTimer >= 1.0f / 30.0f)
{
    g.WheelingAxisAvg.x = ImExponentialMovingAverage(g.WheelingAxisAvg.x, g.WheelingAxisAvgAccum.x, 15);
    g.WheelingAxisAvg.y = ImExponentialMovingAverage(g.WheelingAxisAvg.y, g.WheelingAxisAvgAccum.y, 15);
    g.WheelingAxisAvgAccumTimer -= 1.0f / 30.0f;
    g.WheelingAxisAvgAccum = ImVec2(0.0f, 0.0f);
}

Since the average are only compared to each other the exact delay doesn't matter.

However I realized this was wrong as our expectation is to use the average for Frame N+1 comparison.

Then I realized the whole Frame N+1 idea described in #3795 (comment) and implemented in the branch is incorrect, as on very fast / unthrottled framerates it is unlikely the OS would submit wheel events at same rate.

  • Instead we should condition the average-compare test (return (g.WheelingAxisAvg.x > g.WheelingAxisAvg.y) ? windows[0] : windows[1];) to start on 1) the second wheel event or 2) a short (e.g. 1/30) time threshold from the first wheel event, whichever happen sooner.

(4)

Regarding "edge direction WIP"/ "scrolling_past_limits" :
are you sure you would want to activate it for every one ?

  • I'm not and I didn't even attempt to activate it yet. I think we can evaluate this later.

I hoped to finish this for 1.89 but unfortunately it's becoming too risky to commit before tagging, but hope to get back to this asap.

ocornut added a commit that referenced this pull request Nov 15, 2022
@ocornut ocornut added this to the v1.90 milestone Nov 15, 2022
ocornut added a commit that referenced this pull request Nov 30, 2022
… nested windows and backend/OS is emitting dual-axis wheeling inputs. (#3795, #4559)
@ocornut
Copy link
Owner

ocornut commented Nov 30, 2022

I have pushed 87caf27 already which is essentially the version discussed in my last message.
I believe there are many improvements to do (mentioned above) but the current version being a definitive improvement I am pushing it already, keeping this open.

@ocornut
Copy link
Owner

ocornut commented Nov 30, 2022

Replying to myself:

(3)

Then I realized the whole Frame N+1 idea described in #3795 (comment) and implemented in the branch is incorrect, as on very fast / unthrottled framerates it is unlikely the OS would submit wheel events at same rate.

It may not be a problem since in all the "ambiguous" data sets above (= both axises non-zero on frame 0) the next event (frame 1 in your data set) confirms the major axis of frame 0. So assuming Frame 1 event appears on Frame 10 with high-framerate, it may not be much of a problem. But this yet has to be tested properly at very high frame-rate.


(4.B) Another thing, while doing some casual testing on Mac one noticeable issue was, with this setup:

Window A (Y scroll + minor amount of X Scroll)
 - Child B (X Scroll)
  • Start with an Y Scroll,
  • Switch to an X Scroll on the same Window A.
  • Soon hitting the edge of X Scroll.
  • But at this point every further X Wheel input is increasing the timer, which stays locked on Window A.

Here we'd like to eventually start scrolling Child B. It is a sort of "opposite" situation as point (4), going from Parent to Child instead of Child to Parent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants