Mouse events by Xaeroxe · Pull Request #344 · rust-windowing/winit

Xaeroxe · 2017-11-06T22:05:52Z

Replaces #192

Resolves conflicts from that PR
Adds support for Windows and OSX

This makes the API more intuitive for common use-cases and allows us to better propagate platform knowledge of motion semantics.

This emphasizes the difference between motion of the host GUI cursor, as used for clicking on things, and raw mouse(-like) input data, as used for first-person controls.

Ralith

1f9afe7 (removing the Motion/Moved fix) bothers me a bit. Since this is a breaking change anyway, I'd like to make the event names consistent to reduce future frustration. This is not a blocker for merging because it's a pre-existing issue.

I'm in favor of the other changes, especially the mousewheel fixes--thanks! This should make the correct solution to typical gamedev mouse input significantly more obvious.

torkleyy

Nice 👍

torkleyy · 2017-11-07T17:16:46Z

src/events.rs

+    },
+
+    /// Motion on some analog axis. May report data redundant to other, more specific events.
    Motion { axis: AxisId, value: f64 },


Not sure if I understood this. Does this mean that whether Motion events are also emitted for the mouse is platform-dependent?

Mouse events should be emitted on all major desktop platforms. Mouse events will also be accompanied by Motion events from the same device with the same deltas. Not all Motion events will be accompanied by Mouse events though, as some Motion events won't come from the Mouse.

You could improve this comment to clearly note that Motion is a fallback event for any motion along axis that are not already handled by MouseMoved or MouseWheel (since it seems to be what your implementation does).

@vberger Oh hm woops, my Windows and OSX implementation of this behaves differently from the X11 implementation provided by @Ralith . I'm not using the motion events as a fall back, but I'm instead using them to accompany the mouse events. I'll need to get those behaviors in sync before we merge.

Ah, I see. So, the actual intended behavior is for the generic Motion to repeat MouseMoved? Is there a clear reason for this repetition?

Motion is good for being abstract over axes, so it makes sense to include mouse axes in that abstraction. MouseMoved is for when you don't want to be abstract over axes, and you just want the mouse.

Would be nice to include a detailed description of this in the docs given the confusion in these reviews.

torkleyy · 2017-11-07T17:20:22Z

@Ralith I prefer the names that @Xaeroxe added since AxisMoved sounds a bit odd to me because it's not the axis that got moved.

elinorbgr · 2017-11-07T17:34:33Z

Looks good to me modulo the doc nitpick. 👍

Ralith · 2017-11-07T17:57:20Z

@torkleyy

I prefer the names that @Xaeroxe added since AxisMoved sounds a bit odd to me because it's not the axis that got moved.

I don't really care what the names are, it'd just be nice if they were consistent.

torkleyy

I like it, but a more detailed description of the mouse / motion events as well as platform-specific behavior would be nice.

torkleyy · 2017-11-07T17:58:46Z

@Ralith I'd like them to be consistent, too, is there any other place where AxisMoved has been used? Or is it about the Moved postfix?

Ralith · 2017-11-07T18:02:38Z

The issue I was discussing was inconsistent use of "Moved" versus "Motion." I don't see a need to have both terms; the introduction of a second one was unintended.

torkleyy · 2017-11-07T18:07:51Z

I see. Let's rename all the variants to *Motion then.

tomaka · 2017-11-07T19:15:45Z

If we're breaking the API of Events, I think we should group all the changes concerning this enum in the same version in order to reduce friction.

Xaeroxe · 2017-11-07T22:33:07Z

r? @torkleyy @Ralith @vberger

elinorbgr · 2017-11-12T14:13:06Z

looks like it needs a rebase. Likely a consequence of #342 which move the pointer logic for wayland into its own file.

Xaeroxe · 2017-11-12T15:25:05Z

@vberger I've resolved the conflict.

So what's the policy for merging PRs in this repo?

elinorbgr · 2017-11-12T15:33:57Z

src/platform/linux/wayland/event_loop.rs

+    axis_buffer: Option<(f32, f32)>,
+    axis_discrete_buffer: Option<(i32, i32)>,
+    axis_state: TouchPhase,
+}


Is it github messing up the displayed diff? It looks like you're undoing part of #342 by re-adding pointer logic in event_loop.rs and removing touch support in the seat...

Woops I think I grabbed the wrong pieces while resolving this, thanks and good catch. Let me try again.

Xaeroxe · 2017-11-12T16:20:59Z

@vberger Ok I think I got it right this time. git has a really weird idea of how changes impact sections of code sometimes and it confused me because I was expecting git to be smarter than it was.

elinorbgr · 2017-11-12T16:24:53Z

Looks like that's not yet it, the struct PointerIData and fn pointer_implementation() should not end up in event_loop.rs.

I think git is getting confused because you modified some part of this code (while changing the variant names), but #342 has moved this code to an other file (pointer.rs)

Xaeroxe · 2017-11-12T17:34:34Z

@vberger Thanks for describing the changes to me, sorry I should have reviewed the PR. I think this is ready now.

elinorbgr

Ok, this looks good. I'll merge this once the CI is finished if all is green.

elinorbgr · 2017-11-12T17:51:55Z

Oh, actually one last thing is missing: could you add a line in the changelog describing the breaking changes which were made?

Xaeroxe · 2017-11-12T18:07:38Z

@vberger done.

elinorbgr · 2017-11-12T20:56:43Z

Alright, perfect! 👍

Ralith and others added 6 commits July 2, 2017 20:34

Explicit mouse-related DeviceEvents

80c6109

This makes the API more intuitive for common use-cases and allows us to better propagate platform knowledge of motion semantics.

Improve event naming consistency

b91acbb

Clarify axis event forwards-compatibility

034b059

Rename WindowEvent::MouseMoved/Entered/Left to CursorMoved/...

15121d3

This emphasizes the difference between motion of the host GUI cursor, as used for clicking on things, and raw mouse(-like) input data, as used for first-person controls.

Merge

0e1670a

Add support for windows and OSX, fix merging

1e3c498

Xaeroxe mentioned this pull request Nov 6, 2017

DeviceIds contain no data on most platforms #338

Closed

Fix warnings and errors on Linux

9099fe8

Xaeroxe mentioned this pull request Nov 6, 2017

Extend amethyst_input with gamepad support amethyst/amethyst#488

Closed

4 tasks

Xaeroxe added 2 commits November 6, 2017 15:28

Remove unnecessary breaking changes

1f9afe7

Add MouseWheel events to windows and OSX

f439c23

Ralith approved these changes Nov 7, 2017

View reviewed changes

Ralith mentioned this pull request Nov 7, 2017

Mouse device events #192

Closed

Fix bad push call.

72b790e

torkleyy reviewed Nov 7, 2017

View reviewed changes

torkleyy approved these changes Nov 7, 2017

View reviewed changes

Fix docs, naming, and x11 events

5f60f36

elinorbgr approved these changes Nov 8, 2017

View reviewed changes

elinorbgr reviewed Nov 12, 2017

View reviewed changes

Merge from upstream

6f5d884

Remove mutability warning

d17560c

elinorbgr approved these changes Nov 12, 2017

View reviewed changes

Add changelog entry

8f3c33c

elinorbgr merged commit cfd087d into rust-windowing:master Nov 12, 2017

Xaeroxe deleted the mouse-events branch November 12, 2017 21:15

Xaeroxe mentioned this pull request Nov 15, 2017

Send mouse position after focused/cursorenter events #349

Merged

Conversation

Xaeroxe commented Nov 6, 2017

Uh oh!

Ralith left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torkleyy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torkleyy commented Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elinorbgr commented Nov 7, 2017

Uh oh!

Ralith commented Nov 7, 2017

Uh oh!

torkleyy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torkleyy commented Nov 7, 2017

Uh oh!

Ralith commented Nov 7, 2017

Uh oh!

torkleyy commented Nov 7, 2017

Uh oh!

tomaka commented Nov 7, 2017

Uh oh!

Xaeroxe commented Nov 7, 2017

Uh oh!

elinorbgr commented Nov 12, 2017

Uh oh!

Xaeroxe commented Nov 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xaeroxe commented Nov 12, 2017

Uh oh!

elinorbgr commented Nov 12, 2017

Uh oh!

Xaeroxe commented Nov 12, 2017

Uh oh!

elinorbgr left a comment

Choose a reason for hiding this comment

Uh oh!

elinorbgr commented Nov 12, 2017

Uh oh!

Xaeroxe commented Nov 12, 2017

Uh oh!

elinorbgr commented Nov 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

Ralith left a comment •

edited

Loading

torkleyy commented Nov 7, 2017 •

edited

Loading

torkleyy left a comment •

edited

Loading