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 lock options to requestPointerLock #49

Closed
wants to merge 15 commits into from

Conversation

NavidZ
Copy link
Member

@NavidZ NavidZ commented Jul 16, 2019

The goal of this change is to allow application to access unadjusted mouse movement data while in Pointer Lock. This is a highly requested feature from partners in the gaming space. This change adds a PointerLockOptions object as a parameter to the requestPointerLock() method. The PointerLockOptions object currently only has one useful member which is the boolean unadjustedMovement. Also, to properly return error information and make it easier for developers to implement the requestPointerLock method was changed to return a promise.

The existing Pointer Lock API returns the mouse movement that the platforms give Chrome. By default all platforms include some form of mouse acceleration in the movement information they give Chrome. Mouse acceleration is the artificial increase of velocity when the mouse is moving fast. This is useful in normal use of the pointer when a user is trying to move the mouse across the screen. However, it also makes aiming in first person perspective games very difficult. To solve this problem we are adding an option to get that unadjusted movement data when requesting pointer lock.

With Pointer Lock options now available, applications need the ability to change those options while keeping the lock. This proves difficult using the previous API which fired changed or error events on the document to indicate the result of the request. Particularly troublesome was that the error event gave no reason. To solve this problem for developers a Promise workflow is being introduced. Now when requesting a change the developers can get actionable error information on rejected requests.

Example:

try {
  await element.requestPointerLock({ unadjustedMovement: true });
  console.log(“pointer lock acquired”);
} catch (error) {
  console.log(`Failed to acquire pointer lock due to ${error}`);
}

Closes #36

The following tasks have been completed:

  • Modified Web platform tests (TBD)

Implementation commitment:


Preview | Diff

@NavidZ
Copy link
Member Author

NavidZ commented Jul 17, 2019

FYI @EiraGe

@NavidZ
Copy link
Member Author

NavidZ commented Jul 18, 2019

@BoCupp-Microsoft are you guys also interested in an API like this? Do you have any suggestions regarding the shape of the API?

@marcoscaceres What do you think of this API from Mozilla's perspective? Is that something you would be interested in implementing in FF? Also do you know who would be the best person from WebKit to ping about this?

Copy link
Member

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

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

Quick review and a couple of nits. Will try to get someone with knowledge in this area from Mozilla to let us know if this is something we want to add to Firefox. Will need a few days to get back to you.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@marcoscaceres
Copy link
Member

@rniwa or @hober perhaps can provide some input from WebKit’s perspective?

index.html Outdated Show resolved Hide resolved
@marcoscaceres
Copy link
Member

Applied suggestions and fixed merge conflict.

Blocked on getting a second implementation.

@NavidZ
Copy link
Member Author

NavidZ commented Aug 20, 2019

@marcoscaceres did you find people on Mozilla that can consult more regarding this? maybe @smaug----?

@NavidZ
Copy link
Member Author

NavidZ commented Aug 20, 2019

@rniwa do you have any comments regarding this proposal?

@marcoscaceres
Copy link
Member

Not yet, sorry. On it.

@NavidZ
Copy link
Member Author

NavidZ commented Aug 21, 2019

Also cc @juj as the requirement is very well expressed in @juj's comment

@marcoscaceres
Copy link
Member

I was told that @smaug---- is indeed the right person from Mozilla to comment here 🙇 I also know they are super busy, so hopefully we will hear back soon.

@smaug----
Copy link

(commented in the relevant issue #36 )

Copy link

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Really great to see this firming up! I left a first round of feedback that should get us started.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
brad4d pushed a commit to google/closure-compiler that referenced this pull request Jan 28, 2020
Experimental API proposed in w3c/pointerlock#49 and
implemented in Chromium (crbug/982379)

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=291899961
pull bot pushed a commit to FreddyZeng/chromium that referenced this pull request Mar 13, 2020
Previously if a pointer lock request was made while the pointer was
already locked it would update to the new element in the new request.
However, now that we have options we must also allow the subsequent
requests to change which options they are requesting for.

Relevant Spec change: w3c/pointerlock#49

Bug: 1043640
Change-Id: I777f6de156554254c7a5ffbefcbe32e9d0af504e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2071788
Commit-Queue: James Hollyer <jameshollyer@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#750032}
Copy link

@jameshollyergoogle jameshollyergoogle left a comment

Choose a reason for hiding this comment

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

Domenic, thanks for all the reviews. I tried my best. I am sure there is still more to do. Let me know what you think.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Copy link

@JamesHollyer JamesHollyer left a comment

Choose a reason for hiding this comment

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

Thanks again Domenic! Please check out these changes. I think it is starting to come along.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Copy link

@domenic domenic left a comment

Choose a reason for hiding this comment

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

The model is looking very solid now. What remains are details of the algorithm flow, plus a good deal of editorial nits. (I would encourage you to do a once-over of the PR for grammar/spelling/capitalization consistency.)

But since the model is solid, those changes should be relatively easy. Hooray!

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Copy link

@JamesHollyer JamesHollyer left a comment

Choose a reason for hiding this comment

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

Thanks again. Can you find any other things that need fixing?

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated
<code>screenY</code>, when the pointer is locked).
</p>
<ol>
<li>

Choose a reason for hiding this comment

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

Added.

index.html Outdated
If <a>pointer-lock target</a> is not in the same <a>shadow-including root</a> as <a>this</a>'s document, return.
</li>
<li>
if currently locked to a target whose <a>shadow-including root</a> is this document <a>exit pointer lock</a>

Choose a reason for hiding this comment

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

Done.

@reillyeon
Copy link
Member

This seems close to merging. @smaug----, can you take another look?

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Copy link

@JamesHollyer JamesHollyer left a comment

Choose a reason for hiding this comment

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

A few big mistakes in the last version. Hopefully this one looks better.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@ollop
Copy link

ollop commented May 28, 2021

Wondered if there is any progress on this. It is still not available by default in Chrome Version 91.0.4472.77 and the option to enable it is gone in 93.0.4525.0 canary. As we built something around "unadjustedMovement" I wondered if this will ever become available by default.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@domenic domenic removed their request for review May 7, 2024 02:03
index.html Show resolved Hide resolved
Copy link

@james-howard james-howard left a comment

Choose a reason for hiding this comment

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

I added one small grammar comment, but otherwise looks good to me.

index.html Outdated Show resolved Hide resolved
Copy link
Member

@mustaqahmed mustaqahmed left a comment

Choose a reason for hiding this comment

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

I am approving the PR but suggesting to add one more issue.

index.html Show resolved Hide resolved
@mustaqahmed
Copy link
Member

@marcoscaceres Is there an easy way to bypass or override the failing ipr check? @NavidZ is no longer a participant of WebApps WG.

@mustaqahmed
Copy link
Member

For sake of making real progress on this old PR, I will "resolve" any open conversation that has been commented as "done". After that we will re-examine what's left then move to a brand new PR to bypass the "ipr" check.

@mustaqahmed
Copy link
Member

Thanks @alvinjiooo for creating a new PR to be able to bypass "ipr" checks here. I will now close this PR and ask everyone to resume your review on #99 instead.

The "Changes requested" section above shows pending reviews from @marcoscaceres and @domenic. I spot-checked many comments there over the last few days, they seemed old and already resolved. I would request another quick re-review of #99 in case I missed something.

We are expecting to land #99 soon, before this PR hits its 5th anniversary 😞!

Thanks everyone here for your patient!!!

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

Successfully merging this pull request may close these issues.

Option needed to disable OS-level adjustment for mouse acceleration