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)
Tracking
()
VERIFIED
FIXED
mozilla18
People
(Reporter: azakai, Assigned: cpearce)
References
(Blocks 1 open bug, )
Details
(Keywords: regression, Whiteboard: [games:p2])
Attachments
(3 files, 3 obsolete files)
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jimm
:
review+
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•12 years ago
|
Blocks: gecko-games
Reporter | ||
Comment 1•12 years ago
|
||
Note that the mouse does seem to work while the permission dialog is shown.
Assignee | ||
Comment 2•12 years ago
|
||
Are you running two monitors? And/or nvidia twin view?
Reporter | ||
Comment 3•12 years ago
|
||
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.
Comment 4•12 years ago
|
||
I have the same issue on a Thinkpad W520, Ubuntu 12.04 64bit, NVIDIA. No external monitor or pointing device.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → cpearce
Assignee | ||
Comment 5•12 years ago
|
||
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
Updated•12 years ago
|
Whiteboard: [games:p2]
Comment 6•12 years ago
|
||
It would be good to have this for fx16.
Updated•12 years ago
|
status-firefox15:
--- → affected
status-firefox16:
--- → affected
status-firefox17:
--- → affected
tracking-firefox15:
--- → ?
tracking-firefox16:
--- → ?
tracking-firefox17:
--- → ?
Keywords: regression
Comment 7•12 years ago
|
||
We are not sure on 15 and 16 status. The regression seems in range of 16 so it seems likely that it's affected.
status-firefox15:
affected → ---
status-firefox16:
affected → ---
Comment 8•12 years ago
|
||
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.
status-firefox16:
--- → affected
Comment 9•12 years ago
|
||
Chris - can you investigate and suggest a backout, fix, or wontfix for FF16? This affects the bananabread demo.
Assignee | ||
Comment 10•12 years ago
|
||
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.
Assignee | ||
Updated•12 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 11•12 years ago
|
||
[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?
Reporter | ||
Comment 12•12 years ago
|
||
(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).
Assignee | ||
Comment 13•12 years ago
|
||
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...
Assignee | ||
Updated•12 years ago
|
Attachment #660712 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 14•12 years ago
|
||
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.
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 15•12 years ago
|
||
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
Assignee | ||
Comment 16•12 years ago
|
||
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.
Assignee | ||
Comment 17•12 years ago
|
||
Confirmed this with PlayCanvas on Linux too. It's definitely a regression from Bug 756936's 4be7471e7a95 changeset.
Comment 18•12 years ago
|
||
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.
Assignee | ||
Updated•12 years ago
|
Attachment #660712 -
Attachment is obsolete: true
Assignee | ||
Comment 19•12 years ago
|
||
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...
Assignee | ||
Comment 20•12 years ago
|
||
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)
Assignee | ||
Comment 21•12 years ago
|
||
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)
Assignee | ||
Comment 22•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #662498 -
Flags: review?(bugs) → review+
Comment 23•12 years ago
|
||
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 24•12 years ago
|
||
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 25•12 years ago
|
||
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 26•12 years ago
|
||
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+
Assignee | ||
Comment 27•12 years ago
|
||
Green on Try, modulo random orange:
https://tbpl.mozilla.org/?tree=Try&rev=93b00a03d636
Assignee | ||
Comment 28•12 years ago
|
||
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 29•12 years ago
|
||
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?
Comment 30•12 years ago
|
||
...and not before.
Assignee | ||
Comment 31•12 years ago
|
||
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.
Updated•12 years ago
|
Attachment #663914 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 32•12 years ago
|
||
Setting sSynthCenteringPoint before the synthetic centering move dispatch.
Attachment #663914 -
Attachment is obsolete: true
Attachment #664221 -
Flags: review?(bugs)
Updated•12 years ago
|
Attachment #664221 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 33•12 years ago
|
||
Comment 34•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/74a9ba7336cf
https://hg.mozilla.org/mozilla-central/rev/1f87f13f8da4
https://hg.mozilla.org/mozilla-central/rev/a88b3cd6f28d
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Comment 35•11 years ago
|
||
Verified fixed 32.0a1 (2014-05-18), Ubuntu 13.04
Status: RESOLVED → VERIFIED
status-firefox32:
--- → verified
You need to log in
before you can comment on or make changes to this bug.
Description
•