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

Touchpad magnify and rotate events #8791

Merged
merged 10 commits into from
Jun 8, 2023

Conversation

jim-ec
Copy link
Contributor

@jim-ec jim-ec commented Jun 8, 2023

Objective

The goal of this PR is to receive touchpad magnification and rotation events.

Solution

Implement pendants for winit's TouchpadMagnify and TouchpadRotate events.

Adjust the mouse_input_events.rs example to debug magnify and rotate events.

Since winit only reports these events on macOS, the Bevy events for touchpad magnification and rotation are currently only fired on macOS.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 8, 2023

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Windowing Platform-agnostic interface layer to run your app in O-MacOS Specific to the MacOS (Apple) desktop operating system labels Jun 8, 2023
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The platform limitations should be listed in the docs for each struct, and mentioned in the example. Good addition though!

///
/// - Only available on **`macOS`**.
#[derive(Event, Debug, Clone, Copy, PartialEq, Reflect, FromReflect)]
#[reflect(Debug, PartialEq)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add FromReflect to this attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that already the case? It is the last argument to #[derive(...)].

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, to clarify I’m referring to adding a ReflectFromReflect registration like #[reflect(Debug, PartialEq, FromReflect)]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see. I've added it to #[reflect(...)] but the reason for it not being initially there is that none of the types in bevy_input::mouse use it. So its probably missing there as well, right? Or is that intentional?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it should be there as well. #8776 addresses that issue.

///
/// - Only available on **`macOS`**.
#[derive(Event, Debug, Clone, Copy, PartialEq, Reflect, FromReflect)]
#[reflect(Debug, PartialEq)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

derive(serde::Serialize, serde::Deserialize),
reflect(Serialize, Deserialize)
)]
pub struct Magnify(pub f32);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these also be prefixed with Touchpad for clarity?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved these types into their own module as I think that this is more future proof.

Copy link
Member

@MrGVSV MrGVSV Jun 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like they should still be renamed to tie them back to the touchpad. Rotate especially is pretty generic and could be confusing to some users.

Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Also tested on my MacBook and it seems to be working!

@mockersf mockersf added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Windowing Platform-agnostic interface layer to run your app in C-Feature A new feature, making something new possible O-MacOS Specific to the MacOS (Apple) desktop operating system S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants