-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Pen/Stylus support #4287
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
Pen/Stylus support #4287
Conversation
|
Also it seems that on_touch_cancel in web's pointer.rs is neither assigned nor used anywhere. Should I remove it here or in a follow-up? |
winit-core/src/event.rs
Outdated
| ///  | ||
| /// | ||
| /// <sub> | ||
| /// For image attribution, see the | ||
| /// <a href="https://github.com/rust-windowing/winit/blob/master/docs/res/ATTRIBUTION.md"> | ||
| /// ATTRIBUTION.md | ||
| /// </a> | ||
| /// file. | ||
| /// </sub> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
local path can be used for docs, IIRC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried and well.. seems like I can't :c
| #[cfg_attr(feature = "serde", derive(Deserialize, Serialize))] | ||
| pub struct ToolState { | ||
| /// The force applied to the tool against the surface. | ||
| pub force: Force, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it a pressure from the https://wayland.app/protocols/tablet-v2#zwp_tablet_tool_v2:event:pressure , maybe pressure is a better name in general?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm.. I just looked, W3C also uses pressure for their force, but the winit type is already named Force and is shared with touch so idk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is Touch and Preassure in the same units? I'd probably use different types if they are not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well yeah - they're not just units, it's an enum with either calibrated values or normalized pressure between 0 and 1 for max strength, so yeah they share the same logic.
I could however add a doc alias name if you like that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've now made Force have a doc alias to Pressure. @madsmtm searching through winit docs, there is mac's TouchpadPressure event with a pressure: f32 field, couldn't that also be made a Force?
you can, but not sure if someone would be able to actually review x11 part. We can split it out. Could you check how wayland spec maps to the w3c spec? I think wayland one is more broad so we might have more stuff? I can generally add Wayland. As for the PR itself and merging it, I think we really need |
I'm trying to get the base here through CI, then I could look what can be done for the other backend, but what do you mean by "we ripped windows"? O.o One thing I for sure want to get is apple pencil support for uikit and I can also test that. |
| let Some(DeviceType::Mouse) = self | ||
| .devices | ||
| .borrow() | ||
| .get(&mkdid(event.sourceid as xinput::DeviceId)) | ||
| .map(|device| device.r#type) | ||
| else { | ||
| return; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daxpedda Can you explain why you have done that? I can't quite follow..
| if button != 0 { | ||
| tracing::error!("unexpected touch button id: {button}"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Original had != 1 but W3C says Touch Contact would be 0
It used to work before due to some common handling in terms of just going through touch iirc, but now there's no tablet at all, so to not regress we need it back. The rest of the backends are entirely optional for e.g. next release, etc. |
|
Ok so I looked and the X11 backend for sure isn't done yet. |
- This adds pen/stylus support. - Move `Force::Calibrated::altitude_angle` to `ToolAngle::altitude`. - `Force::normalized()` now takes a `Option<ToolAngle>` to calculate the perpendicular force. - Just introducing types, no implementation yet!
Add a new `CursorButton` and `ToolButton` type.
Fixed: device events are emitted regardless of cursor type.
|
Hmm I'm a bit unsure about the Tool -> Stylus change while reading the web and wayland specs.. should I revert it? |
|
Yes, to |
kchibisov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll give more deep review in terms of API when I'll try to implement Wayland part. For now, it's not really clear for me how to map things (since no browser maps wayland yet) so will take some time, I guess.
| /// **macOS:** Unsupported. | ||
| Touch(FingerId), | ||
| Stylus(StylusTool), | ||
| Tool(ToolKind), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefix everything with Tablet please, since it's not clear what tool otherwise(As I initially requested).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thought was that after a quick search, i didn't find much about how other platforms handle that "tablet pad" buttons and thought I may need to combine tool and pad buttons, but even then a tablet prefix would be smart so yeah I'm gonna change that when I'm back at my pc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without Tablet prefix it's not clear what Tool it is, since Tool is rather a broad term I'd say, that's why I've kind of demanded it, not only because of TabletPad.
|
Opened #4318 which addresses some issues and adds windows/Wayland. |
|
Sorry, I didn't have much time because I had to study but had planned to integrate Apple Pencil support next week or so. Glad to see this has still progress and I also feel easier seeing this in better hands than mine. Let's see if I don't forget to add apple pencil support to your continuation then ^^' |
|
Apple stuff don't block things, we just needed Good luck with studies. |
Adopted #3810
I've made some cleanup to the Web Implementation, and a hovering eraser should now just be treated as that instead of a eraser button push.
The structure now looks like this:
Web Backend
X11 Backend
Wayland Backend (@kchibisov)
UiKit Backend
Tested on all platforms changed
Added an entry to the
changelogmodule if knowledge of this change could be valuable to usersUpdated documentation to reflect any user-facing changes, including notes of platform-specific behavior
Created or updated an example program if it would help users understand this functionality