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

feat(#3759): Implements Apple Pencil double tap functionality #3768

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ActuallyHappening
Copy link

@ActuallyHappening ActuallyHappening commented Jun 29, 2024

Implements #3759
Also see #99
Based on the master branch

  • Tested on all platforms changed
  • Added an entry to the changelog module if knowledge of this change could be valuable to users
  • Updated 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
  • Updated feature matrix, if new features were added or implemented

Ported from #3763, relevant sections pasted below:

I have tested these changes on my iPad and they work exactly as expected. In an ideal world, I would like this to be released on crates.io in a way that bevy could integrate so I could use it in my bevy application (bevyengine/bevy#14060)

Also, the public API I have chosen seems reasonable to me but I am totally open to it being tweaked or re-worked. But I would ideally want this functionality exposed in some way without me having to maintain my own fork as bevy releases roll around. I have intentionally included #[non_exhaustive] in places that I or other developers may add or improve features to. Notably, this PR only handles pen double tap events since that is all I can physically test with my current hardware, but newer Apple Pencil models support squeeze actions that I know (theoretically) how to implement but haven't. The API intentionally allows for this addition later.
The public API I added is also intended to be extendable by other platform implementations, but again I can't test them physically so haven't included here. Each section has ## Platform Specific \n - iOS only documentation for that reason.

Also, I have renamed the API name from PenEvent -> PenSpecialEvent (can refactor to SpecialPenEvent I don't mind) to disambiguate these events from the generic pen events as discussed in #99

To re-iterate why I don't think these can be implemented as part of the Web API in #99, these sorts of events are not associated with a screen tap or specific touch gesture. I want the ability to handle these events either way they are implemented however, so happy to help in any way to get this functionality into a release usable from bevy

@ActuallyHappening
Copy link
Author

The windows nightly checks are failing for a completely unrelated reason

Copy link
Member

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

Shouldn't this be supported on MacOS as well?

See #99 (comment) as well. Though I'm not entirely against just going ahead here, I would prefer if we implement barrel and eraser first and plan ahead a bit so we don't introduce another completely platform-specific event before moving WindowEvent to traits and allowing platform-specific extensions.

@@ -350,6 +392,29 @@ impl WinitView {
this.setContentScaleFactor(scale_factor as _);
}

// adds apple pencil support
// let view: Retained<UIView> = self.view().expect("View has already loaded");
Copy link
Member

Choose a reason for hiding this comment

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

Leftover?

Copy link
Author

Choose a reason for hiding this comment

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

That is leftover, because the apple docs recommend retrieving the view like the commented code

// let view: Retained<UIView> = self.view().expect("View has already loaded");
let interaction: Retained<UIPencilInteraction> = {
let allocated: Allocated<UIPencilInteraction> =
MainThreadMarker::new().unwrap().alloc();
Copy link
Member

Choose a reason for hiding this comment

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

Why aren't you using the MainThreadMarker passed into this function by mtm?

Copy link
Author

Choose a reason for hiding this comment

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

Good catch thanks

@ActuallyHappening
Copy link
Author

Shouldn't this be supported on MacOS as well?

You can't connect Apple Pencils to MacOS, even using Mac Catalyst. If you can I don't know about it and would love to play around!

See #99 (comment) as well. Though I'm not entirely against just going ahead here, I would prefer if we implement barrel and eraser first and plan ahead a bit so we don't introduce another completely platform-specific event before moving WindowEvent to traits and allowing platform-specific extensions.

Hold on a minute, you can define platform specific events? That fits this PR way better than adding the current API, since this PR's API must be platform-independent but has only one implementation!

About barrel and eraser, I don't know what those are and doubt I have the hardware for it anyway since I only have an Apple Pencil Gen 2. I don't really want to implement too much more than this since it is my original use case and all I can physically test at the moment, but I could certainly add it if it will help this functionality get merged. By barrel and eraser your referring to windows stylus pens (among others)?
Reading the spec I really can't find any mention of handling PenSpecialEvent::DoubleTap anywhere in any form, though I definitely might have missed something.

@daxpedda
Copy link
Member

You can't connect Apple Pencils to MacOS, even using Mac Catalyst. If you can I don't know about it and would love to play around!

Its possible through Apple Sidecar or third party alternatives like Duet Display and Luna Display.

Hold on a minute, you can define platform specific events?

Unfortunately not, I was speaking about future changes to the API we have planned, where we could merge something like this without much of a second thought.

About barrel and eraser, I don't know what those are and doubt I have the hardware for it anyway since I only have an Apple Pencil Gen 2. I don't really want to implement too much more than this since it is my original use case and all I can physically test at the moment, but I could certainly add it if it will help this functionality get merged. By barrel and eraser your referring to windows stylus pens (among others)?

I think there is a misunderstanding. My proposal is to implement a basic pen API that follows the Web API spec before implementing platform specific functionality so we know how and where to fit it in the API.

Reading the spec I really can't find any mention of handling PenSpecialEvent::DoubleTap anywhere in any form, though I definitely might have missed something.

Indeed, which is why its platform specific.


I'm planning to make a PR that implements the basic pen API for Web today. So hopefully we will have a clearer picture afterwards.

This was referenced Jul 22, 2024
@madsmtm madsmtm self-assigned this Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants