Mouse coordinate bug on Mac?

General JUCE discussion
1

Hi all,

:skull_and_crossbones: I think I’ve found a mouse bug on Mac (Monterey 12.3.1, JUCE 6.1.6). It’s based on the following assumption:

Under normal circumstances, the MouseEvent parameter passed into mouseEnter(), mouseMove(), mouseDown(), mouseUp() and mouseDrag() should always contain mouse coordinates that fall inside the target components bounds.

I realize there will be exceptions (eg. it might not apply to mouseExit(), and unexpected things can happen if you misuse mouse listeners), but as a general rule, you wouldn’t expect your components mouseMove() handler to fire unless the mouse was actually inside it!

Well, assuming that’s correct, I’m seeing mouse methods getting called with MouseEvent coordinates that fall 1 pixel outside of the component bounds.

It happens reliably for me, but only on Mac. FWIW, I’m using the same Logitech mouse to test on Mac and Windows.

If I understand correctly, it boils down to the fact that JUCE uses float mouse coordinates internally. On Windows, the x/y values are always whole numbers but on Mac they’re always decimals. So, while you might see x coordinates of 123.456 and 654.321 on Mac, they’d just be 123.000 and 654.000 on Windows.

That’s probably expected due to OS differences, but it leads to a rounding error in the MouseInputSourceInternal::sendMouse___() methods which can result in ‘off-by-one’ MouseEvent coordinates. Subtle, but potentially nasty.

For us, it manifests as unexpected behavior and asserts in our UI code, and it happens surprisingly often.

You can test it in DemoRunner. Add the following assert in juce_Component.cpp line 2642 (JUCE 6.1.6), just before the call to mouseMove() in Component::internalMouseMove():

jassert (getLocalBounds().contains (me.x, me.y));

Obviously, the assert simply checks that the MouseEvent x/y coordinates are inside the component. Build it, fire it up, then move the mouse around the UI and wait for it to fire. It happens quite a lot.

A prompt investigation and fix would be very much appreciated :slight_smile:

Many thanks,
Ben

2

An example of a bug caused by this:

We have a component that has a resizable border courtesy of juce::ResizableBorderComponent. When moving the mouse over the right border, there’s a 1 pixel wide zone where the ResizableBorderComponent gets mouse events but doesn’t know what to do because ResizableBorderComponent::Zone::fromPositionOnBorder() thinks the mouse is 1 pixel outside of its border!

Might sound harmless, but it resulted in unexpected UI behavior elsewhere in our app. And we’ve been seeing subtle, hard to reproduce, Mac only mouse issues like that all over our UI for some time, many of which might be explained by this bug.

4 Likes
3

Just giving this a cheeky bump since it seems like a legit JUCE bug, and results in different behavior on Mac and Win :pray:

1 Like
4

Thanks @benstaton for finding this, it’s the cause of a subtle misbehaviour that I’ve started observing after updating a project from an older JUCE to a more recent one.

I have created a minimal example for the JUCE team to reproduce the issue: simply create a default GUI app with Projucer, then replace the MainComponent.h and .cpp files with those attached here.

The problem is shown in the linked video.
You’ll get a square Label showing in the window: slowly moving the mouse cursor over it from left to right is fine, but when the motion is in the opposite direction, from right to left, the following issues happen:

  • when the mouseEnter event is triggered, its mouse coordinate is still not contained in the Label
  • when the mouseExit event is triggered, its mouse coordinate is still contained in the Label.

Both these contraditory situations are notified with a DBG text that appears in the Xcode console (the coordinate values are high because they are global screen coordinates, not relative to window topleft).

For simplicity, the attached reproduction code only deals with coordinates on the x axis, but I have verified that the same also happens on the y axis: moving from top downward there are no issues, moving from the bottom upward, the same two contradictory events are generated.

ezgif-3-529daf8f5e

MainComponent.cpp (2.1 KB)
MainComponent.h (976 Bytes)

5 Likes
5

Just wanted to show my appreciation for the work you’ve both done here! These little things add up and matter a lot to whether interaction “feels good” in an app.

2 Likes
6

Ah, I forgot to mention that I git bisect-ed and apparently the commit that introduced the issue is the following, but I couldn’t find the actual culprit in there:

2 Likes
7

Thank you for thoroughly looking into this @yfede, and for the excellent repro example.

Looking carefully at the framework code, my impression now is that the calculations seem to be correct, and what we are seeing here is an artifact of having sub-pixel resolution events on MacOS.

Using the same operations on the float version equivalents

const auto xRange = comp.getScreenBounds().toFloat().getHorizontalRange();
const auto xCoord = comp.localPointToGlobal(e.position).getX();

the contradicting situations go away.

I think that the best approach to resolving these issues is to rely on the float variable MouseEvent::position instead.

8

Thanks for looking into this.

The proposed workaround solves the problem for the time being, but honestly it feels like a bit of a hack to circumvent the consistency problem that has been introduced in JUCE for int coordinates.

In its own way, it’s a breaking change that could subtly change the behavior of existing code (like it did in my case), and I believe the optimal solution would be that it gets fixed in JUCE so that even the old code yields the expected result as it did before.

Perhaps the solution is to make MouseEvent obtain its int coordinates by truncating the float ones, instead of rounding them? I haven’t got the time now to test that unfortunately.

9

I appreciate that this is causing issues in existing codebases, but I feel that the current approach is the consistent one.

Since MacOS devices often use scaling where physical pixels are smaller than one logical pixel, events will occur on scales that are smaller too. The only non-hacky way to handle this is to use floating point positions.

The integer positions seem to be correctly rounded from the floating point values. Unfortunately the rounding means loss of information and there is no way to work around that without having corner cases.

  • If we round, we are going to have contradictions in the examples you provided.
  • If we ceil or floor, then we are going to have contradictions in half the cases (e.g. on the left end upper edge of objects).
  • We could use integer arithmetics to determine whether an event occurred (I think this is what we did before this change), but then situations could occur where the mouse already entered an area by half a logical pixel, which might even be visible on the screen, but the event hasn’t been fired yet due to the loss of information caused by rounding the value too early.

Considering these it generally seems like a good idea to use the floating point positions.

10

@attila Thank you for responding to my post, and to @yfede for following up with a test project.

@attila I think the underlying issue is being overlooked here. Are you saying it’s OK for a component’s mouseMove()/mouseDrag()/mouseDown() etc methods to get called when the mouse is not inside the component?

Or, to look at it another way, is it OK that the component directly under the mouse is not receiving mouse events in this case?

Or that JUCE mouse behavior differs significantly in this regard between Mac and Windows?

Must all JUCE developers add extra checks to their mouse methods to ensure that the mouse really is inside the component before doing anything? Surely that can’t be right?!

The suggested workaround misses the point IMO. Sure, you can fix @yfede’s specific test project with it, but it (a) ignores the underlying issues, and the fact that it appears to be a regression, (b) relies on developers knowing about this gotcha in the first place, and then (c) remembering to add code to check for it in the mouse methods of all of their components!

This definitely warrants an internal JUCE fix in my opinion. I’d be grateful for your thoughts on this.

Thanks again,
Ben

4 Likes
11

FWIW, our current workaround is in juce_Mac_NSViewComponentPeer.mm, starting at line 1463:

    static Point<float> getMousePos (NSEvent* e, NSView* view)
    {
        NSPoint p = [view convertPoint: [e locationInWindow] fromView: nil];
#if 1
        // TODO: Temporary workaround for Mac mouse coordinate bug:
        return { round( (float)p.x ), round( (float)p.y ) };
#else
        // Original JUCE code:
        return { (float) p.x, (float) p.y };
#endif
    }

Not suggesting that’s the proper way to fix it. Indeed, it simply rounds away the decimal portion of each coordinate! That said, I’ve yet to figure out an actual use for decimal mouse coordinates, and isn’t it the job of a cross platform library to abstract such OS specific inconsistencies away anyway?

It seems to work well and brings Mac mouse behavior back into line with Windows. Haven’t noticed any regressions yet, and we’ve been running with it for a couple of weeks now.

Would still like to see an official fix though :pray:

Edit: To clarify, I’d prefer a fix that retains the decimal coordinates but hides any inconsistencies from the developer somehow. I just can’t spare any more time to look at it.

12

Hi Ben, thanks for bearing with me. For the time being a workaround may be possible after all. The one I’m hopeful for requires the modification of two lines in juce_MouseEvent.cpp. They are changing the initialization of the x and y members of MouseEvent at around line 43.

  x ((int) std::floor (pos.x)),
  y ((int) std::floor (pos.y)),

This stands a chance of becoming the official fix, so please see if it works for you.

This is going to work as long as we specify Component coordinates using integers, but eventually those may become floats too and then the only working approach will be using MouseEvent::position.

1 Like
13

Don’t want to hijack this thread (glad it was a straightforward potential fix!) but just want to say this has my ears perked up, this would be a great move!

It would get rid of a pain point for newcomers, resolve some edge case complexity, but also we’d be able to smoothly animate component position and size! (Right now component animation is jerky due to discrete integer values, but can be mitigated via affine transforms). Go JUCE team, go! :sparkles:

1 Like
14

Awesome! Thank you for reconsidering and looking into it.

I’ll replace our workaround with yours and let you know how it goes…

15

@attila Unfortunately, it didn’t work.

See my 2nd post above, where I give an example of a ResizableBorderComponent bug caused by this.

In that example, there was a 1px zone just after the right-hand resizable border where unexpected behavior happens.

With this new fix, there’s still a 1px zone with unexpected behavior, but now it’s just before the right-hand border.

IOW, it’s just shifted the problem from the right edge of the resizable border to the left edge.

Again, not saying my workaround is the right fix, but FWIW it seems to resolve the issue altogether. The obvious difference between the 2 fixes being that one uses std::floor() and the other std::round().

16

Thank you for checking. It seems we will be reverting the problematic part of 4ca923a34b3cec79b50c3f1cb794f8327d7530a0 after all.

If you feel like taking a look I’m sharing a patch for the proposed change. I think this should make all observed problems go away.

use-integer-position-for-hittest.patch (728 Bytes)

1 Like
17

Thanks. I’ll try the patch later today or tomorrow and let you know if I run into any issues with it.

18

@attila I haven’t forgotten about this. We have some unrelated customizations in JUCE, and I’m trying to ensure they’re not interfering with my test results. Bear with me - will get back to you later today.

19

@attila I can confirm that the patch seems to work correctly :tada: :pray:

Took a while because I was getting different results in DemoRunner and our app, but that turned out to be a problem at our end.

Thanks again for looking into it. I’ll keep an eye out for the official fix in due course.

1 Like
20

Thanks for testing it. The change is now out on develop

2 Likes