Issue 2805793002: ozone: evdev: Add gamepad support

jkwang

Description was changed from ========== Support Gamepad in Ozone. This patch add gamepad event and ...

3 years, 8 months ago (2017-04-07 00:03:49 UTC) #1

jkwang

jkwang@google.com changed reviewers: + sadrul@chromium.org

3 years, 8 months ago (2017-04-07 00:03:49 UTC) #2

sadrul

sadrul@chromium.org changed reviewers: + dtapuska@chromium.org

3 years, 8 months ago (2017-04-10 18:14:04 UTC) #3

sadrul

+dtapuska@ for thoughts on using PointerEvent to represent a gamepad event. https://codereview.chromium.org/2805793002/diff/1/ui/events/event.h File ui/events/event.h (right): ...

3 years, 8 months ago (2017-04-10 18:14:05 UTC) #4

jkwang

https://codereview.chromium.org/2805793002/diff/1/ui/events/event.h File ui/events/event.h (right): https://codereview.chromium.org/2805793002/diff/1/ui/events/event.h#newcode791 ui/events/event.h:791: }; On 2017/04/10 18:14:05, sadrul wrote: > I feel ...

3 years, 8 months ago (2017-04-10 18:19:08 UTC) #5

dtapuska

https://codereview.chromium.org/2805793002/diff/1/ui/events/event.h File ui/events/event.h (right): https://codereview.chromium.org/2805793002/diff/1/ui/events/event.h#newcode791 ui/events/event.h:791: }; On 2017/04/10 18:19:08, jkwang wrote: > On 2017/04/10 ...

3 years, 8 months ago (2017-04-10 19:13:35 UTC) #6

jkwang

https://codereview.chromium.org/2805793002/diff/1/ui/events/event.h File ui/events/event.h (right): https://codereview.chromium.org/2805793002/diff/1/ui/events/event.h#newcode791 ui/events/event.h:791: }; On 2017/04/10 19:13:35, dtapuska wrote: > On 2017/04/10 ...

3 years, 8 months ago (2017-04-10 20:07:47 UTC) #7

https://codereview.chromium.org/2805793002/diff/1/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/2805793002/diff/1/ui/events/event.h#newcode791
ui/events/event.h:791: };
On 2017/04/10 19:13:35, dtapuska wrote:
> On 2017/04/10 18:19:08, jkwang wrote:
> > On 2017/04/10 18:14:05, sadrul wrote:
> > > I feel like this should simply be a PointerEvent, with type set to a new 
> > > POINTER_TYPE_GAMEPAD.
> > 
> > Would it be confusing?
> 
> Game pad logically has no pointer position does it?
> 
> Gamepad isn't combined in W3C land so I guess the question is what
similarities
> exist between a pointing device and a gamepad device. I think a keyboard might
> have more similarities to a gamepad device than a pointer (keys dowm,
> repetition, etc). So I'm on the fence. 
> 
> I think to be a pointer you need a logical x/y location presented by the
> hwd/device. I don't think a gamepad device presents that.

Mostly, gamepad report EV_ABS and EV_KEY. So mapping it to "generic axis moving"
and Key event won't be a bad idea. (If my memory is correct, it is what was done
in android.) However, the event_converter part of gamepad does not really fit
into the keyboard event_converter. To map gamepads to w3c standard, we sometimes
need to map axis to button. With that said, I did not find a "generic axis
event". AND, the w3c gamepad button carry a double value
(https://www.w3.org/TR/gamepad/ )! If we are going to reuse KeyEvent, we need to
do some invasive change. I really want to avoid that.

https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h
File ui/events/event_constants.h (right):

https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h...
ui/events/event_constants.h:46: ET_GAMEPAD_FRAME,
On 2017/04/10 19:13:35, dtapuska wrote:
> On 2017/04/10 18:19:08, jkwang wrote:
> > On 2017/04/10 18:14:05, sadrul wrote:
> > > What's GAMEPAD_FRAME event?
> > 
> > It's essentially a successful sync (syn_reported instead of syn_dropped).
> > Currently the wayland protocol send explicit gamepad frame to sync with the
> > client.
> >
>
https://cs.chromium.org/chromium/src/third_party/wayland-protocols/unstable/g...
> 
> Is this mapped to a vsync signal?

Sorry, I am not aware of vsync signal. Could you point me to it?
 (Vertical Sync? Why is gamepad mapped to vsync?) Currently, when the frame is
seen, the gamepad events will be accumulated and processed together.

sadrul

https://codereview.chromium.org/2805793002/diff/1/ui/events/event.h File ui/events/event.h (right): https://codereview.chromium.org/2805793002/diff/1/ui/events/event.h#newcode790 ui/events/event.h:790: uint16_t code_; Can you document what |value_| and |code_| ...

3 years, 8 months ago (2017-04-12 18:03:58 UTC) #8

jkwang

jkwang@google.com changed reviewers: + denniskempin@google.com

3 years, 8 months ago (2017-04-12 20:18:59 UTC) #9

jkwang

https://codereview.chromium.org/2805793002/diff/1/ui/events/event.h File ui/events/event.h (right): https://codereview.chromium.org/2805793002/diff/1/ui/events/event.h#newcode790 ui/events/event.h:790: uint16_t code_; On 2017/04/12 18:03:58, sadrul wrote: > Can ...

3 years, 8 months ago (2017-04-12 20:51:55 UTC) #10

https://codereview.chromium.org/2805793002/diff/1/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/2805793002/diff/1/ui/events/event.h#newcode790
ui/events/event.h:790: uint16_t code_;
On 2017/04/12 18:03:58, sadrul wrote:
> Can you document what |value_| and |code_| represent? I went over
> https://www.w3.org/TR/gamepad/, and couldn't figure out their meaning.
Interface is here.
interface Gamepad {
    readonly attribute DOMString           id;
    readonly attribute long                index;
    readonly attribute boolean             connected;
    readonly attribute DOMHighResTimeStamp timestamp;
    readonly attribute GamepadMappingType  mapping;
    readonly attribute double[]            axes;
    readonly attribute GamepadButton[]     buttons;
};

Code is the index of axes or button, value is the axes value or button value.
The meaning is defined in ui/events/webgamepad_constants.h	. I will add more
comments here.

https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h
File ui/events/event_constants.h (right):

https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h...
ui/events/event_constants.h:46: ET_GAMEPAD_FRAME,
On 2017/04/12 18:03:58, sadrul wrote:
> On 2017/04/10 18:19:08, jkwang wrote:
> > On 2017/04/10 18:14:05, sadrul wrote:
> > > What's GAMEPAD_FRAME event?
> > 
> > It's essentially a successful sync (syn_reported instead of syn_dropped).
> > Currently the wayland protocol send explicit gamepad frame to sync with the
> > client.
> >
>
https://cs.chromium.org/chromium/src/third_party/wayland-protocols/unstable/g...
> 
> OK. Can this be handled from the evdev-handler? For touch-events, for example,
> we do not generate a TouchEvent until the EV_SYN. Is there a reason gamepad
> should be different here?

Yeah. I took a look at how Touch EV_SYN is handled. For gamepad, I have several
concerns.

1. Generate Gamepad Event until EV_SYN means we have to aggregate the event,
which easily results in jamming all buttons and axes state into one gamepad
event. This does not feel event-driven.

2. Currently, the client of this gamepad implementation will be exo. Exo send
gamepad frame explicitly. If we aggregate gamepad events here, we will end up
keeping a state in exo, the we have to diff the state, and disassemble the
events and send a gamepad frame (sync) again. That seems to be unnecessary
complexity.

If there is not strong reason that EV_SYN should not be send explicitly, I think
current approach will make things simpler.

sadrul

https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h File ui/events/event_constants.h (right): https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h#newcode46 ui/events/event_constants.h:46: ET_GAMEPAD_FRAME, On 2017/04/12 20:51:55, jkwang wrote: > On 2017/04/12 ...

3 years, 8 months ago (2017-04-12 21:26:32 UTC) #11

https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h
File ui/events/event_constants.h (right):

https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h...
ui/events/event_constants.h:46: ET_GAMEPAD_FRAME,
On 2017/04/12 20:51:55, jkwang wrote:
> On 2017/04/12 18:03:58, sadrul wrote:
> > On 2017/04/10 18:19:08, jkwang wrote:
> > > On 2017/04/10 18:14:05, sadrul wrote:
> > > > What's GAMEPAD_FRAME event?
> > > 
> > > It's essentially a successful sync (syn_reported instead of syn_dropped).
> > > Currently the wayland protocol send explicit gamepad frame to sync with
the
> > > client.
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/wayland-protocols/unstable/g...
> > 
> > OK. Can this be handled from the evdev-handler? For touch-events, for
example,
> > we do not generate a TouchEvent until the EV_SYN. Is there a reason gamepad
> > should be different here?
> 
> Yeah. I took a look at how Touch EV_SYN is handled. For gamepad, I have
several
> concerns.
> 
> 1. Generate Gamepad Event until EV_SYN means we have to aggregate the event,
> which easily results in jamming all buttons and axes state into one gamepad
> event. This does not feel event-driven.

The spec says "A client is expected to accumulate the data in all events within
the frame before proceeding.", so accumulating the data in evdev-handler makes
most sense to me.

> 2. Currently, the client of this gamepad implementation will be exo. Exo send
> gamepad frame explicitly. If we aggregate gamepad events here, we will end up
> keeping a state in exo, the we have to diff the state, and disassemble the
> events and send a gamepad frame (sync) again. That seems to be unnecessary
> complexity.
> 
> If there is not strong reason that EV_SYN should not be send explicitly, I
think
> current approach will make things simpler. 

In the current approach, each consumer (i.e. any ui::EventHandler) of gamepad
events will need to do the aggregation separately, which doesn't make sense.
There is also the risk that events in the same frame go to different handlers.

I would also like to see how these events are actually going to be consumed from
exo. Is there a draft CL I could look at for that part?

jkwang

https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h File ui/events/event_constants.h (right): https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h#newcode46 ui/events/event_constants.h:46: ET_GAMEPAD_FRAME, On 2017/04/12 21:26:32, sadrul wrote: > On 2017/04/12 ...

3 years, 8 months ago (2017-04-12 21:41:19 UTC) #12

https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h
File ui/events/event_constants.h (right):

https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h...
ui/events/event_constants.h:46: ET_GAMEPAD_FRAME,
On 2017/04/12 21:26:32, sadrul wrote:
> On 2017/04/12 20:51:55, jkwang wrote:
> > On 2017/04/12 18:03:58, sadrul wrote:
> > > On 2017/04/10 18:19:08, jkwang wrote:
> > > > On 2017/04/10 18:14:05, sadrul wrote:
> > > > > What's GAMEPAD_FRAME event?
> > > > 
> > > > It's essentially a successful sync (syn_reported instead of
syn_dropped).
> > > > Currently the wayland protocol send explicit gamepad frame to sync with
> the
> > > > client.
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/wayland-protocols/unstable/g...
> > > 
> > > OK. Can this be handled from the evdev-handler? For touch-events, for
> example,
> > > we do not generate a TouchEvent until the EV_SYN. Is there a reason
gamepad
> > > should be different here?
> > 
> > Yeah. I took a look at how Touch EV_SYN is handled. For gamepad, I have
> several
> > concerns.
> > 
> > 1. Generate Gamepad Event until EV_SYN means we have to aggregate the event,
> > which easily results in jamming all buttons and axes state into one gamepad
> > event. This does not feel event-driven.
> 
> The spec says "A client is expected to accumulate the data in all events
within
> the frame before proceeding.", so accumulating the data in evdev-handler makes
> most sense to me.
> 
> > 2. Currently, the client of this gamepad implementation will be exo. Exo
send
> > gamepad frame explicitly. If we aggregate gamepad events here, we will end
up
> > keeping a state in exo, the we have to diff the state, and disassemble the
> > events and send a gamepad frame (sync) again. That seems to be unnecessary
> > complexity.
> > 
> > If there is not strong reason that EV_SYN should not be send explicitly, I
> think
> > current approach will make things simpler. 
> 
> In the current approach, each consumer (i.e. any ui::EventHandler) of gamepad
> events will need to do the aggregation separately, which doesn't make sense.
> There is also the risk that events in the same frame go to different handlers.
> 
> I would also like to see how these events are actually going to be consumed
from
> exo. Is there a draft CL I could look at for that part?
Sorry, there is no draft CL yet. But it will be very straight forward. Other
than binding gamepad to the gaming seat. It will route the events through
gamepad delegate. Which is here :
https://cs.chromium.org/chromium/src/components/exo/gamepad_delegate.h.

Exo is not going to do any aggregation, the aggregation will be in the android
side.
If the aggregation is done here, I am afraid there won't be
+  ET_GAMEPAD_BTN_PRESSED,
+  ET_GAMEPAD_BTN_RELEASED,
+  ET_GAMEPAD_ABS_MOVED,.
There will only be ET_GAMEPAD cause all the pressed/released can happen at the
same time. It will be one event sending the state of gamepad.

spang

On 2017/04/12 21:26:32, sadrul wrote: > https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h > File ui/events/event_constants.h (right): > > https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h#newcode46 > ...

3 years, 8 months ago (2017-04-12 23:53:30 UTC) #13

On 2017/04/12 21:26:32, sadrul wrote:
> https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h
> File ui/events/event_constants.h (right):
> 
>
https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h...
> ui/events/event_constants.h:46: ET_GAMEPAD_FRAME,
> On 2017/04/12 20:51:55, jkwang wrote:
> > On 2017/04/12 18:03:58, sadrul wrote:
> > > On 2017/04/10 18:19:08, jkwang wrote:
> > > > On 2017/04/10 18:14:05, sadrul wrote:
> > > > > What's GAMEPAD_FRAME event?
> > > > 
> > > > It's essentially a successful sync (syn_reported instead of
syn_dropped).
> > > > Currently the wayland protocol send explicit gamepad frame to sync with
> the
> > > > client.
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/wayland-protocols/unstable/g...
> > > 
> > > OK. Can this be handled from the evdev-handler? For touch-events, for
> example,
> > > we do not generate a TouchEvent until the EV_SYN. Is there a reason
gamepad
> > > should be different here?
> > 
> > Yeah. I took a look at how Touch EV_SYN is handled. For gamepad, I have
> several
> > concerns.
> > 
> > 1. Generate Gamepad Event until EV_SYN means we have to aggregate the event,
> > which easily results in jamming all buttons and axes state into one gamepad
> > event. This does not feel event-driven.
> 
> The spec says "A client is expected to accumulate the data in all events
within
> the frame before proceeding.", so accumulating the data in evdev-handler makes
> most sense to me.
> 
> > 2. Currently, the client of this gamepad implementation will be exo. Exo
send
> > gamepad frame explicitly. If we aggregate gamepad events here, we will end
up
> > keeping a state in exo, the we have to diff the state, and disassemble the
> > events and send a gamepad frame (sync) again. That seems to be unnecessary
> > complexity.
> > 
> > If there is not strong reason that EV_SYN should not be send explicitly, I
> think
> > current approach will make things simpler. 
> 
> In the current approach, each consumer (i.e. any ui::EventHandler) of gamepad
> events will need to do the aggregation separately, which doesn't make sense.
> There is also the risk that events in the same frame go to different handlers.
> 
> I would also like to see how these events are actually going to be consumed
from
> exo. Is there a draft CL I could look at for that part?

Hold on...

gamepad API doesn't work like UI events. The events are broadcast; there is no
focus or routing to handlers.

device/gamepad shares a global buffer directly to renders than polls for
updates.

I'm not sure what the right way forward is; we need to figure out what the right
division of labor is between device/gamepad and ui/events is for this.

jkwang

On 2017/04/12 23:53:30, spang wrote: > On 2017/04/12 21:26:32, sadrul wrote: > > https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h > ...

3 years, 8 months ago (2017-04-13 00:15:42 UTC) #14

On 2017/04/12 23:53:30, spang wrote:
> On 2017/04/12 21:26:32, sadrul wrote:
> >
https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h
> > File ui/events/event_constants.h (right):
> > 
> >
>
https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h...
> > ui/events/event_constants.h:46: ET_GAMEPAD_FRAME,
> > On 2017/04/12 20:51:55, jkwang wrote:
> > > On 2017/04/12 18:03:58, sadrul wrote:
> > > > On 2017/04/10 18:19:08, jkwang wrote:
> > > > > On 2017/04/10 18:14:05, sadrul wrote:
> > > > > > What's GAMEPAD_FRAME event?
> > > > > 
> > > > > It's essentially a successful sync (syn_reported instead of
> syn_dropped).
> > > > > Currently the wayland protocol send explicit gamepad frame to sync
with
> > the
> > > > > client.
> > > > >
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/wayland-protocols/unstable/g...
> > > > 
> > > > OK. Can this be handled from the evdev-handler? For touch-events, for
> > example,
> > > > we do not generate a TouchEvent until the EV_SYN. Is there a reason
> gamepad
> > > > should be different here?
> > > 
> > > Yeah. I took a look at how Touch EV_SYN is handled. For gamepad, I have
> > several
> > > concerns.
> > > 
> > > 1. Generate Gamepad Event until EV_SYN means we have to aggregate the
event,
> > > which easily results in jamming all buttons and axes state into one
gamepad
> > > event. This does not feel event-driven.
> > 
> > The spec says "A client is expected to accumulate the data in all events
> within
> > the frame before proceeding.", so accumulating the data in evdev-handler
makes
> > most sense to me.
> > 
> > > 2. Currently, the client of this gamepad implementation will be exo. Exo
> send
> > > gamepad frame explicitly. If we aggregate gamepad events here, we will end
> up
> > > keeping a state in exo, the we have to diff the state, and disassemble the
> > > events and send a gamepad frame (sync) again. That seems to be unnecessary
> > > complexity.
> > > 
> > > If there is not strong reason that EV_SYN should not be send explicitly, I
> > think
> > > current approach will make things simpler. 
> > 
> > In the current approach, each consumer (i.e. any ui::EventHandler) of
gamepad
> > events will need to do the aggregation separately, which doesn't make sense.
> > There is also the risk that events in the same frame go to different
handlers.
> > 
> > I would also like to see how these events are actually going to be consumed
> from
> > exo. Is there a draft CL I could look at for that part?
> 
> Hold on...
> 
> gamepad API doesn't work like UI events. The events are broadcast; there is no
> focus or routing to handlers.
> 
> device/gamepad shares a global buffer directly to renders than polls for
> updates.
> 
> I'm not sure what the right way forward is; we need to figure out what the
right
> division of labor is between device/gamepad and ui/events is for this.

Well, yes and no. Most of the time, there is only one instance consuming the 
gamepad events. But I can totally imagine if you have a ui controlled by
gamepad,
and there is a game window side by side. For example, android settings page can
be
controlled by gamepad. I don't have a more concrete example, but I don't think
it's
the worst idea to put gamepad events here.
it logically makes sense.

sadrul

On 2017/04/13 00:15:42, jkwang wrote: > On 2017/04/12 23:53:30, spang wrote: > > On 2017/04/12 ...

3 years, 8 months ago (2017-04-13 00:27:36 UTC) #15

On 2017/04/13 00:15:42, jkwang wrote:
> On 2017/04/12 23:53:30, spang wrote:
> > On 2017/04/12 21:26:32, sadrul wrote:
> > >
> https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h
> > > File ui/events/event_constants.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h...
> > > ui/events/event_constants.h:46: ET_GAMEPAD_FRAME,
> > > On 2017/04/12 20:51:55, jkwang wrote:
> > > > On 2017/04/12 18:03:58, sadrul wrote:
> > > > > On 2017/04/10 18:19:08, jkwang wrote:
> > > > > > On 2017/04/10 18:14:05, sadrul wrote:
> > > > > > > What's GAMEPAD_FRAME event?
> > > > > > 
> > > > > > It's essentially a successful sync (syn_reported instead of
> > syn_dropped).
> > > > > > Currently the wayland protocol send explicit gamepad frame to sync
> with
> > > the
> > > > > > client.
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/wayland-protocols/unstable/g...
> > > > > 
> > > > > OK. Can this be handled from the evdev-handler? For touch-events, for
> > > example,
> > > > > we do not generate a TouchEvent until the EV_SYN. Is there a reason
> > gamepad
> > > > > should be different here?
> > > > 
> > > > Yeah. I took a look at how Touch EV_SYN is handled. For gamepad, I have
> > > several
> > > > concerns.
> > > > 
> > > > 1. Generate Gamepad Event until EV_SYN means we have to aggregate the
> event,
> > > > which easily results in jamming all buttons and axes state into one
> gamepad
> > > > event. This does not feel event-driven.
> > > 
> > > The spec says "A client is expected to accumulate the data in all events
> > within
> > > the frame before proceeding.", so accumulating the data in evdev-handler
> makes
> > > most sense to me.
> > > 
> > > > 2. Currently, the client of this gamepad implementation will be exo. Exo
> > send
> > > > gamepad frame explicitly. If we aggregate gamepad events here, we will
end
> > up
> > > > keeping a state in exo, the we have to diff the state, and disassemble
the
> > > > events and send a gamepad frame (sync) again. That seems to be
unnecessary
> > > > complexity.
> > > > 
> > > > If there is not strong reason that EV_SYN should not be send explicitly,
I
> > > think
> > > > current approach will make things simpler. 
> > > 
> > > In the current approach, each consumer (i.e. any ui::EventHandler) of
> gamepad
> > > events will need to do the aggregation separately, which doesn't make
sense.
> > > There is also the risk that events in the same frame go to different
> handlers.
> > > 
> > > I would also like to see how these events are actually going to be
consumed
> > from
> > > exo. Is there a draft CL I could look at for that part?
> > 
> > Hold on...
> > 
> > gamepad API doesn't work like UI events. The events are broadcast; there is
no
> > focus or routing to handlers.
> > 
> > device/gamepad shares a global buffer directly to renders than polls for
> > updates.
> > 
> > I'm not sure what the right way forward is; we need to figure out what the
> right
> > division of labor is between device/gamepad and ui/events is for this.
> 
> Well, yes and no. Most of the time, there is only one instance consuming the 
> gamepad events. But I can totally imagine if you have a ui controlled by
> gamepad,
> and there is a game window side by side. For example, android settings page
can
> be
> controlled by gamepad. I don't have a more concrete example, but I don't think
> it's
> the worst idea to put gamepad events here.
> it logically makes sense.

Using gamepad to control the ui doesn't sound terrible to me, maybe. But that's
why I wanted to see how these events will be consumed by exo. Because, it sounds
like, the gamepad events will just always be dispatched to the root-window, and
any consumer will need to install a global pre-target event-handler on the
system to be able to receive those events. That isn't a good way of doing this.
So until we figure out how to target+dispatch gamepad events, using it as a ui
event is probably not the best solution.

Have you looked at using //device/gamepad for this?

spang

On 2017/04/12 23:53:30, spang wrote: > On 2017/04/12 21:26:32, sadrul wrote: > > https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h > ...

3 years, 8 months ago (2017-04-13 00:37:18 UTC) #16

On 2017/04/12 23:53:30, spang wrote:
> On 2017/04/12 21:26:32, sadrul wrote:
> >
https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h
> > File ui/events/event_constants.h (right):
> > 
> >
>
https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h...
> > ui/events/event_constants.h:46: ET_GAMEPAD_FRAME,
> > On 2017/04/12 20:51:55, jkwang wrote:
> > > On 2017/04/12 18:03:58, sadrul wrote:
> > > > On 2017/04/10 18:19:08, jkwang wrote:
> > > > > On 2017/04/10 18:14:05, sadrul wrote:
> > > > > > What's GAMEPAD_FRAME event?
> > > > > 
> > > > > It's essentially a successful sync (syn_reported instead of
> syn_dropped).
> > > > > Currently the wayland protocol send explicit gamepad frame to sync
with
> > the
> > > > > client.
> > > > >
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/wayland-protocols/unstable/g...
> > > > 
> > > > OK. Can this be handled from the evdev-handler? For touch-events, for
> > example,
> > > > we do not generate a TouchEvent until the EV_SYN. Is there a reason
> gamepad
> > > > should be different here?
> > > 
> > > Yeah. I took a look at how Touch EV_SYN is handled. For gamepad, I have
> > several
> > > concerns.
> > > 
> > > 1. Generate Gamepad Event until EV_SYN means we have to aggregate the
event,
> > > which easily results in jamming all buttons and axes state into one
gamepad
> > > event. This does not feel event-driven.
> > 
> > The spec says "A client is expected to accumulate the data in all events
> within
> > the frame before proceeding.", so accumulating the data in evdev-handler
makes
> > most sense to me.
> > 
> > > 2. Currently, the client of this gamepad implementation will be exo. Exo
> send
> > > gamepad frame explicitly. If we aggregate gamepad events here, we will end
> up
> > > keeping a state in exo, the we have to diff the state, and disassemble the
> > > events and send a gamepad frame (sync) again. That seems to be unnecessary
> > > complexity.
> > > 
> > > If there is not strong reason that EV_SYN should not be send explicitly, I
> > think
> > > current approach will make things simpler. 
> > 
> > In the current approach, each consumer (i.e. any ui::EventHandler) of
gamepad
> > events will need to do the aggregation separately, which doesn't make sense.
> > There is also the risk that events in the same frame go to different
handlers.
> > 
> > I would also like to see how these events are actually going to be consumed
> from
> > exo. Is there a draft CL I could look at for that part?
> 
> Hold on...
> 
> gamepad API doesn't work like UI events. The events are broadcast; there is no
> focus or routing to handlers.
> 
> device/gamepad shares a global buffer directly to renders than polls for
> updates.
> 
> I'm not sure what the right way forward is; we need to figure out what the
right
> division of labor is between device/gamepad and ui/events is for this.

I'm not sure it makes sense to go through PlatformEventSource for this.

I think the leanest way to do this is probably to add a
gamepad_platform_data_fetcher_ozone, and do something like (in device/gamepad):

GamepadProviderOzone* gamepad_provider =
OzonePlatform::GetInstance()->CreateGamepadProvider();

gamepad_provider->RegisterGamepadCallbacksMustBeThreadsafe(
   &UpdateWebGamepadDigital, &UpdateWebGamepadAnalog);

and we'd call those callbacks directly from our thread. In the optimal case,
we'd get into the renderer's buffer without a thread hop.

Exo should hopefully be able to hook up to device/gamepad just like the renderer
host does.

spang

spang@chromium.org changed reviewers: + spang@chromium.org

3 years, 8 months ago (2017-04-13 00:52:18 UTC) #17

spang

https://codereview.chromium.org/2805793002/diff/1/ui/events/ozone/evdev/event_device_info_unittest.cc File ui/events/ozone/evdev/event_device_info_unittest.cc (right): https://codereview.chromium.org/2805793002/diff/1/ui/events/ozone/evdev/event_device_info_unittest.cc#newcode29 ui/events/ozone/evdev/event_device_info_unittest.cc:29: TEST_F(EventDeviceInfoTest, BasicCrosGamepad) { It's not a "Cros" device; that's ...

3 years, 8 months ago (2017-04-13 00:52:20 UTC) #18

spang

On 2017/04/13 00:15:42, jkwang wrote: > On 2017/04/12 23:53:30, spang wrote: > > On 2017/04/12 ...

3 years, 8 months ago (2017-04-13 01:09:58 UTC) #19

On 2017/04/13 00:15:42, jkwang wrote:
> On 2017/04/12 23:53:30, spang wrote:
> > On 2017/04/12 21:26:32, sadrul wrote:
> > >
> https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h
> > > File ui/events/event_constants.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h...
> > > ui/events/event_constants.h:46: ET_GAMEPAD_FRAME,
> > > On 2017/04/12 20:51:55, jkwang wrote:
> > > > On 2017/04/12 18:03:58, sadrul wrote:
> > > > > On 2017/04/10 18:19:08, jkwang wrote:
> > > > > > On 2017/04/10 18:14:05, sadrul wrote:
> > > > > > > What's GAMEPAD_FRAME event?
> > > > > > 
> > > > > > It's essentially a successful sync (syn_reported instead of
> > syn_dropped).
> > > > > > Currently the wayland protocol send explicit gamepad frame to sync
> with
> > > the
> > > > > > client.
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/wayland-protocols/unstable/g...
> > > > > 
> > > > > OK. Can this be handled from the evdev-handler? For touch-events, for
> > > example,
> > > > > we do not generate a TouchEvent until the EV_SYN. Is there a reason
> > gamepad
> > > > > should be different here?
> > > > 
> > > > Yeah. I took a look at how Touch EV_SYN is handled. For gamepad, I have
> > > several
> > > > concerns.
> > > > 
> > > > 1. Generate Gamepad Event until EV_SYN means we have to aggregate the
> event,
> > > > which easily results in jamming all buttons and axes state into one
> gamepad
> > > > event. This does not feel event-driven.
> > > 
> > > The spec says "A client is expected to accumulate the data in all events
> > within
> > > the frame before proceeding.", so accumulating the data in evdev-handler
> makes
> > > most sense to me.
> > > 
> > > > 2. Currently, the client of this gamepad implementation will be exo. Exo
> > send
> > > > gamepad frame explicitly. If we aggregate gamepad events here, we will
end
> > up
> > > > keeping a state in exo, the we have to diff the state, and disassemble
the
> > > > events and send a gamepad frame (sync) again. That seems to be
unnecessary
> > > > complexity.
> > > > 
> > > > If there is not strong reason that EV_SYN should not be send explicitly,
I
> > > think
> > > > current approach will make things simpler. 
> > > 
> > > In the current approach, each consumer (i.e. any ui::EventHandler) of
> gamepad
> > > events will need to do the aggregation separately, which doesn't make
sense.
> > > There is also the risk that events in the same frame go to different
> handlers.
> > > 
> > > I would also like to see how these events are actually going to be
consumed
> > from
> > > exo. Is there a draft CL I could look at for that part?
> > 
> > Hold on...
> > 
> > gamepad API doesn't work like UI events. The events are broadcast; there is
no
> > focus or routing to handlers.
> > 
> > device/gamepad shares a global buffer directly to renders than polls for
> > updates.
> > 
> > I'm not sure what the right way forward is; we need to figure out what the
> right
> > division of labor is between device/gamepad and ui/events is for this.
> 
> Well, yes and no. Most of the time, there is only one instance consuming the 
> gamepad events. But I can totally imagine if you have a ui controlled by
> gamepad,
> and there is a game window side by side. For example, android settings page
can
> be
> controlled by gamepad. I don't have a more concrete example, but I don't think
> it's
> the worst idea to put gamepad events here.
> it logically makes sense.

If there's UI with nontrivial security requirements controlled by gamepad input,
that's a bug.

I can see why you want to make it work like UI events in that case, but there's
a big impedence mismatch with that and how gamepad input works in Chrome today.

jkwang

On 2017/04/13 00:37:18, spang wrote: > On 2017/04/12 23:53:30, spang wrote: > > On 2017/04/12 ...

3 years, 8 months ago (2017-04-13 18:28:15 UTC) #20

On 2017/04/13 00:37:18, spang wrote:
> On 2017/04/12 23:53:30, spang wrote:
> > On 2017/04/12 21:26:32, sadrul wrote:
> > >
> https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h
> > > File ui/events/event_constants.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h...
> > > ui/events/event_constants.h:46: ET_GAMEPAD_FRAME,
> > > On 2017/04/12 20:51:55, jkwang wrote:
> > > > On 2017/04/12 18:03:58, sadrul wrote:
> > > > > On 2017/04/10 18:19:08, jkwang wrote:
> > > > > > On 2017/04/10 18:14:05, sadrul wrote:
> > > > > > > What's GAMEPAD_FRAME event?
> > > > > > 
> > > > > > It's essentially a successful sync (syn_reported instead of
> > syn_dropped).
> > > > > > Currently the wayland protocol send explicit gamepad frame to sync
> with
> > > the
> > > > > > client.
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/wayland-protocols/unstable/g...
> > > > > 
> > > > > OK. Can this be handled from the evdev-handler? For touch-events, for
> > > example,
> > > > > we do not generate a TouchEvent until the EV_SYN. Is there a reason
> > gamepad
> > > > > should be different here?
> > > > 
> > > > Yeah. I took a look at how Touch EV_SYN is handled. For gamepad, I have
> > > several
> > > > concerns.
> > > > 
> > > > 1. Generate Gamepad Event until EV_SYN means we have to aggregate the
> event,
> > > > which easily results in jamming all buttons and axes state into one
> gamepad
> > > > event. This does not feel event-driven.
> > > 
> > > The spec says "A client is expected to accumulate the data in all events
> > within
> > > the frame before proceeding.", so accumulating the data in evdev-handler
> makes
> > > most sense to me.
> > > 
> > > > 2. Currently, the client of this gamepad implementation will be exo. Exo
> > send
> > > > gamepad frame explicitly. If we aggregate gamepad events here, we will
end
> > up
> > > > keeping a state in exo, the we have to diff the state, and disassemble
the
> > > > events and send a gamepad frame (sync) again. That seems to be
unnecessary
> > > > complexity.
> > > > 
> > > > If there is not strong reason that EV_SYN should not be send explicitly,
I
> > > think
> > > > current approach will make things simpler. 
> > > 
> > > In the current approach, each consumer (i.e. any ui::EventHandler) of
> gamepad
> > > events will need to do the aggregation separately, which doesn't make
sense.
> > > There is also the risk that events in the same frame go to different
> handlers.
> > > 
> > > I would also like to see how these events are actually going to be
consumed
> > from
> > > exo. Is there a draft CL I could look at for that part?
> > 
> > Hold on...
> > 
> > gamepad API doesn't work like UI events. The events are broadcast; there is
no
> > focus or routing to handlers.
> > 
> > device/gamepad shares a global buffer directly to renders than polls for
> > updates.
> > 
> > I'm not sure what the right way forward is; we need to figure out what the
> right
> > division of labor is between device/gamepad and ui/events is for this.
> 
> I'm not sure it makes sense to go through PlatformEventSource for this.
> 
> I think the leanest way to do this is probably to add a
> gamepad_platform_data_fetcher_ozone, and do something like (in
device/gamepad):
> 
> GamepadProviderOzone* gamepad_provider =
> OzonePlatform::GetInstance()->CreateGamepadProvider();
> 
> gamepad_provider->RegisterGamepadCallbacksMustBeThreadsafe(
>    &UpdateWebGamepadDigital, &UpdateWebGamepadAnalog);
> 
> and we'd call those callbacks directly from our thread. In the optimal case,
> we'd get into the renderer's buffer without a thread hop.
> 
> Exo should hopefully be able to hook up to device/gamepad just like the
renderer
> host does.

This sounds OK to me. If gamepad events is not routed as ui events, maybe I
should
make the device connection/disconnection different too. What do you think about
something like:

class GamepadConsumer {
  void OnGamepadConnectionChange();

  virtual void OnGamepadAxis(int device_id, int code, double value) {}
  virtual void OnGamepadButton(int device_id, int code, dobule value) {}
  virtual void OnGamepadFrame(int device_id) {}
}

class GamepadService {
  public:
    static GameapdService* GetInstance();

    void AddGamepadConsumer(GamepadConsumer* c);

    void RemoveGamepadConsumer(GamepadConsumer* c)

    std::vector<InputDevice> GetGamepadDevices();
}

spang

On 2017/04/13 18:28:15, jkwang wrote: > On 2017/04/13 00:37:18, spang wrote: > > On 2017/04/12 ...

3 years, 8 months ago (2017-04-13 20:25:25 UTC) #21

On 2017/04/13 18:28:15, jkwang wrote:
> On 2017/04/13 00:37:18, spang wrote:
> > On 2017/04/12 23:53:30, spang wrote:
> > > On 2017/04/12 21:26:32, sadrul wrote:
> > > >
> >
https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h
> > > > File ui/events/event_constants.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2805793002/diff/1/ui/events/event_constants.h...
> > > > ui/events/event_constants.h:46: ET_GAMEPAD_FRAME,
> > > > On 2017/04/12 20:51:55, jkwang wrote:
> > > > > On 2017/04/12 18:03:58, sadrul wrote:
> > > > > > On 2017/04/10 18:19:08, jkwang wrote:
> > > > > > > On 2017/04/10 18:14:05, sadrul wrote:
> > > > > > > > What's GAMEPAD_FRAME event?
> > > > > > > 
> > > > > > > It's essentially a successful sync (syn_reported instead of
> > > syn_dropped).
> > > > > > > Currently the wayland protocol send explicit gamepad frame to sync
> > with
> > > > the
> > > > > > > client.
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/wayland-protocols/unstable/g...
> > > > > > 
> > > > > > OK. Can this be handled from the evdev-handler? For touch-events,
for
> > > > example,
> > > > > > we do not generate a TouchEvent until the EV_SYN. Is there a reason
> > > gamepad
> > > > > > should be different here?
> > > > > 
> > > > > Yeah. I took a look at how Touch EV_SYN is handled. For gamepad, I
have
> > > > several
> > > > > concerns.
> > > > > 
> > > > > 1. Generate Gamepad Event until EV_SYN means we have to aggregate the
> > event,
> > > > > which easily results in jamming all buttons and axes state into one
> > gamepad
> > > > > event. This does not feel event-driven.
> > > > 
> > > > The spec says "A client is expected to accumulate the data in all events
> > > within
> > > > the frame before proceeding.", so accumulating the data in evdev-handler
> > makes
> > > > most sense to me.
> > > > 
> > > > > 2. Currently, the client of this gamepad implementation will be exo.
Exo
> > > send
> > > > > gamepad frame explicitly. If we aggregate gamepad events here, we will
> end
> > > up
> > > > > keeping a state in exo, the we have to diff the state, and disassemble
> the
> > > > > events and send a gamepad frame (sync) again. That seems to be
> unnecessary
> > > > > complexity.
> > > > > 
> > > > > If there is not strong reason that EV_SYN should not be send
explicitly,
> I
> > > > think
> > > > > current approach will make things simpler. 
> > > > 
> > > > In the current approach, each consumer (i.e. any ui::EventHandler) of
> > gamepad
> > > > events will need to do the aggregation separately, which doesn't make
> sense.
> > > > There is also the risk that events in the same frame go to different
> > handlers.
> > > > 
> > > > I would also like to see how these events are actually going to be
> consumed
> > > from
> > > > exo. Is there a draft CL I could look at for that part?
> > > 
> > > Hold on...
> > > 
> > > gamepad API doesn't work like UI events. The events are broadcast; there
is
> no
> > > focus or routing to handlers.
> > > 
> > > device/gamepad shares a global buffer directly to renders than polls for
> > > updates.
> > > 
> > > I'm not sure what the right way forward is; we need to figure out what the
> > right
> > > division of labor is between device/gamepad and ui/events is for this.
> > 
> > I'm not sure it makes sense to go through PlatformEventSource for this.
> > 
> > I think the leanest way to do this is probably to add a
> > gamepad_platform_data_fetcher_ozone, and do something like (in
> device/gamepad):
> > 
> > GamepadProviderOzone* gamepad_provider =
> > OzonePlatform::GetInstance()->CreateGamepadProvider();
> > 
> > gamepad_provider->RegisterGamepadCallbacksMustBeThreadsafe(
> >    &UpdateWebGamepadDigital, &UpdateWebGamepadAnalog);
> > 
> > and we'd call those callbacks directly from our thread. In the optimal case,
> > we'd get into the renderer's buffer without a thread hop.
> > 
> > Exo should hopefully be able to hook up to device/gamepad just like the
> renderer
> > host does.
> 
> This sounds OK to me. If gamepad events is not routed as ui events, maybe I
> should
> make the device connection/disconnection different too. What do you think
about
> something like:
> 
> class GamepadConsumer {
>   void OnGamepadConnectionChange();
> 
>   virtual void OnGamepadAxis(int device_id, int code, double value) {}
>   virtual void OnGamepadButton(int device_id, int code, dobule value) {}
>   virtual void OnGamepadFrame(int device_id) {}
> }
> 
> class GamepadService {
>   public:
>     static GameapdService* GetInstance();
> 
>     void AddGamepadConsumer(GamepadConsumer* c);
> 
>     void RemoveGamepadConsumer(GamepadConsumer* c)
> 
>     std::vector<InputDevice> GetGamepadDevices();
> }

I was with you until I saw GamepadService (as that already exists in
device/gamepad). Shouldn't it be GamepadProviderOzone? Otherwise yes, that looks
like what I was thinking.

There is a counterargument that gamepad input going into exo should be secured
after all (especially if apps in the ARC++ world are assuming that is secure).
This probably means changes to device/gamepad/, though, and you could probably
keep the above strategy for the ozone interface. I am not sure how much we need
to worry about it, this is a question for the security team. There is a tradeoff
between security and latency; right now gamepads are optimized in favor of
latency.

jkwang

https://codereview.chromium.org/2805793002/diff/1/ui/events/ozone/evdev/event_device_info_unittest.cc File ui/events/ozone/evdev/event_device_info_unittest.cc (right): https://codereview.chromium.org/2805793002/diff/1/ui/events/ozone/evdev/event_device_info_unittest.cc#newcode29 ui/events/ozone/evdev/event_device_info_unittest.cc:29: TEST_F(EventDeviceInfoTest, BasicCrosGamepad) { On 2017/04/13 00:52:19, spang wrote: > ...

3 years, 8 months ago (2017-04-19 19:42:24 UTC) #22

sadrul

sadrul@chromium.org changed reviewers: + scottmg@chromium.org

3 years, 8 months ago (2017-04-21 03:20:01 UTC) #23

spang

https://codereview.chromium.org/2805793002/diff/1/ui/events/ozone/evdev/gamepad_event_converter_evdev.cc File ui/events/ozone/evdev/gamepad_event_converter_evdev.cc (right): https://codereview.chromium.org/2805793002/diff/1/ui/events/ozone/evdev/gamepad_event_converter_evdev.cc#newcode105 ui/events/ozone/evdev/gamepad_event_converter_evdev.cc:105: ResetGamepad(); On 2017/04/19 19:42:23, jkwang wrote: > On 2017/04/13 ...

3 years, 8 months ago (2017-04-21 05:38:47 UTC) #25

https://codereview.chromium.org/2805793002/diff/1/ui/events/ozone/evdev/gamep...
File ui/events/ozone/evdev/gamepad_event_converter_evdev.cc (right):

https://codereview.chromium.org/2805793002/diff/1/ui/events/ozone/evdev/gamep...
ui/events/ozone/evdev/gamepad_event_converter_evdev.cc:105: ResetGamepad();
On 2017/04/19 19:42:23, jkwang wrote:
> On 2017/04/13 00:52:19, spang wrote:
> > Should sync using EVIOCGABS and EVIOCGKEY here.
> 
> Sounds reasonable. But this will be a rare corner case, I am trying to just
> pretend everything is released and prevent something annoying (character keep
> moving in one direction). I think it will be good enough for gaming.

I still think we should do this right for absolute inputs at least; it's not too
hard. Otherwise we're stuck returning wrong data sometimes.

Suppose one of the axes is a throttle with no spring in it and it's set to
"max". Then not syncing the initial state means you'll return 0.0, instead of
1.0, until the user goes and wiggles the control so that it changes value. Some
absoute inputs are not spring loaded to return to center.

It's the same code on startup as on SYN_DROPPED. Look at the MT code for an
example for how to do it (TouchEventConverterEvdev::Initialize).

I don't care as much about syncing keys since it'd only matter if you're holding
them down.

https://codereview.chromium.org/2805793002/diff/1/ui/events/ozone/evdev/gamep...
ui/events/ozone/evdev/gamepad_event_converter_evdev.cc:144: if (mapped_code ==
WG_BUTTON_L2 || mapped_code == WG_BUTTON_R2) {
Shouldn't this kind of stuff go in the mapping?

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/evdev/e...
File ui/events/ozone/evdev/event_device_info.cc (right):

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/evdev/e...
ui/events/ozone/evdev/event_device_info.cc:436: // concrete solution will use
ID_INPUT_JOYSTICK with some patch remving false
typo: removing

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/evdev/g...
File ui/events/ozone/evdev/gamepad_event_converter_evdev.cc (right):

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/evdev/g...
ui/events/ozone/evdev/gamepad_event_converter_evdev.cc:43: // determine if 127
is 0.0 or 128 is 0.0. We will just fuzz by one percent
This seems dubious to me. Why is 1% the right minimum fuzziness for all devices?

I see why you are concerned, we cannot have a linear scale from 2^k values that
has exact representations of all of -1.0, 0.0, and 1.0. Why is it better to make
the mapping nonlinear though? It's not exact to begin with.

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/evdev/g...
File ui/events/ozone/evdev/gamepad_event_converter_evdev_unittest.cc (right):

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/evdev/g...
ui/events/ozone/evdev/gamepad_event_converter_evdev_unittest.cc:60: const
ui::DeviceAbsoluteAxis kWrongAbsGamepadAbsAxes[] = {
I'm confused by this - why is it different from the other copy of the Xbox
controller capabilities? Do you actually have two controllers with different
capabilities (or is this because of different versions of the xpad driver)?

If not, I don't think we should be testing it (please buy controllers with
whatever quirks you're trying to support, and capture real data from them).

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/evdev/g...
ui/events/ozone/evdev/gamepad_event_converter_evdev_unittest.cc:145: {{0, 0},
EV_KEY, BTN_A, 1},      {{0, 0}, EV_KEY, BTN_B, 1},
Please prefer real data for tests. To capture data, use evtest. Feel free to
remove events that are incidental to the test, but this is the most editing you
should do. Don't remove synchronization events for non-empty frames; that's
clearly missing here and so it's obvious this data is fake because it does not
actually obey the protocol.

I really want our tests to be as realistic as possible. The good ones are using
data from evtest and a comment saying that the data is captured from a real
session with the device.

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/gamepad...
File ui/events/ozone/gamepad/gamepad_event.h (right):

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/gamepad...
ui/events/ozone/gamepad/gamepad_event.h:21: int get_device_id() { return
device_id_; }
This should be called just "device_id()". Remove get_ from all these.

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/gamepad...
File ui/events/ozone/gamepad/gamepad_mapping.cc (right):

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/gamepad...
ui/events/ozone/gamepad/gamepad_mapping.cc:370: static struct MappingData {
Please make all this stuff const.

I do not think we should copy the data into a std::map; this will use memory
that will never be freed and will be accessed rarely.

You can make static arrays + sizes and never copy the data (like
DeviceCapabilities).

It's unfortunate to duplicate all of this mapping data between the joydev
support in device/gamepad and here... it would be good to find a way to avoid
it. Let's talk to the gamepad owners before getting too deeply into mapping
here.

You might actually want to leave all the mapping stuff for a followup patch.

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/gamepad...
File ui/events/ozone/gamepad/gamepad_observer.h (right):

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/gamepad...
ui/events/ozone/gamepad/gamepad_observer.h:20: virtual void OnGamepadEvent(const
GamepadEvent& event) {}
We might want to consider if we can just

#include device/gamepad/public/cpp/gamepad.h

(after adding a suitable dependency) and do

OnGamepadFrame(const Gamepad& gamepad);

this would avoid adding new event types and we'd need fewer conversions.

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/gamepad...
File ui/events/ozone/gamepad/gamepad_provider_ozone.h (right):

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/gamepad...
ui/events/ozone/gamepad/gamepad_provider_ozone.h:26: static
GamepadProviderOzone* GetInstance();
Can you get this through the existing OzonePlatform singleton rather than make a
new singleton (look at SystemInputInjector for an example).

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/gamepad...
ui/events/ozone/gamepad/gamepad_provider_ozone.h:60: std::set<GamepadObserver*>
observers_;
set is a binary search tree; this is overkill. Just use a vector or possibly
ObserverList

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/gamepad...
File ui/events/ozone/gamepad/webgamepad_constants.h (right):

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/gamepad...
ui/events/ozone/gamepad/webgamepad_constants.h:8: #include <cstdint>
<stdint.h>

spang

Description was changed from ========== Support Gamepad in Ozone. This patch add gamepad event and ...

3 years, 8 months ago (2017-04-21 17:54:00 UTC) #27

jkwang

https://codereview.chromium.org/2805793002/diff/1/ui/events/ozone/evdev/gamepad_event_converter_evdev.cc File ui/events/ozone/evdev/gamepad_event_converter_evdev.cc (right): https://codereview.chromium.org/2805793002/diff/1/ui/events/ozone/evdev/gamepad_event_converter_evdev.cc#newcode105 ui/events/ozone/evdev/gamepad_event_converter_evdev.cc:105: ResetGamepad(); On 2017/04/21 05:38:46, spang wrote: > On 2017/04/19 ...

3 years, 8 months ago (2017-04-25 21:16:42 UTC) #28

https://codereview.chromium.org/2805793002/diff/1/ui/events/ozone/evdev/gamep...
File ui/events/ozone/evdev/gamepad_event_converter_evdev.cc (right):

https://codereview.chromium.org/2805793002/diff/1/ui/events/ozone/evdev/gamep...
ui/events/ozone/evdev/gamepad_event_converter_evdev.cc:105: ResetGamepad();
On 2017/04/21 05:38:46, spang wrote:
> On 2017/04/19 19:42:23, jkwang wrote:
> > On 2017/04/13 00:52:19, spang wrote:
> > > Should sync using EVIOCGABS and EVIOCGKEY here.
> > 
> > Sounds reasonable. But this will be a rare corner case, I am trying to just
> > pretend everything is released and prevent something annoying (character
keep
> > moving in one direction). I think it will be good enough for gaming.
> 
> I still think we should do this right for absolute inputs at least; it's not
too
> hard. Otherwise we're stuck returning wrong data sometimes.
> 
> Suppose one of the axes is a throttle with no spring in it and it's set to
> "max". Then not syncing the initial state means you'll return 0.0, instead of
> 1.0, until the user goes and wiggles the control so that it changes value.
Some
> absoute inputs are not spring loaded to return to center.
> 
> It's the same code on startup as on SYN_DROPPED. Look at the MT code for an
> example for how to do it (TouchEventConverterEvdev::Initialize).
> 
> I don't care as much about syncing keys since it'd only matter if you're
holding
> them down.

Done.

https://codereview.chromium.org/2805793002/diff/1/ui/events/ozone/evdev/gamep...
ui/events/ozone/evdev/gamepad_event_converter_evdev.cc:144: if (mapped_code ==
WG_BUTTON_L2 || mapped_code == WG_BUTTON_R2) {
On 2017/04/21 05:38:46, spang wrote:
> Shouldn't this kind of stuff go in the mapping?

This is fixed, when the mapped axis is L2 or R2 the range will be 0.0 to 1.0.
Other axis range is -1.0 to 1.0.

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/evdev/e...
File ui/events/ozone/evdev/event_device_info.cc (right):

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/evdev/e...
ui/events/ozone/evdev/event_device_info.cc:436: // concrete solution will use
ID_INPUT_JOYSTICK with some patch remving false
On 2017/04/21 05:38:46, spang wrote:
> typo: removing

Done.

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/evdev/g...
File ui/events/ozone/evdev/gamepad_event_converter_evdev.cc (right):

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/evdev/g...
ui/events/ozone/evdev/gamepad_event_converter_evdev.cc:43: // determine if 127
is 0.0 or 128 is 0.0. We will just fuzz by one percent
On 2017/04/21 05:38:46, spang wrote:
> This seems dubious to me. Why is 1% the right minimum fuzziness for all
devices?
> 
> I see why you are concerned, we cannot have a linear scale from 2^k values
that
> has exact representations of all of -1.0, 0.0, and 1.0. Why is it better to
make
> the mapping nonlinear though? It's not exact to begin with.

I am doing this because I am concerned that some game will rely on the special
meaning of -1.0, 0.0, and 1.0. (Though I was not able to see the source code,
but I do think some game rely on 0.0: The character will move slowly when the
reported axis value is below 0.01 of 1.0.). Yes, it is nonlinear. But I would
assume that 1% movement is small enough.

Anyway, I agree this is a little bit hacky. I am moving this implementation
similar to android gamepad axis implementation. 
https://codesearch.corp.google.com/android/frameworks/native/services/inputfl...

Quick Note: Fuzz will reset to be 0.25f * flat when it was reported as zero.

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/evdev/g...
File ui/events/ozone/evdev/gamepad_event_converter_evdev_unittest.cc (right):

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/evdev/g...
ui/events/ozone/evdev/gamepad_event_converter_evdev_unittest.cc:60: const
ui::DeviceAbsoluteAxis kWrongAbsGamepadAbsAxes[] = {
On 2017/04/21 05:38:46, spang wrote:
> I'm confused by this - why is it different from the other copy of the Xbox
> controller capabilities? Do you actually have two controllers with different
> capabilities (or is this because of different versions of the xpad driver)?
> 
> If not, I don't think we should be testing it (please buy controllers with
> whatever quirks you're trying to support, and capture real data from them).

Sure. I am removing this and will add some other gamepad later.

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/gamepad...
File ui/events/ozone/gamepad/gamepad_event.h (right):

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/gamepad...
ui/events/ozone/gamepad/gamepad_event.h:21: int get_device_id() { return
device_id_; }
On 2017/04/21 05:38:46, spang wrote:
> This should be called just "device_id()". Remove get_ from all these.

Done.

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/gamepad...
File ui/events/ozone/gamepad/gamepad_mapping.cc (right):

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/gamepad...
ui/events/ozone/gamepad/gamepad_mapping.cc:370: static struct MappingData {
On 2017/04/21 05:38:46, spang wrote:
> Please make all this stuff const.
Sure.
> 
> I do not think we should copy the data into a std::map; this will use memory
> that will never be freed and will be accessed rarely.
> 
> You can make static arrays + sizes and never copy the data (like
> DeviceCapabilities).
I was hesitating about static array or map. Did not think the memory will be a
huge problem. While map can avoid explicit loop through the array and make the
code cleaner, it might slow down the shut down time cause of the dtor. I am
changing it to static array. 

> 
> It's unfortunate to duplicate all of this mapping data between the joydev
> support in device/gamepad and here... it would be good to find a way to avoid
> it. Let's talk to the gamepad owners before getting too deeply into mapping
> here.
No. It's not duplicating. Jodev->Webgamepad and Evdev->Webgamepad are quite
different. I don't think anything can be easily reused.
> 
> You might actually want to leave all the mapping stuff for a followup patch.

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/gamepad...
File ui/events/ozone/gamepad/gamepad_observer.h (right):

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/gamepad...
ui/events/ozone/gamepad/gamepad_observer.h:20: virtual void OnGamepadEvent(const
GamepadEvent& event) {}
On 2017/04/21 05:38:46, spang wrote:
> We might want to consider if we can just
> 
> #include device/gamepad/public/cpp/gamepad.h
> 
> (after adding a suitable dependency) and do
> 
> OnGamepadFrame(const Gamepad& gamepad);
> 
> this would avoid adding new event types and we'd need fewer conversions.
The Gamepad class works great if the gamepad provider expose gamepad state
rather than send "gamepad event". As far as my client side (arc++ gamepad), I
care about gamepad event more than state. (I also believe most gamepad client
deal with gamepad event). Using OnGamepadFrame(const Gamepad& gamepad) here will
result in aggregating the events here and than differentiate gamepad state in
the client. It seems a waste to me.

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/gamepad...
File ui/events/ozone/gamepad/gamepad_provider_ozone.h (right):

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/gamepad...
ui/events/ozone/gamepad/gamepad_provider_ozone.h:26: static
GamepadProviderOzone* GetInstance();
On 2017/04/21 05:38:47, spang wrote:
> Can you get this through the existing OzonePlatform singleton rather than make
a
> new singleton (look at SystemInputInjector for an example).

Will this result in a circular dependency? ui/ozone need to depende
ui/events/ozone to create GamepadProviderOzone instance and ui/events/ozone
depend ui/ozone because it need to GamepadProviderOzone to dispatch the events. 
I am not sure if moving GamepadProviderOzone to ui/ozone/public is a good idea?

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/gamepad...
ui/events/ozone/gamepad/gamepad_provider_ozone.h:60: std::set<GamepadObserver*>
observers_;
On 2017/04/21 05:38:47, spang wrote:
> set is a binary search tree; this is overkill. Just use a vector or possibly
> ObserverList

Done.

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/gamepad...
File ui/events/ozone/gamepad/webgamepad_constants.h (right):

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/gamepad...
ui/events/ozone/gamepad/webgamepad_constants.h:8: #include <cstdint>
On 2017/04/21 05:38:47, spang wrote:
> <stdint.h>

Done.

scottmg

Sorry, missed that I was added here. Is it possible to break this CL up? ...

3 years, 7 months ago (2017-05-01 18:20:53 UTC) #29

jkwang

https://codereview.chromium.org/2805793002/diff/40001/ui/events/BUILD.gn File ui/events/BUILD.gn (left): https://codereview.chromium.org/2805793002/diff/40001/ui/events/BUILD.gn#oldcode1 ui/events/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights reserved. ...

3 years, 7 months ago (2017-05-01 22:21:57 UTC) #30

jkwang

Description was changed from ========== Support Gamepad in Ozone. This patch add gamepad event and ...

3 years, 7 months ago (2017-05-02 18:19:10 UTC) #31

spang

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/evdev/event_device_info.cc File ui/events/ozone/evdev/event_device_info.cc (right): https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/evdev/event_device_info.cc#newcode438 ui/events/ozone/evdev/event_device_info.cc:438: for (int key = BTN_JOYSTICK; key <= BTN_THUMBR; ++key) ...

3 years, 7 months ago (2017-05-03 22:00:38 UTC) #32

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/evdev/e...
File ui/events/ozone/evdev/event_device_info.cc (right):

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/evdev/e...
ui/events/ozone/evdev/event_device_info.cc:438: for (int key = BTN_JOYSTICK; key
<= BTN_THUMBR; ++key)
Please add braces around all multiline blocks. The outer block here has two
lines.

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/gamepad...
File ui/events/ozone/gamepad/gamepad_mapping.cc (right):

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/gamepad...
ui/events/ozone/gamepad/gamepad_mapping.cc:370: static struct MappingData {
On 2017/04/25 21:16:41, jkwang wrote:
> On 2017/04/21 05:38:46, spang wrote:
> > Please make all this stuff const.
> Sure.
> > 
> > I do not think we should copy the data into a std::map; this will use memory
> > that will never be freed and will be accessed rarely.
> > 
> > You can make static arrays + sizes and never copy the data (like
> > DeviceCapabilities).
> I was hesitating about static array or map. Did not think the memory will be a
> huge problem. While map can avoid explicit loop through the array and make the
> code cleaner, it might slow down the shut down time cause of the dtor. I am
> changing it to static array. 
> 
> > 
> > It's unfortunate to duplicate all of this mapping data between the joydev
> > support in device/gamepad and here... it would be good to find a way to
avoid
> > it. Let's talk to the gamepad owners before getting too deeply into mapping
> > here.
> No. It's not duplicating. Jodev->Webgamepad and Evdev->Webgamepad are quite
> different. I don't think anything can be easily reused.
> > 
> > You might actually want to leave all the mapping stuff for a followup patch.

I'm confident you could compute joydev mappings from evdev mappings, because the
joydev state is computed from the evdev state in the kernel. If it's not true,
then the mappings must be inconsistent - which is really the problem we're
opening ourselves to by having two mappings for the same devices.

Unifying things is not necessarily a showstopper though.

https://codereview.chromium.org/2805793002/diff/60001/ui/events/ozone/evdev/e...
File ui/events/ozone/evdev/event_device_info.cc (right):

https://codereview.chromium.org/2805793002/diff/60001/ui/events/ozone/evdev/e...
ui/events/ozone/evdev/event_device_info.cc:444: for (int key = BTN_JOYSTICK; key
<= BTN_THUMBR; ++key)
Please brace all multiline blocks such as the outer loop here.

https://codereview.chromium.org/2805793002/diff/60001/ui/events/ozone/evdev/g...
File ui/events/ozone/evdev/gamepad_event_converter_evdev.cc (right):

https://codereview.chromium.org/2805793002/diff/60001/ui/events/ozone/evdev/g...
ui/events/ozone/evdev/gamepad_event_converter_evdev.cc:138: if (evdev_ev.value >
abs_info.value - abs_info.fuzz &&
Can't this fuzzing prevent the value from returning zero? That seems bad.

https://codereview.chromium.org/2805793002/diff/60001/ui/events/ozone/evdev/g...
ui/events/ozone/evdev/gamepad_event_converter_evdev.cc:149: double offset =
(abs_info.maximum + abs_info.minimum) /
offset and scale can be pre-calculated. If you do that up front I don't think
trigger needs to be special here.

I think at a high level what you should do is replace abs_info_ with a new local
"struct Axis" so that you can add new stuff there such as the scale factor.
Then, pre-calculate the values you need here in Initialize().

https://codereview.chromium.org/2805793002/diff/60001/ui/events/ozone/evdev/g...
ui/events/ozone/evdev/gamepad_event_converter_evdev.cc:172: (mapped_abs_value ==
0.0 && old_value < abs_info.flat)) {
It's hard to follow this because it mixes use of the the raw and transformed
values. If you can, please do the transform first, and do the rest of the
computations with those values, or do them last, and do the rest of the
computations with the raw values.

It's also hard to see that this button logic is safe in the sense that buttons
cannot become stuck, and it looks like a rapid change from positive to negative
or vice versa will get lost until the value changes again. How about something
like:

bool hat_left_press = (mapped_abs_value > threshold);
bool hat_right_press = (mapped_abs_value < -threshold);

if (hat_left_press != last_hat_left_press_) {
  OnButtonChange(WG_BUTTON_DPAD_LEFT, hat_left_press);
  last_hat_left_press_ = hat_left_press;
}

if (hat_right_press != last_hat_right_press_) {
  OnButtonChange(WG_BUTTON_DPAD_RIGHT, hat_right_press);
  last_hat_right_press_ = hat_right_press;
}

Finally, is using flat right here? This seems like it would be too sensitive.
device/gamepad is using 0.5.

https://codereview.chromium.org/2805793002/diff/60001/ui/events/ozone/evdev/g...
ui/events/ozone/evdev/gamepad_event_converter_evdev.cc:202: void
GamepadEventConverterEvdev::ResyncGamepad() {
Please either sync buttons or release them (or you might get a stuck button).

https://codereview.chromium.org/2805793002/diff/60001/ui/events/ozone/evdev/g...
ui/events/ozone/evdev/gamepad_event_converter_evdev.cc:215: event.time =
base::Time::UnixEpoch().ToTimeVal();
Unix epoch is for the real time clock. Input uses the monotonic clock. Also, the
fact that timestamps of events from the same device are monotonically increasing
can be important.

Use base::TimeTicks::Now().

Please call it once (not in the loop).

spang

On 2017/05/01 18:20:53, scottmg wrote: > Sorry, missed that I was added here. > > ...

3 years, 7 months ago (2017-05-03 22:11:03 UTC) #33

jkwang

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/evdev/event_device_info.cc File ui/events/ozone/evdev/event_device_info.cc (right): https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/evdev/event_device_info.cc#newcode438 ui/events/ozone/evdev/event_device_info.cc:438: for (int key = BTN_JOYSTICK; key <= BTN_THUMBR; ++key) ...

3 years, 7 months ago (2017-05-05 00:07:45 UTC) #34

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/evdev/e...
File ui/events/ozone/evdev/event_device_info.cc (right):

https://codereview.chromium.org/2805793002/diff/20001/ui/events/ozone/evdev/e...
ui/events/ozone/evdev/event_device_info.cc:438: for (int key = BTN_JOYSTICK; key
<= BTN_THUMBR; ++key)
On 2017/05/03 22:00:37, spang wrote:
> Please add braces around all multiline blocks. The outer block here has two
> lines.

Done.

https://codereview.chromium.org/2805793002/diff/60001/ui/events/ozone/evdev/e...
File ui/events/ozone/evdev/event_device_info.cc (right):

https://codereview.chromium.org/2805793002/diff/60001/ui/events/ozone/evdev/e...
ui/events/ozone/evdev/event_device_info.cc:444: for (int key = BTN_JOYSTICK; key
<= BTN_THUMBR; ++key)
On 2017/05/03 22:00:37, spang wrote:
> Please brace all multiline blocks such as the outer loop here.

Done.

https://codereview.chromium.org/2805793002/diff/60001/ui/events/ozone/evdev/g...
File ui/events/ozone/evdev/gamepad_event_converter_evdev.cc (right):

https://codereview.chromium.org/2805793002/diff/60001/ui/events/ozone/evdev/g...
ui/events/ozone/evdev/gamepad_event_converter_evdev.cc:138: if (evdev_ev.value >
abs_info.value - abs_info.fuzz &&
On 2017/05/03 22:00:37, spang wrote:
> Can't this fuzzing prevent the value from returning zero? That seems bad.

Done.

https://codereview.chromium.org/2805793002/diff/60001/ui/events/ozone/evdev/g...
ui/events/ozone/evdev/gamepad_event_converter_evdev.cc:149: double offset =
(abs_info.maximum + abs_info.minimum) /
On 2017/05/03 22:00:38, spang wrote:
> offset and scale can be pre-calculated. If you do that up front I don't think
> trigger needs to be special here.
> 
> I think at a high level what you should do is replace abs_info_ with a new
local
> "struct Axis" so that you can add new stuff there such as the scale factor.
> Then, pre-calculate the values you need here in Initialize().

Done.

https://codereview.chromium.org/2805793002/diff/60001/ui/events/ozone/evdev/g...
ui/events/ozone/evdev/gamepad_event_converter_evdev.cc:172: (mapped_abs_value ==
0.0 && old_value < abs_info.flat)) {
On 2017/05/03 22:00:37, spang wrote:
> It's hard to follow this because it mixes use of the the raw and transformed
> values. If you can, please do the transform first, and do the rest of the
> computations with those values, or do them last, and do the rest of the
> computations with the raw values.
> 
> It's also hard to see that this button logic is safe in the sense that buttons
> cannot become stuck, and it looks like a rapid change from positive to
negative
> or vice versa will get lost until the value changes again. How about something
> like:
> 
> bool hat_left_press = (mapped_abs_value > threshold);
> bool hat_right_press = (mapped_abs_value < -threshold);
> 
> if (hat_left_press != last_hat_left_press_) {
>   OnButtonChange(WG_BUTTON_DPAD_LEFT, hat_left_press);
>   last_hat_left_press_ = hat_left_press;
> }
> 
> if (hat_right_press != last_hat_right_press_) {
>   OnButtonChange(WG_BUTTON_DPAD_RIGHT, hat_right_press);
>   last_hat_right_press_ = hat_right_press;
> }
> 
> Finally, is using flat right here? This seems like it would be too sensitive.
> device/gamepad is using 0.5.

Done.

https://codereview.chromium.org/2805793002/diff/60001/ui/events/ozone/evdev/g...
ui/events/ozone/evdev/gamepad_event_converter_evdev.cc:202: void
GamepadEventConverterEvdev::ResyncGamepad() {
On 2017/05/03 22:00:37, spang wrote:
> Please either sync buttons or release them (or you might get a stuck button).

Done.

https://codereview.chromium.org/2805793002/diff/60001/ui/events/ozone/evdev/g...
ui/events/ozone/evdev/gamepad_event_converter_evdev.cc:215: event.time =
base::Time::UnixEpoch().ToTimeVal();
On 2017/05/03 22:00:38, spang wrote:
> Unix epoch is for the real time clock. Input uses the monotonic clock. Also,
the
> fact that timestamps of events from the same device are monotonically
increasing
> can be important.
> 
> Use base::TimeTicks::Now().
> 
> Please call it once (not in the loop).

Done.

spang

just a few nits left https://codereview.chromium.org/2805793002/diff/100001/ui/events/ozone/evdev/gamepad_event_converter_evdev.cc File ui/events/ozone/evdev/gamepad_event_converter_evdev.cc (right): https://codereview.chromium.org/2805793002/diff/100001/ui/events/ozone/evdev/gamepad_event_converter_evdev.cc#newcode20 ui/events/ozone/evdev/gamepad_event_converter_evdev.cc:20: double kHatThreshold = 0.5; ...

3 years, 7 months ago (2017-05-05 15:37:43 UTC) #35

spang

Description was changed from ========== Support Gamepad in Ozone. This patch add gamepad event and ...

3 years, 7 months ago (2017-05-05 17:22:51 UTC) #36

jkwang

https://codereview.chromium.org/2805793002/diff/100001/ui/events/ozone/evdev/gamepad_event_converter_evdev.cc File ui/events/ozone/evdev/gamepad_event_converter_evdev.cc (right): https://codereview.chromium.org/2805793002/diff/100001/ui/events/ozone/evdev/gamepad_event_converter_evdev.cc#newcode20 ui/events/ozone/evdev/gamepad_event_converter_evdev.cc:20: double kHatThreshold = 0.5; On 2017/05/05 15:37:43, spang wrote: ...

3 years, 7 months ago (2017-05-05 21:32:58 UTC) #37

scottmg

https://codereview.chromium.org/2805793002/diff/100001/ui/events/ozone/evdev/gamepad_event_converter_evdev.cc File ui/events/ozone/evdev/gamepad_event_converter_evdev.cc (right): https://codereview.chromium.org/2805793002/diff/100001/ui/events/ozone/evdev/gamepad_event_converter_evdev.cc#newcode57 ui/events/ozone/evdev/gamepad_event_converter_evdev.cc:57: if (*mapped_value<scaled_flat_&& * mapped_value> - scaled_flat_) { On 2017/05/05 ...

3 years, 7 months ago (2017-05-05 22:04:35 UTC) #38

spang

On 2017/05/05 22:04:35, scottmg wrote: > https://codereview.chromium.org/2805793002/diff/100001/ui/events/ozone/evdev/gamepad_event_converter_evdev.cc > File ui/events/ozone/evdev/gamepad_event_converter_evdev.cc (right): > > https://codereview.chromium.org/2805793002/diff/100001/ui/events/ozone/evdev/gamepad_event_converter_evdev.cc#newcode57 > ...

3 years, 7 months ago (2017-05-05 22:43:26 UTC) #39