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

Add cursor grab for web target #2025

Merged
merged 5 commits into from Jan 5, 2022

Conversation

TotalKrill
Copy link
Contributor

  • Tested on all platforms changed patched winit, tried it in bevy_winit, works
  • Compilation warnings were addressed cargo build --target wasm32-unknown-unknown
  • cargo fmt has been run on this branch
  • cargo doc builds successfully
  • Added an entry to CHANGELOG.md 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

@cwfitzgerald
Copy link

cwfitzgerald commented Nov 15, 2021

I have implemented this independent of winit, and enabling pointer grab causes this call to throw a nasty error: https://github.com/rust-windowing/winit/blob/master/src/platform_impl/web/web_sys/canvas/pointer_handler.rs#L82-L84. We likely would need to disable this call when cursor grab is enabled.

scene-viewer.js:347 panicked at 'Failed to set pointer capture: JsValue(InvalidStateError: Failed to execute 'setPointerCapture' on 'Element': InvalidStateError

@TotalKrill
Copy link
Contributor Author

Would not the correct way be to not have an expect in fallible code?

The other way is if we could accurately predict that the state was actually that the pointer is already locked, that would solve your specific case, but since I do not know which ways that the pointerlock function can fail, its not a very good fix.

I mean, unfortunately in web, elements are shared in an unsafe manner, making it hard to have full control. As you say, you can do JS calls on the element and rust code will not ( i believe ) have a chance to react to it?

@cwfitzgerald
Copy link

That sounds like a reasonable plan, I'll fork winit tonight and disable the expect to see what happens.

@cwfitzgerald
Copy link

@TotalKrill I just checked out your branch and disabled the expect there, and everything works as we expect. So with that one change I think this PR is ready to go.

@maroider
Copy link
Member

So with that one change I think this PR is ready to go.

+ resolving the merge conflict for CHANGELOG.md

@TotalKrill
Copy link
Contributor Author

TotalKrill commented Nov 18, 2021

Im actually not entirely sure just removing the expect, I rather dislike silent errors

Would a log::error! be okay to add after the set_pointer_capture() call? like this:

                let e = canvas.set_pointer_capture(event.pointer_id());
                if !e.is_ok() {
                    log::error!("{:?}", e);
                }

@TotalKrill TotalKrill force-pushed the cursor-grab branch 2 times, most recently from ad4d1e2 to 4f11b67 Compare November 18, 2021 21:08
@cwfitzgerald
Copy link

cwfitzgerald commented Nov 18, 2021

The issue with a log is that the error will occur every single time you click the mouse when the cursor is grabbed. That would unnecessarily spam the logs. I don't think there's a situation where this could fail that we really care that it fails.

@maroider maroider self-requested a review November 19, 2021 15:58
@maroider maroider mentioned this pull request Nov 20, 2021
9 tasks
@TotalKrill
Copy link
Contributor Author

I added a comment there and modified the commit message to be a bit more explicit

src/window.rs Outdated Show resolved Hide resolved
@maroider
Copy link
Member

maroider commented Dec 27, 2021

After coming back to this PR, I've come to the conclusion that the PR is fine as-is, although I'd like to test it myself before approving it.

@maroider maroider self-requested a review December 27, 2021 21:13
@maroider maroider mentioned this pull request Jan 2, 2022
3 tasks
Copy link
Member

@maroider maroider left a comment

Choose a reason for hiding this comment

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

Seems to work fine when I tested it.

@ArturKovacs
Copy link
Contributor

Note that the chagelog entry will end up under the wrong release after merging.

I suggest merging the latest master into this branch and then moving your changelog entry to the topmost section.

    code would crash if handling pointer capture outside of winit on
    every mouse click

    we swallow the error since we could not think of a case where this
    would fail in a way that would want to handle that it fails
@maroider
Copy link
Member

maroider commented Jan 5, 2022

I rebased the branch on top of master to get rid of the merge conflicts. Hope you don't mind :)

I'll merge this in a bit.

@maroider maroider merged commit c5c99d2 into rust-windowing:master Jan 5, 2022
@TotalKrill
Copy link
Contributor Author

I definitely don't mind, in fact I am thankful 👍

@TotalKrill TotalKrill deleted the cursor-grab branch January 5, 2022 11:03
@TotalKrill TotalKrill restored the cursor-grab branch January 5, 2022 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C - waiting on maintainer A maintainer must review this code DS - web
Development

Successfully merging this pull request may close these issues.

None yet

6 participants