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

Handle pointerlock rejections, if promise based #13487

Merged
merged 2 commits into from
Feb 1, 2023

Conversation

852Kerfunkle
Copy link
Contributor

This should get rid of the unhandled top-level exception if requesting pointer lock fails.

I think the intended way to use pointer lock in Babylon is to sub to onCanvasBlurObservable and onCanvasFocusObservable, so element.focus() probably belongs into the resolved handler.

The alternative would be to focus regardless, but this is probably not desired:

if (promise instanceof Promise) promise.catch(() => {});
element.focus();

A good way to try this behaviour is to enter and exit pointerlock in quick succession on a chromium based browser.

@bjsplat
Copy link
Collaborator

bjsplat commented Jan 31, 2023

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@bjsplat
Copy link
Collaborator

bjsplat commented Jan 31, 2023

@852Kerfunkle
Copy link
Contributor Author

Well, it fails in a pretty unrelated test. To be honest: no idea.

Neither Engine.enterPointerlock nor Engine._RequestPointerlock should be called anywhere near this (unless these tests are somehow run in fullscreen, not that I could find anything like it).

Anecdotal evidence: I've been doing this for months now and no Safari user ever complained. I mean, they complain quite a bit, just not about this specifically. :)

It's not really important, this unhandled exception just kept bugging me.

@sebavan sebavan requested review from RaananW and Popov72 January 31, 2023 20:42
Copy link
Member

@RaananW RaananW left a comment

Choose a reason for hiding this comment

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

No modern browser is synchronous anymore. Chrome/edge from 92, firefox and iOS as well.
Great catch :-)

@RaananW RaananW merged commit 03125b1 into BabylonJS:master Feb 1, 2023
@852Kerfunkle 852Kerfunkle deleted the pointerlock-rejection branch February 1, 2023 14:09
@852Kerfunkle
Copy link
Contributor Author

No modern browser is synchronous anymore. Chrome/edge from 92, firefox and iOS as well. Great catch :-)

Last I checked firefox still returns undefined, but maybe that changed since I last checked. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants