Closed Bug 782729 Opened 12 years ago Closed 12 years ago

Pointer lock does not work on FPS "PlayCanvas" demo

Categories

(Core :: DOM: Core & HTML, defect)

x86
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla18
Tracking Status
firefox15 - ---
firefox16 - affected
firefox17 - affected
firefox32 --- verified

People

(Reporter: azakai, Assigned: cpearce)

References

(Blocks 1 open bug, )

Details

(Keywords: regression, Whiteboard: [games:p2])

Attachments

(3 files, 3 obsolete files)

http://apps.playcanvas.com/playcanvas/scifi/latest is an FPS demo. Press space to go to fullscreen. In that mode, the mouse has no effect on Linux, but people tell me it works on other platforms, so seems platform-specific.
Blocks: gecko-games
Note that the mouse does seem to work while the permission dialog is shown.
Are you running two monitors? And/or nvidia twin view?
Nothing fancy on my system, just a macbook running linux. No external monitors. I have ATI and their binary drivers enabled. bjacob said he also saw this bug, not sure what his setup is.
I have the same issue on a Thinkpad W520, Ubuntu 12.04 64bit, NVIDIA. No external monitor or pointing device.
Assignee: nobody → cpearce
Nightly 2012-06-21-05-30-48 pointer lock doesn't work on either my primary or secondary monitor. Nightly 2012-06-20-06-51-38 pointer locks works on my secondary monitor but not on my primary monitor. Therefore I assume the regression range is: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c3190d715044&tochange=10e019421e6b It seems it's a safe assumption that regressor is these changesets: Wed Jun 20 16:38:48 2012 -0700 d047caf12f04 Chris Pearce — Bug 756936 - Minor tidy ups to pointer lock code. r=smaug 4be7471e7a95 Chris Pearce — Bug 756936 - Ensure MouseEvent.mozMovement{X,Y} values are correct on secondary monitors. r=smaug
Whiteboard: [games:p2]
It would be good to have this for fx16.
We are not sure on 15 and 16 status. The regression seems in range of 16 so it seems likely that it's affected.
We're already about to build our final RC for FF15, so not tracking for that. Will track for 16, since this is a regression.
Chris - can you investigate and suggest a backout, fix, or wontfix for FF16? This affects the bananabread demo.
We should backout Bug 756936 on beta, and fix this bug on Aurora. Bug 756936 fixes pointer lock in a few edge cases, notably when there is multiple monitors on Windows. AFAICT there's been no-one complaining about the issues Bug 756936 fixes, so I think we can live without Bug 756936's changes in release for another six weeks.
Status: NEW → ASSIGNED
Attached patch Backout bug 756936 on beta for Firefox 16 (obsolete) (deleted) — Splinter Review
[Approval Request Comment] Bug caused by (feature/regressing bug #): 756936 User impact if declined: Pointer lock won't work on Linux. However with this patch pointer lock won't work on secondary monitors (on Windows and Linux, maybe Mac too), but we had this behaviour in FF14 and no one complained (bug 756936 shipped in FF15, fixing pointer lock on secondary monitors but breaking linux pointer lock). Testing completed (on m-c, etc.): mochitests pass locally. They passed before bug 756936 landed, so once we back it out they should still pass. Risk to taking this patch (and alternatives if risky): Low, since this is a backout... String or UUID changes made by this patch: None.
Attachment #660712 - Flags: approval-mozilla-beta?
(In reply to Alex Keybl [:akeybl] from comment #9) > Chris - can you investigate and suggest a backout, fix, or wontfix for FF16? > This affects the bananabread demo. Hmm, I doublechecked and actually it looks like this does *not* affect other demos than the one reported here. BananaBread is ok, as is http://media.tojicode.com/q3bsp/ , I checked on 16, 17 and 18. So the problem appears specific to the playcanvas demo for some unknown reason. Given that, I would recommend not making any changes to aurora or beta. But it would be good to find out why that demo does not work on nightly even though others do (and since the demo does work in other browsers).
Hmmm, indeed my pointer lock demo at http://pearce.org.nz/fullscreen/pointerlock-position.html works ok in Linux, but *only* when I don't have a secondary monitor active. I'll cancel the approval request until we can figure out what's going on here...
Attachment #660712 - Flags: approval-mozilla-beta?
I will fix the issue of pointer lock not working on Linux with multiple displays in bug 791121, and we'll leave this one for tracking down why http://apps.playcanvas.com/playcanvas/scifi/latest doesn't work with pointer lock on Linux.
This is not Linux specific, PlayCanvas also doesn't work for me in [Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0]. Please disregard the regression range I reported in comment 5, that's a regression range for the problem with Linux and pointer lock with multiple monitors. I misunderstood the bug report here. Sorry about that.
Summary: Pointer lock does not work in Linux on FPS demo → Pointer lock does not work on FPS "PlayCanvas" demo
I take it back. After re-doing a regression range search on Windows using the PlayCanvas demo as a test case I have discovered that this bug has the same regression range I reported in comment 5, i.e. the regression range is https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c3190d715044&tochange=10e019421e6b Thus it's probably a regression from Bug 756936.
Confirmed this with PlayCanvas on Linux too. It's definitely a regression from Bug 756936's 4be7471e7a95 changeset.
Since this only affects a single demo (as opposed to pointer lock on Linux in general), we're no longer going to track for release.
Attachment #660712 - Attachment is obsolete: true
The problem with the PlayCanvas demo was caused by removing this if branch in 4be7471e7a95: https://hg.mozilla.org/integration/mozilla-inbound/rev/4be7471e7a95#l1.65 With that if-branch resurrected, plus all calls to SynthesizeNativeMouseMove in screen coords, and with Windows' SynthesizeNativeMouseMove implementation altered to accept screen coords, pointer lock appears to work properly; the PlayCanvas demo works fine, on multiple monitors, and bug 782777 also seems to be fixed. Patches to follow...
I was wrong in changeset 4be7471e7a95 to switch to passing widget coords to nsIWidget::SynthesizeNativeMouseMove(). We should pass in screen coords. All the nsIWidget::Synthesize* methods are documented that they take screen coords, and the widget backends expect screen coords (except for Windows, which is a bug that I'm going to fix in the next patch). See Bug 791121 comment 2 for more info. With this patch pointer lock now behaves correctly on non-Windows platforms with multiple monitors. This will break Windows pointer lock on secondary monitors without the next patch.
Attachment #662498 - Flags: review?(bugs)
In changeset 4be7471e7a95 I removed this branch: https://hg.mozilla.org/integration/mozilla-inbound/rev/4be7471e7a95#l1.65 I removed it because I was never able to observe it having any affect. Guess I was wrong. Removing this branch caused the PlayCanvas demo to break.
Attachment #662500 - Flags: review?(bugs)
The other widget backends' implementation of SynthesizeNativeMouseMove() accept a coord in device pixels, but the Windows implementation is accepting it in widget coords (relative to the window's top left). All implementations should accept coords in the same coordinate system, otherwise cross platform code will be wrong.
Attachment #662502 - Flags: review?(bugs)
Attachment #662498 - Flags: review?(bugs) → review+
Comment on attachment 662500 [details] [diff] [review] Patch 2 v1: Resurrect pointer lock refPoint restore on centering Trying to figure out the reason for this...
Comment on attachment 662502 [details] [diff] [review] Patch 3 v1: Make Windows nsWindow::SynthesizeNativeMouseEvent accept coords in screen coords, not widget coords This needs a review from Windows widget peer.
Attachment #662502 - Flags: review?(jmathies)
Attachment #662502 - Flags: review?(bugs)
Attachment #662502 - Flags: review+
Comment on attachment 662500 [details] [diff] [review] Patch 2 v1: Resurrect pointer lock refPoint restore on centering Hmm, shouldn't we actually check that if refPoint == sLastRefPoint and don't dispatch anything in that case. We do want to dispatch 0,0 movement when mouse is back to center, right? Re-ask review if I'm wrong here :) The code needs some comments here.
Attachment #662500 - Flags: review?(bugs) → review-
Comment on attachment 662502 [details] [diff] [review] Patch 3 v1: Make Windows nsWindow::SynthesizeNativeMouseEvent accept coords in screen coords, not widget coords This has been this way since it landed in 09, so please run through try to confirm this doesn't break any tests.
Attachment #662502 - Flags: review?(jmathies) → review+
The previous "refPoint restore" code which I was resurrecting in my previous patch was confusing, so I changed the code to do what I *think* it meant to do, and what makes sense to me, and added I comments to explain how it works. I explicitly look out for and cancel the "synthetic native" mouse event that we dispatch to re-center the pointer, and this fixes the PlayCanvas demo, and bug 782777 as well. Explicitly tracking when we should cancel the mouse event also prevents us from cancelling mouse events that are targeted at the center of the window and which follow a pointer-lock re-centering mouse move (earlier versions of *this* patch had mochitests failures because the pointerlock mochitests dispatch synthetic mouse moves to the center of the window, I don't know if existing pointer lock test orange is/was caused by this). Greenish: https://tbpl.mozilla.org/?tree=Try&rev=a2eed6362fc4
Attachment #662500 - Attachment is obsolete: true
Attachment #663914 - Flags: review?(bugs)
Comment on attachment 663914 [details] [diff] [review] Patch 2 v2: Cancel synth centering pointer moves, comment code. Why do we set sSynthCenteringPoint = center; after dispatching the synth event?
...and not before.
So we know to expect a synthetic event, so that we can cancel the next event (which is dispatched asynchronously based on my testing). It can be set before dispatching I imagine. Some of the mochitests were failing because they also dispatch synthetic events to the center of the window, and we were cancelling them.
Attachment #663914 - Flags: review?(bugs) → review-
Setting sSynthCenteringPoint before the synthetic centering move dispatch.
Attachment #663914 - Attachment is obsolete: true
Attachment #664221 - Flags: review?(bugs)
Attachment #664221 - Flags: review?(bugs) → review+
Verified fixed 32.0a1 (2014-05-18), Ubuntu 13.04
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: