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

Feature/grab mouse #614

Merged
merged 5 commits into from Jul 18, 2016

Conversation

Projects
None yet
@mantognini
Member

mantognini commented May 25, 2014

Related to #394.

Adds sf::Window::setCursorGrabbed implementations for Linux, Windows and Mac OS X. Android and iOS devices have a «not applicable» implementation.

Requires testing on:

  • Linux
  • Windows
  • OS X
  • iOS (build check only)
  • Android (build check only)

Here are a few scenarios to be tested:

  1. Grabbing the cursor works when the window has the focus.
  2. When grabbed, the cursor should not be able to leave the window.
  3. Closing the window should release the cursor.
  4. When grabbed, the window should not be resizable by dragging one of its sides.
  5. When switching to another app, the cursor should be released and re-grabbed when reactivating the SFML app.
  6. If the cursor is outside the view when it is grabbed the cursor should be projected into the window (and thus generate a mouse in event).
  7. The user should not be able to move fast the cursor and click outside the window to activate another application.
  8. The user should not be able to use the window bar to move, minimize, maximize or close the window.
  9. Fullscreen windows should grab the cursor correctly with 2+ monitors.
  10. Programmatically moving the window should keep the cursor inside the rendering area in case the cursor was grabbed and properly update the "grab area" to match the new window position.
  11. Like 10. but when programmatically resizing the window.

This code can be useful for testing.

Below is the list of issue from the above list that need to be fixed before merging this PR:

  • Linux: 5 (should be fixed, confirmation needed)
  • OS X: N/A
  • Window: N/A
  • iOS: N/A
  • Android: N/A

Regarding OS X: [Updated on 25/4/2016]
The current impl is not bad but still an approximation of the feature (test n° 7 is somewhat supported and n° 8 is not supported at all) . It seems really hard to do it on Mac. Other libs (e.g. SDL) have even more bugs or undesired behaviour than this implementation. However, a lot of tricks are used which is not nice at all for future modifications of SFOpenGLView (among other things).

Commit d16d38e contains a more detailed explanation of the known issues (in commit message and comments in the code itself). I'm reluctant to include this commit in SFML codebase because it will considerably increase the complexity of the code and make it really hard to maintain. All that to "only" workaround a stupid corner case... Please let me know what you think.

An alternative for games that need grabbing would be to hide the cursor and use relative motion, preventing the cursor to actually move. This would involve much fewer corner cases in the implementations. Actually, SDL also provides this alternative.

Please refer to commit 4f5baa5 for detailed description of the workarounds involved.

@mantognini mantognini added this to the 2.2 milestone May 25, 2014

@mantognini mantognini self-assigned this May 25, 2014

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini May 25, 2014

Member

@Bromeon Refactoring the code to put common logic into sf::Window might complexify even more OS X impl. It also might increase complexity and make it harder to reason about the code since not all the implementation is contained in OS specific classes. Since the benefit is not that huge I would keep it that way. What do you think?

Member

mantognini commented May 25, 2014

@Bromeon Refactoring the code to put common logic into sf::Window might complexify even more OS X impl. It also might increase complexity and make it harder to reason about the code since not all the implementation is contained in OS specific classes. Since the benefit is not that huge I would keep it that way. What do you think?

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon May 26, 2014

Member

Can't this be handled uniformly for all platforms? When the mouse grab state is not changed, we obviously do not have to do any work for any platform. That would be as simple as

Window::setMouseGrabbed(bool grabbed)
{
    if (m_impl && (m_cursorGrabbed != grabbed))
    {    
        m_impl->setMouseCursorGrabbed(grabbed);
        m_cursorGrabbed = grabbed;
    }
}

I'm not sure about checking for fullscreen, but I could imagine it to be similar. Or is it more complicated for OS X?

A further problem is that it's currently not handled consistently. Some implementations check for these conditions, while others don't.

Member

Bromeon commented May 26, 2014

Can't this be handled uniformly for all platforms? When the mouse grab state is not changed, we obviously do not have to do any work for any platform. That would be as simple as

Window::setMouseGrabbed(bool grabbed)
{
    if (m_impl && (m_cursorGrabbed != grabbed))
    {    
        m_impl->setMouseCursorGrabbed(grabbed);
        m_cursorGrabbed = grabbed;
    }
}

I'm not sure about checking for fullscreen, but I could imagine it to be similar. Or is it more complicated for OS X?

A further problem is that it's currently not handled consistently. Some implementations check for these conditions, while others don't.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch May 26, 2014

Member

It could be done, but I think the current design is better. The interface doesn't know how/if/when the cursor is indeed grabbed (there might be extra handling, like fullscreen or some special window style).

Member

MarioLiebisch commented May 26, 2014

It could be done, but I think the current design is better. The interface doesn't know how/if/when the cursor is indeed grabbed (there might be extra handling, like fullscreen or some special window style).

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini May 26, 2014

Member

When the mouse grab state is not changed, we obviously do not have to do any work for any platform.

While that's true I'm not sure we gain something (performance-wise or implementation-wise).

If we move logic (and thus m_cursorGrabbed + m_fullscreen) into sf::Window or, better yet, into sf::priv::WindowImpl, the OS X impl will suffer from it (it might or might not be a good thing for Linux/Windows impls) since SFOpenGLView would have to access those variables anyway.

Also, I don't know what that would mean for Android and iOS implementation.

Member

mantognini commented May 26, 2014

When the mouse grab state is not changed, we obviously do not have to do any work for any platform.

While that's true I'm not sure we gain something (performance-wise or implementation-wise).

If we move logic (and thus m_cursorGrabbed + m_fullscreen) into sf::Window or, better yet, into sf::priv::WindowImpl, the OS X impl will suffer from it (it might or might not be a good thing for Linux/Windows impls) since SFOpenGLView would have to access those variables anyway.

Also, I don't know what that would mean for Android and iOS implementation.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon May 26, 2014

Member

The interface doesn't know how/if/when the cursor is indeed grabbed (there might be extra handling, like fullscreen or some special window style).

Can you elaborate? The user expects that the mouse is grabbed if and only if he called setMouseCursorGrabbed(true). The implementation cannot do anything meaningful if setMouseCursorGrabbed() is called again with the same value, and in my opinion it doesn't make sense to let each implementation check for it, when it can be done once in sf::Window.

Note that other checks like "disable mouse grabbing when the window loses focus" are not affected by this, I'm only talking about the bool flag set by the user.

Member

Bromeon commented May 26, 2014

The interface doesn't know how/if/when the cursor is indeed grabbed (there might be extra handling, like fullscreen or some special window style).

Can you elaborate? The user expects that the mouse is grabbed if and only if he called setMouseCursorGrabbed(true). The implementation cannot do anything meaningful if setMouseCursorGrabbed() is called again with the same value, and in my opinion it doesn't make sense to let each implementation check for it, when it can be done once in sf::Window.

Note that other checks like "disable mouse grabbing when the window loses focus" are not affected by this, I'm only talking about the bool flag set by the user.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch May 26, 2014

Member

Hm, so you mean you'd set/accept/apply the internal flag either way no matter whether it should be possible to toggle right now? Hm... As an example, when running in exclusive fullscreen, the call shouldn't have any effect. If you request the current state, I'd expect it to return the actual state, not what's set before. Think it boils down to what should be returned, if the user queries the current status.

Edit: For iOS/Android it would be some minor overhead (those variables).

Member

MarioLiebisch commented May 26, 2014

Hm, so you mean you'd set/accept/apply the internal flag either way no matter whether it should be possible to toggle right now? Hm... As an example, when running in exclusive fullscreen, the call shouldn't have any effect. If you request the current state, I'd expect it to return the actual state, not what's set before. Think it boils down to what should be returned, if the user queries the current status.

Edit: For iOS/Android it would be some minor overhead (those variables).

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon May 26, 2014

Member

While that's true I'm not sure we gain something (performance-wise or implementation-wise).

I think it's both: platform implementations are simpler and don't contain code duplication, and we also have no unnecessary system calls when nothing changes.

If we move logic (and thus m_cursorGrabbed + m_fullscreen) into sf::Window or, better yet, into sf::priv::WindowImpl, the OS X impl will suffer from it [...] since SFOpenGLView would have to access those variables anyway.

As said, I'm not sure about fullscreen, but what would suffer in the other case? Do you mean that one would have to store m_mouseGrabbed in both sf::Window and implementation? While I agree that this is not extremely beautiful, duplicating the same code for every platform isn't, either.

Also, I don't know what that would mean for Android and iOS implementation.

Nothing, they'd stay as is. The only change is that their impl->setMouseCursorGrabbed() is called less often.

Member

Bromeon commented May 26, 2014

While that's true I'm not sure we gain something (performance-wise or implementation-wise).

I think it's both: platform implementations are simpler and don't contain code duplication, and we also have no unnecessary system calls when nothing changes.

If we move logic (and thus m_cursorGrabbed + m_fullscreen) into sf::Window or, better yet, into sf::priv::WindowImpl, the OS X impl will suffer from it [...] since SFOpenGLView would have to access those variables anyway.

As said, I'm not sure about fullscreen, but what would suffer in the other case? Do you mean that one would have to store m_mouseGrabbed in both sf::Window and implementation? While I agree that this is not extremely beautiful, duplicating the same code for every platform isn't, either.

Also, I don't know what that would mean for Android and iOS implementation.

Nothing, they'd stay as is. The only change is that their impl->setMouseCursorGrabbed() is called less often.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon May 26, 2014

Member

Hm, so you mean you'd set/accept/apply the internal flag either way no matter whether it should be possible to toggle right now?

The flag in sf::Window has no idea about the implementation, it only stores the value set by the user. The only effect on the platform implementation is that its setMouseCursorGrabbed method is called less often, namely only in the case where it's required.

If you request the current state, I'd expect it to return the actual state, not what's set before.

Maybe this yields a more general question: do you want to have setMouseCursorGrabbed(true) to have an effect if the same call has happened before? I.e. are there situations like a window not in focus where this call would change something? I personally don't think there are, and it's probably not what the user would expect anyway.

Member

Bromeon commented May 26, 2014

Hm, so you mean you'd set/accept/apply the internal flag either way no matter whether it should be possible to toggle right now?

The flag in sf::Window has no idea about the implementation, it only stores the value set by the user. The only effect on the platform implementation is that its setMouseCursorGrabbed method is called less often, namely only in the case where it's required.

If you request the current state, I'd expect it to return the actual state, not what's set before.

Maybe this yields a more general question: do you want to have setMouseCursorGrabbed(true) to have an effect if the same call has happened before? I.e. are there situations like a window not in focus where this call would change something? I personally don't think there are, and it's probably not what the user would expect anyway.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini May 26, 2014

Member

I think it's both: platform implementations are simpler and don't contain code duplication, and we also have no unnecessary system calls when nothing changes.

Imagine the following case:

  1. the user has a SFML window with focus
  2. setCursorGrabbed(true) is called
  3. the user switch to another app (or another app takes the focus by force) thus releasing the cursor
  4. the user then go back to his SFML app

At this point, when the window get back the focus, the impl has to know if the cursor was previously grabbed or not to possibly grab it again.

If the state (m_cursorGrabbed) is stored in sf::Window, there's no way to access it from the impl (or we need to add a cyclic dependency...). If it is stored in sf::priv::WindowImpl, that possible for Linux and Windows impl quite easily but it's less simple on Mac since SFOpenGLView (which handles most of the logic) is not a subclass of sf::priv::WindowImpl.

Same goes for m_fullscreen of course.

So it's not much a win for OS impl. Regarding performance I think it's really negligible compared to any ogl call.

do you want to have setMouseCursorGrabbed(true) to have an effect if the same call has happened before? I.e. are there situations like a window not in focus where this call would change something? I personally don't think there are, and it's probably not what the user would expect anyway.

I think we agree. Here is how I see things:

a. When the window doesn't have the focus, the cursor should not be constraint to the window area
b. Calling setCursorGrabbed on a window that doesn't have focus should grab the cursor as soon as the window gain focus (not before).

Member

mantognini commented May 26, 2014

I think it's both: platform implementations are simpler and don't contain code duplication, and we also have no unnecessary system calls when nothing changes.

Imagine the following case:

  1. the user has a SFML window with focus
  2. setCursorGrabbed(true) is called
  3. the user switch to another app (or another app takes the focus by force) thus releasing the cursor
  4. the user then go back to his SFML app

At this point, when the window get back the focus, the impl has to know if the cursor was previously grabbed or not to possibly grab it again.

If the state (m_cursorGrabbed) is stored in sf::Window, there's no way to access it from the impl (or we need to add a cyclic dependency...). If it is stored in sf::priv::WindowImpl, that possible for Linux and Windows impl quite easily but it's less simple on Mac since SFOpenGLView (which handles most of the logic) is not a subclass of sf::priv::WindowImpl.

Same goes for m_fullscreen of course.

So it's not much a win for OS impl. Regarding performance I think it's really negligible compared to any ogl call.

do you want to have setMouseCursorGrabbed(true) to have an effect if the same call has happened before? I.e. are there situations like a window not in focus where this call would change something? I personally don't think there are, and it's probably not what the user would expect anyway.

I think we agree. Here is how I see things:

a. When the window doesn't have the focus, the cursor should not be constraint to the window area
b. Calling setCursorGrabbed on a window that doesn't have focus should grab the cursor as soon as the window gain focus (not before).

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r May 26, 2014

Member

This currently doesn't work on Windows (8.1) with MinGW (4.9). As soon as I click outside of the Windows or try to push any of the set buttons, the application crashes and the call stack only shows this:
#0 004DC90A __pthread_self_lite () (??:??)

Not quite sure why.

Member

eXpl0it3r commented May 26, 2014

This currently doesn't work on Windows (8.1) with MinGW (4.9). As soon as I click outside of the Windows or try to push any of the set buttons, the application crashes and the call stack only shows this:
#0 004DC90A __pthread_self_lite () (??:??)

Not quite sure why.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini May 27, 2014

Member

I suggest we postpone it to 2.3 and concentrate our effort on #613 to have one nice feature instead of two somewhat working features. Do you agree?

Member

mantognini commented May 27, 2014

I suggest we postpone it to 2.3 and concentrate our effort on #613 to have one nice feature instead of two somewhat working features. Do you agree?

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r May 27, 2014

Member

@MarioLiebisch Hmm yeah that's really odd and I guess implicit conversion allowed the conversion to bool. This might also be the reason why it crashes for me...

Member

eXpl0it3r commented May 27, 2014

@MarioLiebisch Hmm yeah that's really odd and I guess implicit conversion allowed the conversion to bool. This might also be the reason why it crashes for me...

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch May 27, 2014

Member

Ah, right! The call to ClipCursor(). :) And yes, it's indeed the reason for the crash, since it will cause a stack overflow.

Member

MarioLiebisch commented May 27, 2014

Ah, right! The call to ClipCursor(). :) And yes, it's indeed the reason for the crash, since it will cause a stack overflow.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r May 27, 2014

Member

Changing it to ClipCursor fixes the crash and it actually grabs the mouse in window mode. In fullscreen mode the mouse can still travel to the second monitor.

I suggest we postpone it to 2.3 and concentrate our effort on #613 to have one nice feature instead of two somewhat working features. Do you agree?

In my opinion we can leave it for 2.2 and get everything else (website, tutorials, etc.) and if we haven't found time to fix this, till then, we could still move it to 2.x.

Member

eXpl0it3r commented May 27, 2014

Changing it to ClipCursor fixes the crash and it actually grabs the mouse in window mode. In fullscreen mode the mouse can still travel to the second monitor.

I suggest we postpone it to 2.3 and concentrate our effort on #613 to have one nice feature instead of two somewhat working features. Do you agree?

In my opinion we can leave it for 2.2 and get everything else (website, tutorials, etc.) and if we haven't found time to fix this, till then, we could still move it to 2.x.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini May 27, 2014

Member

In my opinion we can leave it for 2.2 and get everything else (website, tutorials, etc.) and if we haven't found time to fix this, till then, we could still move it to 2.x.

Well, I'm not sure we would find someone to review the Mac part soon and it's particularly important for this PR (cf. §Regarding OS X in the description).

BTW, it's easier if only one person pushes to feature/bugfix branches. ;-)

Member

mantognini commented May 27, 2014

In my opinion we can leave it for 2.2 and get everything else (website, tutorials, etc.) and if we haven't found time to fix this, till then, we could still move it to 2.x.

Well, I'm not sure we would find someone to review the Mac part soon and it's particularly important for this PR (cf. §Regarding OS X in the description).

BTW, it's easier if only one person pushes to feature/bugfix branches. ;-)

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon May 27, 2014

Member

BTW, it's easier if only one person pushes to feature/bugfix branches. ;-)

Yes, I'd say only the assignee pushes to his branch, but he can of course ask other people for help (or they can create a separate branch for bigger changes).

By the way, @eXpl0it3r's commit has been authored with a different e-mail address (flexworld instead of my-gate). Maybe the simplest would be for @mantognini to squash it into his previous commit... but I let you two handle that :)

Member

Bromeon commented May 27, 2014

BTW, it's easier if only one person pushes to feature/bugfix branches. ;-)

Yes, I'd say only the assignee pushes to his branch, but he can of course ask other people for help (or they can create a separate branch for bigger changes).

By the way, @eXpl0it3r's commit has been authored with a different e-mail address (flexworld instead of my-gate). Maybe the simplest would be for @mantognini to squash it into his previous commit... but I let you two handle that :)

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r May 27, 2014

Member

BTW, it's easier if only one person pushes to feature/bugfix branches. ;-)

Oops, sorry. I guess we're equal then, for when you merged your PR on your own. 😉

By the way, @eXpl0it3r's commit has been authored with a different e-mail address (flexworld instead of my-gate).

I've setup multiple ones, might want to fix that at some point...

Member

eXpl0it3r commented May 27, 2014

BTW, it's easier if only one person pushes to feature/bugfix branches. ;-)

Oops, sorry. I guess we're equal then, for when you merged your PR on your own. 😉

By the way, @eXpl0it3r's commit has been authored with a different e-mail address (flexworld instead of my-gate).

I've setup multiple ones, might want to fix that at some point...

@mantognini mantognini modified the milestones: 2.x, 2.2 May 28, 2014

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini May 28, 2014

Member

Yes, I'll squash everything nicely when we are happy with all implementations.

PS: I've changed the milestone to 2.x.

Member

mantognini commented May 28, 2014

Yes, I'll squash everything nicely when we are happy with all implementations.

PS: I've changed the milestone to 2.x.

@mantognini mantognini referenced this pull request Jun 14, 2014

Closed

Add Mouse Capture/Grab #394

@dharmab dharmab referenced this pull request Jul 31, 2014

Closed

Mouse Input Support #32

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Aug 20, 2014

Member

I reviewed the code, and it looks good (I only understand the non-OS X stuff 😉).

I tested this on Windows, and all the tests pass, although I don't know if this was intended by 8., but if I switch focus to another window and start dragging the SFML app while it doesn't have focus, I can still drag it via the titlebar, and once I stop dragging, the cursor will be grabbed 😛.

The same goes for Linux. All tests pass, if you disregard that cheaty issue with 8.

On Linux however, I noticed that if I call setMouseCursorGrabbed(true) "too fast" after the window is created, the cursor won't be grabbed, until one of the other things happen, i.e. drag window, resize window, but not clicking in the window or moving the mouse into the window, those won't lead to grabbing either. If I insert an sf::sleep for some arbitrary time (not too short), between creating the window and calling setMouseCursorGrabbed(true), it will all work as intended.

Member

binary1248 commented Aug 20, 2014

I reviewed the code, and it looks good (I only understand the non-OS X stuff 😉).

I tested this on Windows, and all the tests pass, although I don't know if this was intended by 8., but if I switch focus to another window and start dragging the SFML app while it doesn't have focus, I can still drag it via the titlebar, and once I stop dragging, the cursor will be grabbed 😛.

The same goes for Linux. All tests pass, if you disregard that cheaty issue with 8.

On Linux however, I noticed that if I call setMouseCursorGrabbed(true) "too fast" after the window is created, the cursor won't be grabbed, until one of the other things happen, i.e. drag window, resize window, but not clicking in the window or moving the mouse into the window, those won't lead to grabbing either. If I insert an sf::sleep for some arbitrary time (not too short), between creating the window and calling setMouseCursorGrabbed(true), it will all work as intended.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Aug 20, 2014

Member

I tested this on Windows, and all the tests pass, although I don't know if this was intended by 8., but if I switch focus to another window and start dragging the SFML app while it doesn't have focus, I can still drag it via the titlebar, and once I stop dragging, the cursor will be grabbed

I think that's related due to the way window events are processed (by Windows). Nothing you can do about it and it's the same for other programs as well.

For example, most first person shooters will capture the cursor, but using the trick you mentioned, it's possible to move their windows with the mouse.

Member

MarioLiebisch commented Aug 20, 2014

I tested this on Windows, and all the tests pass, although I don't know if this was intended by 8., but if I switch focus to another window and start dragging the SFML app while it doesn't have focus, I can still drag it via the titlebar, and once I stop dragging, the cursor will be grabbed

I think that's related due to the way window events are processed (by Windows). Nothing you can do about it and it's the same for other programs as well.

For example, most first person shooters will capture the cursor, but using the trick you mentioned, it's possible to move their windows with the mouse.

@nabijaczleweli

This comment has been minimized.

Show comment
Hide comment
@nabijaczleweli

nabijaczleweli Jun 5, 2016

Filthy casual :P

nabijaczleweli commented Jun 5, 2016

Filthy casual :P

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jun 8, 2016

Member

I've applied the patch suggested by @eXpl0it3r and rebased against master. Thankfully, OS X didn't need a patch for multi monitor support. :-)

Member

mantognini commented Jun 8, 2016

I've applied the patch suggested by @eXpl0it3r and rebased against master. Thankfully, OS X didn't need a patch for multi monitor support. :-)

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jun 16, 2016

Member

I've updated the PR description to include a list of OSes that need to be tested as well as a list of scenarios to be examined. It would be great if someone different from the implemented could test those.

Member

mantognini commented Jun 16, 2016

I've updated the PR description to include a list of OSes that need to be tested as well as a list of scenarios to be examined. It would be great if someone different from the implemented could test those.

@victorlevasseur

This comment has been minimized.

Show comment
Hide comment
@victorlevasseur

victorlevasseur Jun 17, 2016

Contributor

Tested on Linux. Works nice except that the mouse is not re-grabbed when the window gets the focus back.

Specs : Fedora 23 (x64), Gnome 3.18

Contributor

victorlevasseur commented Jun 17, 2016

Tested on Linux. Works nice except that the mouse is not re-grabbed when the window gets the focus back.

Specs : Fedora 23 (x64), Gnome 3.18

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jun 17, 2016

Member

Thanks for testing! 😃

I've updated the description (and I'll try to keep it up-to-date) with a list of defects to simplify bookkeeping.

Member

mantognini commented Jun 17, 2016

Thanks for testing! 😃

I've updated the description (and I'll try to keep it up-to-date) with a list of defects to simplify bookkeeping.

@victorlevasseur

This comment has been minimized.

Show comment
Hide comment
@victorlevasseur

victorlevasseur Jun 17, 2016

Contributor

More information : this bug only occurs when the window get the focus back while the mouse is outside the window area (and it doesn't write "Failed to grab the mouse cursor !", so the request seems strangely to be successful for XCB).
It also happens sometimes when the cursor is still in the window but it displays "Failed to grab the mouse cursor" those times.

Contributor

victorlevasseur commented Jun 17, 2016

More information : this bug only occurs when the window get the focus back while the mouse is outside the window area (and it doesn't write "Failed to grab the mouse cursor !", so the request seems strangely to be successful for XCB).
It also happens sometimes when the cursor is still in the window but it displays "Failed to grab the mouse cursor" those times.

@victorlevasseur

This comment has been minimized.

Show comment
Hide comment
@victorlevasseur

victorlevasseur Jun 17, 2016

Contributor

It seems that SDL avoids this problem by trying to grab the mouse until it succeeds (but this would cause the app to freeze if the grab is impossible).

Contributor

victorlevasseur commented Jun 17, 2016

It seems that SDL avoids this problem by trying to grab the mouse until it succeeds (but this would cause the app to freeze if the grab is impossible).

@victorlevasseur

This comment has been minimized.

Show comment
Hide comment
@victorlevasseur

victorlevasseur Jun 21, 2016

Contributor

I managed to fix this on my own Github fork of SFML. Can I send you a PR to add the commit to the official branch or should I send a patch instead ?

Contributor

victorlevasseur commented Jun 21, 2016

I managed to fix this on my own Github fork of SFML. Can I send you a PR to add the commit to the official branch or should I send a patch instead ?

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jun 21, 2016

Member

Great! You can create a PR and I'll manually integrate it on this branch so that other can test your fix. :-)

Member

mantognini commented Jun 21, 2016

Great! You can create a PR and I'll manually integrate it on this branch so that other can test your fix. :-)

@victorlevasseur

This comment has been minimized.

Show comment
Hide comment
@victorlevasseur

victorlevasseur Jun 21, 2016

Contributor

Done (#1107). 👍

Contributor

victorlevasseur commented Jun 21, 2016

Done (#1107). 👍

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jun 22, 2016

Member

The proposed patch was included in this PR, so linux users feedback (on both the implementation and its effect) is welcome.

Member

mantognini commented Jun 22, 2016

The proposed patch was included in this PR, so linux users feedback (on both the implementation and its effect) is welcome.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jul 5, 2016

Member

I just tested it again on Windows and all 11 points seem to work as expected, except for point 8, which I'm not certain how you meant it exactly.

  1. The user should not be able to use the window bar to move, minimize, maximize or close the window.

Does that mean one should under no circumstances be able to move the window? On Windows I can't minimize, maximize or close the window via the window bar, but I can move the window, when I first "tab out" of the window and then move the cursor to the bar. Not sure how other OS handle this, but personally I think this may very well be left as OS-specific quirk (unless there's a clean "fix").

Additionally I've successfully tested another point, that being: When you change the mouse cursor programmatically, it shouldn't leave the grabbed area.

I added the following few lines to your code:

case sf::Keyboard::M:
    sf::Mouse::setPosition({-10,-10});
    break;

case sf::Keyboard::R:
    window.setSize({100, 100});
    break;

case sf::Keyboard::P:
    window.setPosition({0, 0});
    break;
Member

eXpl0it3r commented Jul 5, 2016

I just tested it again on Windows and all 11 points seem to work as expected, except for point 8, which I'm not certain how you meant it exactly.

  1. The user should not be able to use the window bar to move, minimize, maximize or close the window.

Does that mean one should under no circumstances be able to move the window? On Windows I can't minimize, maximize or close the window via the window bar, but I can move the window, when I first "tab out" of the window and then move the cursor to the bar. Not sure how other OS handle this, but personally I think this may very well be left as OS-specific quirk (unless there's a clean "fix").

Additionally I've successfully tested another point, that being: When you change the mouse cursor programmatically, it shouldn't leave the grabbed area.

I added the following few lines to your code:

case sf::Keyboard::M:
    sf::Mouse::setPosition({-10,-10});
    break;

case sf::Keyboard::R:
    window.setSize({100, 100});
    break;

case sf::Keyboard::P:
    window.setPosition({0, 0});
    break;
@victorlevasseur

This comment has been minimized.

Show comment
Hide comment
@victorlevasseur

victorlevasseur Jul 5, 2016

Contributor

Does that mean one should under no circumstances be able to move the window? On Windows I can't minimize, maximize or close the window via the window bar, but I can move the window, when I first "tab out" of the window and then move the cursor to the bar. Not sure how other OS handle this, but personally I think this may very well be left as OS-specific quirk (unless there's a clean "fix").

I think it should be the case only when the window has the focus (that's the way I understood it).

Contributor

victorlevasseur commented Jul 5, 2016

Does that mean one should under no circumstances be able to move the window? On Windows I can't minimize, maximize or close the window via the window bar, but I can move the window, when I first "tab out" of the window and then move the cursor to the bar. Not sure how other OS handle this, but personally I think this may very well be left as OS-specific quirk (unless there's a clean "fix").

I think it should be the case only when the window has the focus (that's the way I understood it).

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jul 5, 2016

Member

Yes, that's what I had in mind.

Member

mantognini commented Jul 5, 2016

Yes, that's what I had in mind.

@victorlevasseur

This comment has been minimized.

Show comment
Hide comment
@victorlevasseur

victorlevasseur Jul 8, 2016

Contributor

I can confirm that all points work well on Linux.

Contributor

victorlevasseur commented Jul 8, 2016

I can confirm that all points work well on Linux.

@mantognini mantognini added s:accepted and removed s:undecided labels Jul 9, 2016

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jul 14, 2016

Member

I assume you've tested it on OS X, @mantognini? Doubt we can find anyone for iOS, so should I add it to the merge list?

Member

eXpl0it3r commented Jul 14, 2016

I assume you've tested it on OS X, @mantognini? Doubt we can find anyone for iOS, so should I add it to the merge list?

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jul 15, 2016

Member

Yes, you can go ahead.

Member

mantognini commented Jul 15, 2016

Yes, you can go ahead.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jul 17, 2016

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Jul 17, 2016

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

MarioLiebisch and others added some commits May 1, 2014

Added ability to grab the cursor (w/ Windows impl.)
 * When grabbed, the mouse cursor may not be moved outside a window's client frame.
 * Fullscreen windows always grab the mouse cursor.
 * The effect is only active while the SFML window is the active
foreground window.
 * Right now this is only implemented for Windows.

Signed-off-by: Marco Antognini <antognini.marco@gmail.com>
Added OS X implementation of sf::Window::setCursorGrabbed
This implementation uses the following workaround:
 - resize flag is removed from the window when the cursor is grabbed
 - when grabbed, the cursor is projected inside the view if needed
 - to avoid letting the user clic outside the view, the cursor is
   disconnected and manually moved using relative motion
 - the initial potential projection of the cursor results in a big
   delta for the next move and needed to be somehow ignored (see
   note about m_deltaXBuffer and m_deltaYBuffer in SFOpenGLView.h)
Added Unix implementation of sf::Window::setCursorGrabbed (#394), fix…
…ed xcb_set_input_focus being called on a window before it is viewable on some window managers (#991).

@eXpl0it3r eXpl0it3r merged commit 6152662 into master Jul 18, 2016

14 checks passed

debian-gcc-64 Build #161 done.
Details
freebsd-gcc-64 Build #161 done.
Details
osx-clang-el-capitan Build #41 done.
Details
static-analysis Build #161 done.
Details
windows-gcc-492-tdm-32 Build #46 done.
Details
windows-gcc-492-tdm-64 Build #46 done.
Details
windows-gcc-530-mingw-32 Build #46 done.
Details
windows-gcc-530-mingw-64 Build #46 done.
Details
windows-vc11-32 Build #162 done.
Details
windows-vc11-64 Build #162 done.
Details
windows-vc12-32 Build #162 done.
Details
windows-vc12-64 Build #160 done.
Details
windows-vc14-32 Build #161 done.
Details
windows-vc14-64 Build #163 done.
Details
@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jul 18, 2016

Member

🎉

Member

eXpl0it3r commented Jul 18, 2016

🎉

@eXpl0it3r eXpl0it3r deleted the feature/grab_mouse branch Jul 18, 2016

@daijin12

This comment has been minimized.

Show comment
Hide comment
@daijin12

daijin12 Aug 19, 2016

Hello,
for some reasons, when creating a window integrated into a Qt widget, the lines 924 and 925 "while(!m_windowMapped) processEvents();" are lost inside a infinite loop...
Comment these lines allows to solve the problem.
Do you have an idea why?

Cheers,
daijin12

daijin12 commented on 6f3273b Aug 19, 2016

Hello,
for some reasons, when creating a window integrated into a Qt widget, the lines 924 and 925 "while(!m_windowMapped) processEvents();" are lost inside a infinite loop...
Comment these lines allows to solve the problem.
Do you have an idea why?

Cheers,
daijin12

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 19, 2016

Member

It would be best, if you could open a forum thread where you properly outline your situation and what exactly happens.

Member

eXpl0it3r replied Aug 19, 2016

It would be best, if you could open a forum thread where you properly outline your situation and what exactly happens.

This comment has been minimized.

Show comment
Hide comment
@daijin12

daijin12 replied Aug 19, 2016

ok

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