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

Pointer API Sketch #3001

Closed

Conversation

0x182d4454fb211940
Copy link

  • Tested on all platforms changed (No. Only wayland tested)
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users (No)
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior (No)
  • Created or updated an example program if it would help users understand this functionality (No)
  • Updated feature matrix, if new features were added or implemented (No)

This is a very rough sketch of what a pointer-based API could look like (based on #336). Personally I'm interested in moving winit towards an API that can support pen input, which is why I'm interested in this, and I'm willing to help implement it. But I'd like to make sure the WindowEvent API matches what the winit team wants and what would be useful and ergonomic for the user, so please let me know your thoughts!

It's worth noting that by heading in this direction we will be breaking apps which already use cursor and touch events. Would it be better to just add a "pen" event like with touch?

@0x182d4454fb211940 0x182d4454fb211940 marked this pull request as ready for review August 4, 2023 11:02
@daxpedda
Copy link
Member

daxpedda commented Aug 4, 2023

I don't think we need all these different types, I think having one enum that can either be Mouse/Touch/Pen with the appropriate data attached to all events should work.

Generally speaking I'm in favor of unifying all three into one event though, it also fits well with the Pointer API on Web.

@0x182d4454fb211940
Copy link
Author

The reason I went with the extra types is some states don't make sense. For example, a PointerInput will never have both PointerSource::Touch { .. } and button: Some(_). But I guess there's no need to be overly correct, especially if it's easier to use.

@daxpedda
Copy link
Member

daxpedda commented Aug 4, 2023

The reason I went with the extra types is some states don't make sense. For example, a PointerInput will never have both PointerSource::Touch { .. } and button: Some(_). But I guess there's no need to be overly correct, especially if it's easier to use.

Yeah, you are right, maybe these types made sense after all? Let's wait for more input.
I would prefer types to represent mutually exclusive stuff indeed, so your previous implementation actually makes more sense to me.

@kchibisov
Copy link
Member

There was an attempt to do it here #1374 before.

@daxpedda
Copy link
Member

daxpedda commented Aug 4, 2023

I don't think I like splitting up all the information in separate events, but it makes remove the mess of having so many different types.

@0x182d4454fb211940
Copy link
Author

0x182d4454fb211940 commented Aug 5, 2023

Some thoughts and questions while designing this version:

  • In Experiment with an improved event design #1374, what is PointerContactArea? Is that necessary for a first implementation?
  • I renamed twist to angle (perhaps rotation would be better?) since I found twist a bit of strange name. What name would be best? Should that value be newtyped?
  • On Wayland at least, I know the tablet api sends a "frame" event once all information (pressure, tilt, position) has been sent, this might be a useful thing to add but I don't know how well support would work across platforms. (Thinking about it I'd expect it to work, either they send the event in parts like wayland, and then they sort of have to say when it ends, or they send the event as a whole, and then we can just send a frame event when all the information has been sent).
  • The internals of PointerId will probably want to be hidden as in Experiment with an improved event design #1374.
  • We might want to send a "Capabilties" enum alongside the "created" event or just after. I don't know what that would look like cross-platform, but I suppose we could simulate it by reporting capabilities as the info comes in. Would this even be useful?
  • If you squint your eyes, this api seems to be capable of expressing macOS touchpad events too (rotation, pressure, could add an event for magnification)... as well as this could scrolling become part of the pointer api?
  • As you say @daxpedda it is unfortunate that all the info gets split. I think it would probably be quite easy to make some kind of struct which keeps track of the state that comes out of these events, but maybe that's what winit should be doing in the first place. Should this be a kind of "middle api", which the user can opt in to? (so we first unify all platforms in this simple event system, then express more abstract events, closer to what winit currently does)

(I didn't bother updating the wayland implementation, there's not really any point right now)

@daxpedda
Copy link
Member

daxpedda commented Aug 5, 2023

I think that's #2942, so no, it's not necessary for a first implementation.

  • I renamed twist to angle (perhaps rotation would be better?) since I found twist a bit of strange name. What name would be best? Should that value be newtyped?

We don't currently support twist/angle/rotation, so I would argue we can leave this for another PR.

  • On Wayland at least, I know the tablet api sends a "frame" event once all information (pressure, tilt, position) has been sent, this might be a useful thing to add but I don't know how well support would work across platforms. (Thinking about it I'd expect it to work, either they send the event in parts like wayland, and then they sort of have to say when it ends, or they send the event as a whole, and then we can just send a frame event when all the information has been sent).

I can't really comment on that, I only maintain the Web backend where this doesn't happen, but I think we can figure that out when we get to the implementation phase.

in #1374 PointerId was what we call DeviceId now. So I don't think they are comparable. It might be interesting to explore if we can can fold this two types together, but I don't particularly see the point of that.

  • We might want to send a "Capabilties" enum alongside the "created" event or just after. I don't know what that would look like cross-platform, but I suppose we could simulate it by reporting capabilities as the info comes in. Would this even be useful?

Depends on what exactly this would entail but it doesn't sound very appealing to me, in any case, I think this is a separate problem we can figure out outside this PR.

  • If you squint your eyes, this api seems to be capable of expressing macOS touchpad events too (rotation, pressure, could add an event for magnification)... as well as this could scrolling become part of the pointer api?

Probably not a bad idea, again though, imo we can figure this out later, cc #2963.

  • As you say @daxpedda it is unfortunate that all the info gets split. I think it would probably be quite easy to make some kind of struct which keeps track of the state that comes out of these events, but maybe that's what winit should be doing in the first place. Should this be a kind of "middle api", which the user can opt in to? (so we first unify all platforms in this simple event system, then express more abstract events, closer to what winit currently does)

I think this is really over-complicating it. Maintainers just have to make a decision here.

@kchibisov I would strongly prefer to send as much information as possible in a single event instead of splitting it up like #1374 did. So I prefer the original implementation of @0x182d4454fb211940: 71125f9. Hopefully we can clean this up a bit to reduce the amount of types, but that's what I would currently prefer, WDYT?

(I didn't bother updating the wayland implementation, there's not really any point right now)

Don't waste your time on it, we can figure it out after we finish the design phase.

@kchibisov kchibisov added this to the Version 0.30.0 milestone Aug 11, 2023
@0x182d4454fb211940
Copy link
Author

0x182d4454fb211940 commented Aug 13, 2023

So in the meantime I've made a rough fork of winit and bevy to start working on my project. What worked there has given me another potential idea: splitting the data from the event.

let event_loop = EventLoop::new();
let window = WindowBuilder::new().build(&event_loop).unwrap();

event_loop.run(move |event, _, control_flow| {
    control_flow.set_wait();

    match event {
        Event::WindowEvent {
            event: WindowEvent::PointerDown(PointerId::Pen(id)),
            ..
        } => {
            println!("pen is down, with pressure of {}", window.pen(id).pressure.unwrap_or(f64::NAN));
        },
        Event::WindowEvent {
            event: WindowEvent::PointerDown(PointerId::Mouse(id)),
            ..
        } if window.mouse(id).just_pressed(MouseButton::Left) => {
            println!("you just clicked at {:?}", window.mouse(id).position());
        },
        Event::WindowEvent {
            event: WindowEvent::PointerUp(id),
            ..
        } => {
            println!("pointer {id:?} just lost contact with the screen at {:?}", window.pointer(id).position());
        },
        /* snip */
    }
});

This is quite radically different from how winit currently does things, and I'm not sure if this is better (it would either require something which pre-processes events or some kind of cross-module interior mutability). But it does solve the problem of representing attached data correctly (in order to access the extra data, you need to use the id with the correct type), and would also mean that someone who wants to check if the user clicked in a certain area, they could just access the mouses position alongside the event. I've found a similar (integrated with bevy) approach to work quite well for my project.

@kchibisov
Copy link
Member

See #3876.

@kchibisov kchibisov closed this Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants