Closed
Bug 831781
Opened 11 years ago
Closed 11 years ago
Allow panning using gamepad d-pad/joystick
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 22
People
(Reporter: kats, Assigned: kats)
References
Details
Attachments
(4 files, 5 obsolete files)
10.79 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
6.74 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
5.82 KB,
patch
|
cwiiis
:
review+
|
Details | Diff | Splinter Review |
4.70 KB,
patch
|
cwiiis
:
review+
|
Details | Diff | Splinter Review |
Currently it is hard/impossible to scroll down pages in Fennec on the ouya because we don't do anything with the d-pad/joystick input events. We should support those to scroll around the page.
Assignee | ||
Comment 1•11 years ago
|
||
This is just cleanup/generification; no functional changes. I should have realized before that mouse wheel code doesn't belong in TouchEventHandler since they're not touch events.
Assignee | ||
Comment 2•11 years ago
|
||
This allows scrolling around the page smoothly with the joystick. The InputDevice.MotionRange objects for the Ouya gamepad seems to be returning improper data so I hard-coded that bit for now. Also this doesn't do any sort of axis locking which we might want to do.
Assignee | ||
Comment 3•11 years ago
|
||
Assignee | ||
Comment 4•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #703620 -
Flags: review?(chrislord.net)
Assignee | ||
Comment 5•11 years ago
|
||
Comment on attachment 703621 [details] [diff] [review] Part 2 - Support joystick scrolling Btw, I believe the "else" clause of this hunk: mDisplacement += (mLastTouchPos - mTouchPos) * getEdgeResistance(false); else - mDisplacement += mVelocity; + mDisplacement += mVelocity * getEdgeResistance(false); was previously unused, so this change shouldn't affect any previous behaviour. It is used in the autoscroll codepath though. Please let me know if you think it was used in some codepath previously because then this change might regress something.
Attachment #703621 -
Flags: review?(chrislord.net)
Assignee | ||
Updated•11 years ago
|
Attachment #704031 -
Flags: review?(chrislord.net)
Assignee | ||
Comment 6•11 years ago
|
||
We'll probably want to deliver the events to content first and then only handle them if content doesn't use them, similarly to what we're doing in TouchEventHandler. However I think that a lot of this code will change as we merge PZC with B2G's APZC so I figure it's not too much of a big deal right now.
Attachment #704032 -
Attachment is obsolete: true
Attachment #704543 -
Flags: review?(chrislord.net)
Comment 7•11 years ago
|
||
Excuse me for taking a little longer on review than usual, I feel I'd like to try these out before r+'ing and, unfortunately, I don't have a suitable arm objdir atm to do a quick incremental build from.
Assignee | ||
Comment 8•11 years ago
|
||
No problem; I definitely want you to try out the patches and help me tune them if needed. In particular for the zooming one I'm not sure if we should be doing a linear bump in the zoom level (which is what the patch does) or a multiplicative increase/decrease. (e.g. R2 does zoomFactor *= 1.1 instead of zoomFactor += 0.2)
Assignee | ||
Comment 9•11 years ago
|
||
:Cwiiis, do you mind reviewing part 1 here? It's not ouya-specific but it will simplify some of my PZC unification work so I'd like to get that part in at least.
Assignee | ||
Comment 10•11 years ago
|
||
Actually, scratch that last comment. I'll need to rework it somewhat so I might as well rewrite it and incorporate it into the other bug.
Assignee | ||
Comment 11•11 years ago
|
||
Chris, ping. Seeing as we haven't really had much time to clean this up I'd like to get it reviewed and landed so that at least scrolling on the Ouya is possible. We can improve it in future bugs.
Comment 12•11 years ago
|
||
Comment on attachment 703620 [details] [diff] [review] Part 1 - Move mouse wheel code out of TouchEventHandler Review of attachment 703620 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine.
Attachment #703620 -
Flags: review?(chrislord.net) → review+
Comment 13•11 years ago
|
||
Comment on attachment 703621 [details] [diff] [review] Part 2 - Support joystick scrolling Review of attachment 703621 [details] [diff] [review]: ----------------------------------------------------------------- This looks ok too. Would be good to see the comment addressed. ::: mobile/android/base/ui/PanZoomController.java @@ +446,5 @@ > return false; > } > > + private float normalizeJoystick(float value, InputDevice.MotionRange range) { > + if (Math.abs(value) < 1e-2 /* this should really be range.getFlat() + range.getFuzz() */) { And why isn't it? Maybe expand the comment?
Attachment #703621 -
Flags: review?(chrislord.net) → review+
Comment 14•11 years ago
|
||
Comment on attachment 704031 [details] [diff] [review] Part 3 - Extract a helper function Review of attachment 704031 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.
Attachment #704031 -
Flags: review?(chrislord.net) → review+
Assignee | ||
Comment 15•11 years ago
|
||
Comment on attachment 703620 [details] [diff] [review] Part 1 - Move mouse wheel code out of TouchEventHandler Err oops, forgot to obsolete this patch. I landed this code as part of bug 777468.
Attachment #703620 -
Attachment is obsolete: true
Comment 16•11 years ago
|
||
Comment on attachment 704543 [details] [diff] [review] Part 4 - Use L2/R2 for zooming Review of attachment 704543 [details] [diff] [review]: ----------------------------------------------------------------- r- because of the mapping, but I have nothing against the actual code - feel free to commit without hooking it up to those buttons, or hooking it up to keyboard zoom in/out buttons or the mouse-wheel. ::: mobile/android/base/gfx/LayerView.java @@ +222,1 @@ > if (mInputConnectionHandler != null) It might be nice to change this if statement to match the one above, in case someone shuffles these around at some point and breaks it - javac should just optimise it out anyway, right? ::: mobile/android/base/ui/PanZoomController.java @@ +209,5 @@ > + switch (event.getKeyCode()) { > + case KeyEvent.KEYCODE_BUTTON_R2: > + return animatedScale(0.2f); > + case KeyEvent.KEYCODE_BUTTON_L2: > + return animatedScale(-0.2f); I don't like this mapping - the Ouya's shoulder buttons are analog triggers, are are most console pad's L2/R2 buttons (certainly PS3, 360 and WiiU 'Pro' pads are) - They should be available on AXIS_LTRIGGER and AXIS_RTRIGGER apparently.
Attachment #704543 -
Flags: review?(chrislord.net) → review-
Assignee | ||
Comment 17•11 years ago
|
||
Rebased, updated to address review comment (about getFlat() and getFuzz()). Carrying r+
Attachment #703621 -
Attachment is obsolete: true
Attachment #718597 -
Flags: review+
Assignee | ||
Comment 18•11 years ago
|
||
Rebased, carrying r+
Attachment #704031 -
Attachment is obsolete: true
Attachment #718599 -
Flags: review+
Assignee | ||
Comment 19•11 years ago
|
||
Rebased, and mapped zoom in/out to the KeyEvent.KEYCODE_ZOOM_IN and _OUT keys respectively. We never actually get these from the ouya gamepad though.
Attachment #704543 -
Attachment is obsolete: true
Attachment #718600 -
Flags: review?(chrislord.net)
Assignee | ||
Comment 20•11 years ago
|
||
This addresses your other review comment in comment 16, and also changes the rest of the onKeyXXXX functions to be consistent. Also I found some unnecessary mLayerClient.getPanZoomController() calls left over from previous code changes and lumped that cleanup in here too.
Attachment #718603 -
Flags: review?(chrislord.net)
Comment 21•11 years ago
|
||
Comment on attachment 718600 [details] [diff] [review] Part 3 - Hook up zooming Review of attachment 718600 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the comment addressed. ::: mobile/android/base/gfx/JavaPanZoomController.java @@ +220,5 @@ > + if (Build.VERSION.SDK_INT <= 11) { > + return false; > + } > + > + if ((event.getSource() & InputDevice.SOURCE_GAMEPAD) == InputDevice.SOURCE_GAMEPAD This isn't quite what I meant, I'm not sure we'd ever get ZOOM_IN/OUT from a gamepad - Can we change this to SOURCE_KEYBOARD, or at least || with SOURCE_KEYBOARD?
Attachment #718600 -
Flags: review?(chrislord.net) → review+
Comment 22•11 years ago
|
||
Comment on attachment 718603 [details] [diff] [review] Part 4 - code cleanup Review of attachment 718603 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.
Attachment #718603 -
Flags: review?(chrislord.net) → review+
Assignee | ||
Comment 23•11 years ago
|
||
(In reply to Chris Lord [:cwiiis] from comment #21) > This isn't quite what I meant, I'm not sure we'd ever get ZOOM_IN/OUT from a > gamepad - Can we change this to SOURCE_KEYBOARD, or at least || with > SOURCE_KEYBOARD? Sure, I can change that. But I don't understand the point here - are there any keyboards that have dedicated "zoom in" and "zoom out" buttons? I don't know of any, and so this is dead code regardless. I guess what I'm wondering is, in an ideal world, what buttons on the Ouya controller would you like to map zooming to? (In reply to Chris Lord [:cwiiis] from comment #16) > I don't like this mapping - the Ouya's shoulder buttons are analog triggers, > are are most console pad's L2/R2 buttons (certainly PS3, 360 and WiiU 'Pro' > pads are) - They should be available on AXIS_LTRIGGER and AXIS_RTRIGGER > apparently. I admit I don't fully understand what you meant here. My interpretation of "shoulder buttons" are L1 and R1, which are not analog. My interpretation of "trigger buttons" are L2 and R2, which are usually analog but on the Ouya seem to not be (or at least I didn't see any analog data coming from them). I didn't find AXIS_LTRIGGER and AXIS_RTRIGGER anywhere, so I don't know what those are referring to.
Comment 24•11 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #23) > (In reply to Chris Lord [:cwiiis] from comment #21) > > This isn't quite what I meant, I'm not sure we'd ever get ZOOM_IN/OUT from a > > gamepad - Can we change this to SOURCE_KEYBOARD, or at least || with > > SOURCE_KEYBOARD? > > Sure, I can change that. But I don't understand the point here - are there > any keyboards that have dedicated "zoom in" and "zoom out" buttons? I don't > know of any, and so this is dead code regardless. I guess what I'm wondering > is, in an ideal world, what buttons on the Ouya controller would you like to > map zooming to? I think my point is that we shouldn't map zoom to digital buttons when we have good analog alternatives (and apart from the WiiMote which has +/- buttons, it goes against convention) - I did like the code to handle this though, so it'd be nice to have it in there. I've had media keyboards with dedicated zoom buttons before, and we could use this code to handle mouse wheels too (which are usually implemented as button presses rather than an analog signal... sigh...) So in an ideal world for Ouya, zoom would be mapped to the 2nd analog stick or the triggers. I quite like it on the 2nd analog stick, personally (this is what the WiiU does). > (In reply to Chris Lord [:cwiiis] from comment #16) > > I don't like this mapping - the Ouya's shoulder buttons are analog triggers, > > are are most console pad's L2/R2 buttons (certainly PS3, 360 and WiiU 'Pro' > > pads are) - They should be available on AXIS_LTRIGGER and AXIS_RTRIGGER > > apparently. > > I admit I don't fully understand what you meant here. My interpretation of > "shoulder buttons" are L1 and R1, which are not analog. My interpretation of > "trigger buttons" are L2 and R2, which are usually analog but on the Ouya > seem to not be (or at least I didn't see any analog data coming from them). > I didn't find AXIS_LTRIGGER and AXIS_RTRIGGER anywhere, so I don't know what > those are referring to. Sorry, I guess we need to establish terminology... I usually think of L1/R1 as hats and L2/R2 as triggers, but I'd call all of them buttons - I think your use of button/trigger is better, let's stick with that. It's worth having a look at the ouya-controller-testapp sample in the Ouya development kit - it shows how to read all of the inputs on the gamepad. They have their own defines for the buttons, but they all map to standard Android equivalents - So for the shoulder buttons, you can listen to onGenericMotionEvent, and use event.getAxisValue(MotionEvent.AXIS_LTRIGGER), something like that - LTRIGGER is API level 12 though, so I guess we'll have to copy the numbers out... Can we map zoom to the right stick though, rather than the triggers? It really feels so much better :)
Assignee | ||
Comment 25•11 years ago
|
||
(In reply to Chris Lord [:cwiiis] from comment #24) > I think my point is that we shouldn't map zoom to digital buttons when we > have good analog alternatives (and apart from the WiiMote which has +/- > buttons, it goes against convention) - I did like the code to handle this > though, so it'd be nice to have it in there. I've had media keyboards with > dedicated zoom buttons before, and we could use this code to handle mouse > wheels too (which are usually implemented as button presses rather than an > analog signal... sigh...) Agreed, it would be better to use an analog input for this feature rather than digital. I can land the patch as-is and then we can work with ibarlow to figure out the best mapping for everything. > So in an ideal world for Ouya, zoom would be mapped to the 2nd analog stick > or the triggers. I quite like it on the 2nd analog stick, personally (this > is what the WiiU does). I think the plan for the second analog stick was to make it do spatial navigation (link-to-link jumping). That needs a 4-way directional input, so I guess it could be mapped to the arrow buttons as well, but would feel more natural on the analog stick, I think. > > (In reply to Chris Lord [:cwiiis] from comment #16) > Sorry, I guess we need to establish terminology... I usually think of L1/R1 > as hats and L2/R2 as triggers, but I'd call all of them buttons - I think > your use of button/trigger is better, let's stick with that. > > It's worth having a look at the ouya-controller-testapp sample in the Ouya > development kit - it shows how to read all of the inputs on the gamepad. Hmm, I should look at that... > They have their own defines for the buttons, but they all map to standard > Android equivalents - So for the shoulder buttons, you can listen to > onGenericMotionEvent, and use event.getAxisValue(MotionEvent.AXIS_LTRIGGER), > something like that - LTRIGGER is API level 12 though, so I guess we'll have > to copy the numbers out... Ah, ok, that makes sense. > Can we map zoom to the right stick though, rather than the triggers? It > really feels so much better :) I'll leave that decision up to ibarlow. I'm fine with it either way as long as we have something good to map spatial nav to.
Assignee | ||
Comment 26•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f65ac1718a02 https://hg.mozilla.org/integration/mozilla-inbound/rev/afac1e603462 https://hg.mozilla.org/integration/mozilla-inbound/rev/c48096095ae2 https://hg.mozilla.org/integration/mozilla-inbound/rev/736b43ee49d4
Comment 27•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f65ac1718a02 https://hg.mozilla.org/mozilla-central/rev/afac1e603462 https://hg.mozilla.org/mozilla-central/rev/c48096095ae2 https://hg.mozilla.org/mozilla-central/rev/736b43ee49d4
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Comment 28•10 years ago
|
||
Just booted up the lastest nightly. Not sure if you guys decided to nix the d-pad for pan or not but its not working on my Ouya.
Assignee | ||
Comment 29•10 years ago
|
||
I think we wanted to use the d-pad for link-to-link navigation which landed at some point (bug 698437) but I don't remember the current status of that. It might be preffed off still. Does joystick-based panning work for you?
Comment 30•10 years ago
|
||
Joystick does still work yes. Thank you for letting me know. link to link is probably the best way to go about it but that doesn't seem to work either I'm sure they know already.
Assignee | ||
Comment 31•10 years ago
|
||
I looked through the code and actually it does seem to be hooked up and enabled. It might just be that the key mappings are wrong for the ouya. Link-to-link navigation works on my android phone which has a hardware keyboard and a d-pad. Probably best to file a new bug for this issue.
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•