Experiment with an improved event design #1374
Conversation
Osspial
changed the title
|
I have not precisely reviewed the actual detailed code, but overall these principles get a 👍 from me. There is just one thing I'd like to mitigate (but it's clearly on the nitpick level):
In terms of matching, I don't think structural patterns are more verbose thant tuple ones, as long as you don't need to rename the fields: match event {
// structural variant
MouseWhell { device_id, delta, phase, modifiers } => {/* ... */},
/// tuple variant
MouseWhell(device_id, delta, phase, modifiers) => {/* ... */}
}And structural patterns come with the advantage of being imo more ergonomic when you only need to process a subset of the fields, and you get the additional bonus of not needing to match the exact order of the fields: match event {
// structural variant
MouseWhell { delta, ... } => {/* ... */},
/// tuple variant
MouseWhell(_, delta, _, _) => {/* ... */}
} |
|
@vberger I don't think those complaints are valid when the new API is taken as a whole, since events have been flattened out significantly. Compare the old MouseWheel {
device_id: DeviceId,
delta: MouseScrollDelta,
phase: TouchPhase,
modifiers: ModifiersState,
},
pub enum MouseScrollDelta {
LineDelta(f32, f32),
PixelDelta(LogicalPosition<f64>),
}to the new scroll events: ScrollStarted,
ScrollDiscrete(Vector<i32>),
ScrollSmooth(Vector<f64>),
ScrollEnded,
pub struct Vector<T> {
pub x: T,
pub y: T,
}You don't have to worry about how ergonomic it is to ignore data when the events don't contain data that needs to be ignored. The event flattening also helps out with field ordering; no event has more than three fields, and only three events have more than two fields: WindowEvent::KeyPress(LogicalKey, ScanCode, PressFlags)
WindowEvent::PointerPress(PointerId, PointerButton, PressFlags)
RawKeyboardEvent::Press(Option<LogicalKey>, ScanCode, RawPressFlags)Personally speaking, I find it much easier to remember "the first field has the keycode, the second field has the scancode, and the third field has boolean flags" than it is to remember the exact names for each of the fields. I constantly get tripped up when matching on the events from memory, since the exact names for things just don't stick, and additionally it's hard to remember which particular events are tuple variants and which are structural variants.
This is true if you're just accessing each field, but is not the case if you're matching on each particular field. Compare matching on an // old
WindowEvent::KeyboardInput(KeyboardInput{virtual_keycode: Some(VirtualKeyCode::A), state: ElementState::Pressed, ..}) => ()
// new
WindowEvent::KeyPress(Some(LogicalKey::A), _, flags) if flags.is_down() => (),That's admittedly a somewhat unfair comparison, since the new API does a lot of additional stuff to reduce verbosity, but it should still be clear that even with noise reduced a minimum tuple variants are significantly more readable than structural variants. |
Some alternative conceptions, since if we're going to break things we should not consider the status quo to be the only alternative to the proposal:
Both of these similarly require no extra imports, have better forwards-compatibility with future additions, and don't require the reader to guess at the meanings of non-enum (e.g. button number) fields. The former is shorter than the proposal. |
|
@Ralith's proposed design:
appeals to me, especially for the reason of forwards-compat. |
|
I especially like the |
|
In lieu of |
I also like that idea, and I've drafted it out for the key-press and pointer-press events. How does this new implementation look to you all? |
src/event.rs
Outdated
| pub(crate) logical_key: Option<LogicalKey>, | ||
| pub(crate) scan_code: u32, | ||
| pub(crate) is_down: bool, | ||
| pub(crate) repeat_count: u32, | ||
| pub(crate) is_synthetic: bool, | ||
| } | ||
|
|
||
| #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] | ||
| pub struct RawKeyPress { | ||
| pub(crate) logical_key: Option<LogicalKey>, | ||
| pub(crate) scan_code: u32, | ||
| pub(crate) is_down: bool, | ||
| } | ||
|
|
||
| #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] | ||
| pub struct PointerPress { | ||
| pub(crate) button: u8, | ||
| pub(crate) is_down: bool, | ||
| pub(crate) click_count: u32, | ||
| } | ||
|
|
||
| /// Touch event has been received | ||
| Touch(Touch), | ||
| #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] | ||
| pub struct RawPointerPress { | ||
| pub(crate) button: u8, | ||
| pub(crate) is_down: bool, |
There was a problem hiding this comment.
What's the benefit of disabling direct access to those fields for crate users. I feel like it should be fine and much simpler than calling getters.
There was a problem hiding this comment.
Direct field access is definitely very convenient. Getters have a couple significant advantages:
- Adding new getters isn't a breaking change, so new data to events is very unlikely to require existing users to update their code.
- Getters can return values that are computed from, rather than stored in, the struct. This is useful when the type of a field need to change to carry additional information, in which case the old getter can be preserved at zero cost, giving users time to switch. Although if fields can be marked deprecated, this might not be a major advantage, since one more field in an event isn't likely to be a significant cost.
There was a problem hiding this comment.
Adding new getters isn't a breaking change, so new data to events is very unlikely to require existing users to update their code
I feel like it's related to a fact, that you'll make a constructing from fields available? The thing is that using getters in match arms is just a pain and make the code more nested and complex, however with direct access it's way easier to write things and it makes more sense in the end.
Getters can return values that are computed from, rather than stored in, the struct. This is useful when the type of a field need to change to carry additional information, in which case the old getter can be preserved at zero cost, giving users time to switch. Although if fields can be marked deprecated, this might not be a major advantage, since one more field in an event isn't likely to be a significant cost.
In general, but we're talking about events here, which you just forward to the user anyway.
Like I just don't feel like it'll make any advantage at all for the end user. You can see that this feature is very heavily used here https://github.com/alacritty/alacritty/blob/058b036fe7f3a7cdc2ebdc21ab25aded783e4e59/alacritty/src/event.rs#L573 , with getters it'll be way harder to write.
There was a problem hiding this comment.
I feel like it's related to a fact, that you'll make a constructing from fields available?
I'm not sure what this means. For the same compatibility reasons, these events don't and shouldn't have public constructors.
The thing is that using getters in match arms is just a pain and make the code more nested and complex, however with direct access it's way easier to write things and it makes more sense in the end.
A side-by-side example was presented above:
WindowEvent::KeyPress { key: Some(LogicalKey::A), is_down: true, .. } => ()
WindowEvent::KeyPress(e) if e.key() == Some(LogicalKey::A) && e.is_down() => ()
The forwards-compatible version is exactly four characters longer, and I don't think it's in any sense "more nested". Further, in my experience you're only matching on exact values in toy code, so the impact of this is limited.
In general, but we're talking about events here, which you just forward to the user anyway.
Whether you're matching on a field or forwarding it elsewhere, having its name/type/presence not change when new capabilities are added reduces breakage.
There was a problem hiding this comment.
You can see that this feature is very heavily used here https://github.com/alacritty/alacritty/blob/058b036fe7f3a7cdc2ebdc21ab25aded783e4e59/alacritty/src/event.rs#L573 , with getters it'll be way harder to write.
Which feature are you referring to specifically? I don't see anything there which would obviously become much more complex with the proposed change.
There was a problem hiding this comment.
Yeah, and then every application create one more event to test things, which just copies winit event, except some fields they don't care about, and then you're forced to create generic abstractions over event just to support testing here or convert everything into your event adding more clones/copies.
Like alacritty is just an example here of an app, which is using winit events for testing, to just prove that it's not something uncommon.
There was a problem hiding this comment.
Every winit app I've ever written has immediately transformed event data into some other form, typically simple primitive values passed to a handler method. There are a variety of different approaches to consuming these events; it is absolutely not the case that literally every application would be writing a bunch of redundant code.
If you have hard data on how what proportion of winit applications do, in fact, store/pass winit events around to a significant degree, that would certainly be useful for informing the design!
There was a problem hiding this comment.
By 'every application' I mean that you don't have choice anymore to do what you're doing or to do what we're doing. You just have one path - have your own event type and translate winit events.
Like ok, I personally can live with such change and we likely can refactor alacritty event handling adding even more quirks to some winit cases (though we'll likely remove one of them soon). But I'm not really sure for how long I want to workaround winit design, since it's becoming stricter and stricter.
There was a problem hiding this comment.
Those are reasonable concerns, and I've added constructors and setters for each of the event types to address those.
I'm hesitant to expose the struct fields directly, since that means that computed fields would be exposed with a different syntax than stored fields, and we're using computed fields to add an is_up() method that can be used instead of !is_down() for detecting keyup events.
There was a problem hiding this comment.
I'm hesitant to expose the struct fields directly, since that means that computed fields would be exposed with a different syntax than stored fields, and we're using computed fields to add an is_up() method that can be used instead of !is_down() for detecting keyup events.
thx.
|
Overall those examples look good to me: forwards-compatible without unduly compromising ergonomics. |
|
Besides the points noted by @kchibisov already, there are two other things that seem a bit odd to me. I'm not really familiar with winit and Wayland (@vberger probably knows more about this), but wouldn't the removal of device ID prevent multiseat support? That seems like a bad idea. Converting values like |
|
Food for thought: an enum could be compared (but not matched) against concisely and without imports by writing |
There's an additional reason which, looking back, I think I forgot to mention. It doesn't make sense to expose the Still, that's an entirely valid point about type safety. I've redefined the |
|
I rather liked the maximal concision of |
|
I've added code to ensure that the new |
|
@Osspial I can contribute Wayland impl once you've done with API on this branch, just ping me at some point, I guess. |
| // and it doesn't make a whole lot of sense for a traditional mouse | ||
| // pointer to be on a non-integer point, so round off any fractional | ||
| // values that might've cropped up. | ||
| position.x = position.x.round(); | ||
| position.y = position.y.round(); |
There was a problem hiding this comment.
it doesn't make a whole lot of sense for a traditional mouse pointer to be on a non-integer point
I think this is a mistake. High-precision mouse pointers are widely supported and are useful for a number of purposes, e.g. in art applications. See prior discussion in #1375.
There was a problem hiding this comment.
High-precision mouse pointers are typically from pen/touch devices, which this rounding does not effect. If there are any relatively widely-used devices that this change would negatively effect I'd be open to reverting this, but otherwise I think it's an appropriate action to take for standard pixel-aligned mouse devices.
There was a problem hiding this comment.
Touchpads are a touch device. I'm not willing to assume without evidence that Windows always discards high-precision data from mice, either.
There was a problem hiding this comment.
Do you mean that touchpads are mouses actually? IIRC touchpads do have high precision data, but are treated as pointers on at least Wayland. So yeah, removing fractional part is likely not a good thing to do and I don't see much issue with it?
Also, even mouse on Wayland will have fractional part by default, so I'm not sure that things should diverge here. clients can always round themselves if they don't care, I guess.
There was a problem hiding this comment.
Sorry, the terminology gets a bit confusing here. I mean that touchpads are high-precision input devices driven by a finger which can, should, and on non-Windows backends do have that precision propagated to the application. Even conventional mice can be, contrary to the comment above.
There's no reason a conventional mouse should be treated differently either, as you observe. Silently discarding perfectly good data is bad behavior.
There was a problem hiding this comment.
although it's not really useful in the windowing system with a standard pointer
I'd like to emphasize that high-precision pointers absolutely can be useful. For example, any time you're interacting with a vector or zoomed-out raster graphics canvas, or operating a continuous UI control like a slider.
There was a problem hiding this comment.
or operating a continuous UI control like a slider
Ah, true there, I have actually noticed that on some KDE sliders where I can barely adjust them but the cursor doesn't actually move...
There was a problem hiding this comment.
Touchpads are a touch device. I'm not willing to assume without evidence that Windows always discards high-precision data from mice, either.
I have a laptop that uses the Windows Precision Touchpad drivers. When using the touchpad, Windows does not dispatch sub-pixel pointer movement events, and each movement event's position is within a +-0.05 margin of an integer pixel number. WRT mice - I have a Logitech G502 gaming mouse, and Windows exhibits the same behavior there. This is a rounding error, and should be treated as such.
My mouse, standard (very high resolution) mouse also sends fractional parts when moving, and although it's not really useful in the windowing system with a standard pointer, it's very useful in games that support them.
Are you on a Windows system? If so, could you check whether or not event-polish with this diff applied emits subpixel mouse movement events in the window example?
diff --git a/src/platform_impl/windows/event_loop.rs b/src/platform_impl/windows/event_loop.rs
index 3702d352..b1d7ae8c 100644
--- a/src/platform_impl/windows/event_loop.rs
+++ b/src/platform_impl/windows/event_loop.rs
@@ -1549,8 +1549,8 @@ unsafe extern "system" fn public_window_callback<T: 'static>(
// and it doesn't make a whole lot of sense for a traditional mouse
// pointer to be on a non-integer point, so round off any fractional
// values that might've cropped up.
- position.x = position.x.round();
- position.y = position.y.round();
+ // position.x = position.x.round();
+ // position.y = position.y.round();
buttons_down.set(
PointerButton::MOUSE_LEFT.into_flags(),There was a problem hiding this comment.
If it is ultimately determined that windows genuinely never reports non-integer mouse/touchpad coordinates, then at the very least the comment should reflect that, rather than claiming there's no use case for higher precision data. That way if a future change makes higher-precision data available there won't be cause for confusion as to whether it should be discarded.
There was a problem hiding this comment.
Are you on a Windows system?
I'm not, linux with a custom mouse driver because my mouse is... unique.
| PointerCreated(PointerId), | ||
| PointerEntered(PointerId), | ||
| PointerForce(PointerId, Force), | ||
| PointerTilt(PointerId, PointerTiltEvent), | ||
| PointerTwist(PointerId, f64), | ||
| PointerContactArea(PointerId, PhysicalSize<f64>), | ||
| PointerMoved(PointerId, PhysicalPosition<f64>), | ||
| // TODO: IMPLEMENT. IS THIS REASONABLE TO IMPLEMENT ON NON-WINDOWS PLATFORMS? | ||
| // PointerHovered(PointerId), | ||
| PointerButton(PointerId, PointerButtonEvent), | ||
| PointerDestroyed(PointerId), | ||
| PointerLeft(PointerId), |
There was a problem hiding this comment.
These fields lack documentation. Do they work as follows?
- Create a pointer corresponding to the mouse on app start
- Create a pointer corresponding to a touch (with unique ID) when the touch starts and destroy when the touch ends
- Send
PointerButtonevents just after creating and before destroying every touch event - The same for a pen, if it exists (since in general one can't provide
PointerMovedevents for a pen not touching the screen)
For motion events, presumably the app must remember the last position for each pointer if it wishes to know relative motion? As you see here I ended up providing both absolute and relative positions for motion events in my unified API (but this API is for higher-level usage and misses some of the details).
How do the pen events (force, tilt, twist) work for sensitive pens? Do they tend to send all of these events each frame? If so it may be more convenient to receive all this data simultaneously. (But I'm only guessing since I haven't worked with pens.)
|
A curiosity, does this give an ID to mice as well? I.E. would multiple mice (on system types that support it like X11 or wayland, or windows with an admin callback hook) be able to distinguish the different mice? I exceptionally hate having to bypass windowing mice handling to be able to support this feature so it would be nice to actually be built in for once. |
|
@OvermindDL1 it looks like it, given that a |
If a program isn't built to handle them then they usually just handle the 'first' mouse or just kind of combine all their inputs together (bleh). For programs that are aware of it as I try for mine to be then they work quite well and has some very useful usecases even outside of games. |
|
Multi-mouse may not be exclusive to Linux either (though this sounds limited): https://www.mousemux.com/ |
|
It's not that buggy on Wayland, but I'm not sure that it's that supported across compositors, in general, multiseat(aka multiple keyboard + pointers) is a thing. winit side here will be simple (when it comes to Wayland at least), since everything is already handed by you, and you know that you, let's say, have multiple pointers, if you have multiple seats with pointer capability. The pointer/keyboard event are per seat, so the only thing where winit should likely do a bit more work here, is 'proper id' for them(not sure how it'll work). |
|
Plus you want a good DE that's supporting it well as well, otherwise yes, as like on the link it can cause flickering of which program has mouse 'activity'. KDE used to be the best supported, wonder if it still is... Although if both mice are being used in the same program then that's not an issue anyway. |
|
Just tested on X11 + KDE: it works, but the second cursor causes horrible flickering. Additionally, it appears the keyboard should only follow the first pointer, but the second can shift keyboard focus within most apps, and sometimes it even gets stuck in the wrong app. Conclusion: it's horribly buggy, as expected, and I doubt it will ever get fixed properly (it's old and thus far remained a barely known niche feature), thus I see little point caring. |
This is the reason why the support is spotty between apps. Even still, within a single app they work perfectly, and this is my uses for it. |
|
I implemented stylus support for X11 which might be helpful in porting this to X11 (it looks like only windows is implemented right now) https://github.com/DorianRudolph/winit/tree/stylus |
|
Hi can I ask what the state of this pr is? I'm interested in using winit in a stylus-based project and so I'd be willing to help with this if needed. I tried suggesting an API in #3001 but this looks more comprehensive. |
|
I could say for sure that it's unlikely to get rebased 0x182d4454fb211940 , but you could use it as basis to sketch API. |
|
#3876 took care of it, to some extent. |
cargo fmthas been run on this branchcargo docbuilds successfullyCHANGELOG.mdif knowledge of this change could be valuable to usersThis PR is a draft for an updated event design that tries to fix the old event API design's numerous flaws. These include:
There are too many structural enum variants. Structural variants are annoyingly verbose to match on, and have been universally replaced with tuple variants.
Many events aren't flat. Several events contain other structures or enums that need to be imported separately to get matched on, which is an absolute PITA. Examples:
WindowEvent::MouseInputand want to check if the left mouse button was pressed, you need to import two other types (ElementStateandMouseButton). Those shouldn't be necessary.KeyboardInputorTouchtypes, which could be removed from the API completely.Extraneous types that need to get imported separately have been reduced to an absolute minimum.
Events expose useless data. Most
WindowEventshave aDeviceIdfield that just gets set to a meaningless values on all backends exceptX11, and an arbitrary set of events includeModifiersStatedespiteModifiersChangedexisting. Those have been removed.There's a lot of inconsistency and redundancy. "Mouse" and "Cursor" are used for different purposes, and it's difficult to remember which one is used where. Touch events and mouse events have completely distinct APIs, despite being used for the same thing in a lot of cases. "Touchpad pressure" and "touch pressure" are two entirely distinct events with no overlap. All of these events have been pulled into a consistent set of
Pointerevents.DeviceEventdoesn't have a design - it's a blob of variants thrown together into one thing. A "Device" isn't a good unit of abstraction, since there are several different device types with almost entirely orthagonal usecases.DeviceEventhas been split intoRawPointerEventandRawKeyboardEvent, and variants that were rarely emitted have been removed.DeviceEvent::Motionhas been removed, since the current design is pretty much unusable, it was only emitted on X11, and gamepad support is on the radar.Event::(Suspended|Resumed)don't really belong in the rootEventenum, and have been moved into anAppEventvariant.AppEventcould in theory be used in the future for additional application lifecycle events.WindowEvent's lifetime. Not being able to clone or buffer events is annoying, and the events that do need that lifetime should be quarantined off from the ones that don't. Lifetimed window events have been split intoWindowEventImmediate.I think that's a fairly comprehensive list. Please review the API and provide feedback if you have any issues.
Also,
PressFlagsare used instead of booleans in the button press events, since booleans are absolutely awful to match on when they're in tuple variants, as true and false doesn't have any inherent semantic meaning when casually reading the source code.