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

Lock mouse position #16479

Merged
merged 5 commits into from Jun 27, 2019

Conversation

@teinarss
Copy link
Contributor

commented May 1, 2019

Fixes #16475
Fixes #16466

@pchote

This comment has been minimized.

Copy link
Member

commented May 4, 2019

Using the airstrike (both with or without activating the drag mode) resets the cursor position to the middle of the window. We should store and then restore the cursor position after disabling relative mode.

@teinarss teinarss force-pushed the teinarss:lock_mouse branch from 5b123a9 to 2cc928d May 4, 2019

OpenRA.Game/Graphics/HardwareCursor.cs Outdated Show resolved Hide resolved
OpenRA.Game/Graphics/HardwareCursor.cs Show resolved Hide resolved
OpenRA.Game/Graphics/HardwareCursor.cs Outdated Show resolved Hide resolved

@teinarss teinarss force-pushed the teinarss:lock_mouse branch from 2cc928d to 4dc0e70 May 4, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

updated

@teinarss teinarss force-pushed the teinarss:lock_mouse branch from 4dc0e70 to 865be7f May 4, 2019

@teinarss teinarss force-pushed the teinarss:lock_mouse branch 2 times, most recently from 4c3bcb6 to e6a36b0 May 4, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

Updated

@pchote
Copy link
Member

left a comment

I'm sorry to say that I found a couple more issues while testing. This will hopefully be the last set!

OpenRA.Game/Graphics/HardwareCursor.cs Outdated Show resolved Hide resolved
OpenRA.Game/Graphics/SoftwareCursor.cs Show resolved Hide resolved

@teinarss teinarss force-pushed the teinarss:lock_mouse branch from e6a36b0 to cdcec36 May 5, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

Updated

@pchote

This comment has been minimized.

Copy link
Member

commented May 5, 2019

This really is the feature that keeps on giving (and not in a good way)...

Locking the mouse in relative mode doesn't (at least on macOS) freeze the coordinates that are returned through the MouseMotionEvent. If you drag the invisible cursor over the sidebar or command bar the cursor will change and if you drag it over the radar it will start moving the viewport!

Screenshot 2019-05-05 at 12 09 29

I've prototyped a fix in 9cde310 which introduces the concept of a relative mouse move, and freezes the position of the cursor on our event handling side when locked. This also let me implement a polish improvement that I was wanting from the very start: clamping move deltas, so if you scroll e.g. a virtual 300px to the left, but don't have to undo all of that before the direction flips to the right.

I think it would be best if we could merge this with your two commits and then split out a new first commit with all the event and cursor changes, and then a second commit that updates SelectDirectionalTarget to use them.

@pchote

This comment has been minimized.

Copy link
Member

commented May 5, 2019

A second issue that isn't nearly as bad, but still annoying for polish: the drag direction arrows are drawn as part of the "world" layer, which means that they end up behind the UI if you place the cursor down next to the sidebar before dragging.

This can be fixed by rendering the directional arrows from the cursor code. I don't know whether we should expose this as an explicit direction feature on the cursor, or as some more general concept. If we made it more general we could e.g. implement a speed-dial UI for selecting different types of beacons ("attack", "defend", "enemy spotted", "build here", etc). Maybe expose a SetAttachment(string cursor, int2 offset) method and leave it to the caller to calculate the offsets? I don't think we'd need to display more than one at a time.

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

Added a wip on handling the rendering above the UI, no need to comment on things not in the RenderAbove commit atm, haven't polished it at all. Not sure if this is the best solution, maybe we should wait with the issue with the arrows behind the main UI until we have gathered new energy for that feature and better ideas?

@pchote pchote added this to the Next Release milestone May 18, 2019

@teinarss teinarss force-pushed the teinarss:lock_mouse branch from c11ed0c to 240374a Jun 1, 2019

@teinarss teinarss force-pushed the teinarss:lock_mouse branch from 240374a to 0d96631 Jun 1, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

Two issues while testing:

  • RA is missing the mouse attachment widget, so crashes when activating parabombs
  • The cursor seems to blink down-right for a single frame when releasing - probably the hardware cursor being unlocked and rendered wrong just before it is cleared?
@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2019

@teinarss teinarss force-pushed the teinarss:lock_mouse branch from c44da97 to fe293bb Jun 9, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2019

updated

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2019


[12:24:57] | <lawANDorder> hm looks like the cursor now *always* jumps to the center for a short moment
[12:25:15] | <lawANDorder> previously it wouldn't always do that
[12:30:24] | <lawANDorder> Mesacer_ it happens btw in that PR also for the non-directional spyplane
[12:33:25] | <pchote>  i wonder if we can reduce some of these issues by dropping the  relativemousemode and bodging that ourselves using WarpMouse each tick
[12:34:29] | <pchote> PumpInput already knows about cursor locking so in principle we can just force-reset the position there each tick
[12:34:51] | <Mesacer_> i did try that before
[12:34:58] | <Mesacer_> did not work good
[12:35:18] | <pchote> was that before or after the scroll delta changes we put in?
[12:36:31] | <pchote> Mesacer_: we could try doing the cursor resetting thing inside the platform dll
[12:36:47] | <pchote> disable it before calling the sdl function to enable/disable relative mode, then reset it afterwards
[12:37:22] | <pchote> might help if we can't get rid of it completely
[12:38:23] | <Mesacer_> i did that too and the problem was not the delta but with the cursor itself, the cursor had a rubber band effect
[12:38:33] | <pchote> we'd make the cursor invisible



@pchote

This comment has been minimized.

Copy link
Member

commented Jun 9, 2019

137d376 demonstrates how we can work around SDL_SetRelativeMouseMode if that is indeed the source of the problems here.

@teinarss teinarss force-pushed the teinarss:lock_mouse branch from fe293bb to 005b341 Jun 9, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2019

Updated

@matjaeck
Copy link
Contributor

left a comment

Cursor is now fixed on my end and I didn't notice any other regressions. 👍

@teinarss teinarss force-pushed the teinarss:lock_mouse branch from 005b341 to f190eef Jun 9, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented Jun 9, 2019

The core mouse stuff LGTM, but the attachment rework regressed the software cursor pixel doubling mode:

Screenshot 2019-06-09 at 21 29 45

👍 once this is fixed.

@teinarss teinarss force-pushed the teinarss:lock_mouse branch from f190eef to 7100b8e Jun 10, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

Updated

@matjaeck
Copy link
Contributor

left a comment

The cursor looks now the same when pixel doubling is enabled/disabled/toggled while the cursor is active.

@pchote

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Can we get a second review here? I'd really like to get rid of these regressions from bleed so we can move on.

@abcdefg30 abcdefg30 merged commit f325a4d into OpenRA:bleed Jun 27, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abcdefg30

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.