Page MenuHomePhabricator

elput: add/remove Ecore_Device and set dev value in Ecore_Events
AbandonedPublic

Authored by jpeg on Jul 27 2016, 7:33 PM.

Details

Summary

When devices are changed, elput add/remove ecore_devices and send Ecore_Event_Device_Info events. Then ecore_input_evas add/remove evas_devices from those events.
When elput gets input events, elput finds ecore_device and store it in Ecore_Events. Then ecore_input_evas finds evas_device and store it in Evas_Events.
Now enlightenment(server) can get ecore_device/evas_device from events.

Test Plan
  1. Enlightenment gets Evas_Event_Mouse_Down.
  2. You can get source device information from ev->dev. evas_device_class_get(), evas_device_name_get(), evas_device_description_get() etc.

Diff Detail

Repository
rEFL core/efl
Branch
devs/jpeg/ecore_device
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 2364
Build 2429: arc lint + arc unit
ohduna updated this revision to Diff 9656.Jul 27 2016, 7:33 PM
ohduna retitled this revision from to elput: add/remove Ecore_Device and set dev value in Ecore_Events.
ohduna updated this object.
ohduna edited the test plan for this revision. (Show Details)
ohduna added subscribers: input.hacker, JHyun, cedric.
jpeg edited edge metadata.Jul 28 2016, 10:17 PM

Note: based on devs/jpeg/ecore_device which introduces the type Ecore_Device (Efl_Input_Device).

Please refer all kind comments at https://phab.enlightenment.org/D4179.

Evas_Events have a 'Evas_Device *dev' field for identifying event-source device. This field was unused so far.
We'd like to use this field to give device information to server and clients(will be next step).
Identifying devices only from their seat might have limitation on dealing input events and that's not common requirement. I think.

Thank~

jpeg added a comment.Sep 21 2016, 4:57 AM

At first sight, this looks ok. But I don't know any of the details here :(

ManMower edited edge metadata.Sep 21 2016, 12:05 PM

I worry that this may conflict with some of the multi-seat work taking place right now... It may be best to let this rest until that work is concluded?

Due to the way certain backends (wayland, drm/gl_drm) combine input devices into seats in an irreversible way, I think this can lead unaware developers into writing subtly broken code (if anyone's trying to match press/release events per device, they're going to get in trouble...)

At the very least on those backends I think it would be better to report the seat the event was initiated by instead of the exact device, and provide a way to enumerate the devices within that seat.

I still have a hard time understanding what the use case is for this. It can't safely be used to break up a seat into individual devices, and it's overkill if all you want to do is tell the user to press <picture of jump button> on their controller.

raster added inline comments.Sep 21 2016, 3:46 PM
src/lib/elput/elput_input.c
364

flag should be usefully named like "added" or "is_add" or something... flag just means nothing here until you read the code to find it really means "true == add" :)

raster edited edge metadata.Sep 21 2016, 3:48 PM

@ManMower ... this is in elput - it's not used by wayland CLIENTS... but by any app going to drm/kms itself ... like the compositor (e) or an app just not being a wl client at all and directly living int he "fb". even then elput for these direct-to-fb apps will be hidden mostly under elm abstractions. there is no reason for them to go digging down layers to non-portable layers of efl.

zmike requested changes to this revision.Sep 21 2016, 4:03 PM
zmike edited edge metadata.

I get what this is trying to do, but based on the lack of any concrete use case to consider (why is this needed???), as well as my own misgivings about the potential results of encouraging developers to try (wrongly) splitting devices out of seats, I'm going to temporarily reject this until everyone is on the same page.

This revision now requires changes to proceed.Sep 21 2016, 4:03 PM

@zmike - i totally disagree with you on this. first "developers" here will be us. not app or other 3rd party devs. evas_device and ecore_device provide this extended device info. this is ensuring it is filled in. by rejecting it you are making swathes of features to do with devices "useless".

now why should someone know? know if this is a mouse or a touch? why NOT? if mouse moves - display pointer (compositor), but if a tocuh/move happens for a TOUCH event (the device tells you what kind of device it is - mouse vs touch), then hide pointer again. rejecting this rejects the ability to do this at all and this is something very basic.

now you have 2 mice? being able to know which one the event comes from is useful. is that the highdpi gamer mouse or just that really el-cheapo chinese $2 job? a game COULD decide to disable mouse input smoothing if its the high quality gaming mouse, but it'd do smoothing to fix up ugly jitters on the el-cheapo one. same for the composite here since this is elput... the compositor could filter events differently based on a database of known "bad/broken" mice or keyboards or whatever. you can display different images of the device in dialogs or in tooltips/hints or stuff overlayed in games showing the correct image and label for the button to press etc.) if you have a db of popular input devices thus providing a far better experience (game engines like unity, unreal etc would likely have such db's and do this for the game in an abstraction). without the info this can never be done.

without the info NOTHING can be done. with it there is at least the option to do so. apps can do all sorts fo stupid stuff with input and data they can get... mountains of it.

I realize this is elput code - but getting it into elput is potentially a first step towards something like: https://review.tizen.org/git/?p=platform/core/uifw/wayland-extension.git;a=blob;f=protocol/tizen-extension.xml;h=7bacfa6dbf1f54a74f83fe3a27bca38419397dde;hb=refs/heads/accepted/tizen_common#l995

And I don't want to be on that road. That's protocol nobody will *ever* support but us. Meanwhile just having protocol to list devices in a seat is something I can potentially upstream in wayland-protocols with a reference impementation in weston.

One of my biggest fears about this whole body of work and all follow up work is that someone is going to try to use it to do a 2 player game with the control devices in the same seat. That cannot be allowed to happen. :(

In D4194#72167, @raster wrote:

now why should someone know? know if this is a mouse or a touch? why NOT? if mouse moves - display pointer (compositor), but if a tocuh/move happens for a TOUCH event (the device tells you what kind of device it is - mouse vs touch), then hide pointer again. rejecting this rejects the ability to do this at all and this is something very basic.

Mouse and touch are provided to us separately (from libinput) in the first place, *we* combine them. Wouldn't it be better to just have a switch saying "this app is modern enough not to need bogus pointer emulation from the touch interface". Then we can properly have touch and cursor not fighting over a pointer.

I'm worried that this is an ugly short term hack that's could have a long term maintenance load. Just keeping the input separate in the first place might be easier.

now you have 2 mice? being able to know which one the event comes from is useful. is that the highdpi gamer mouse or just that really el-cheapo chinese $2 job?

Nobody does this. Libinput is already normalizing for DPI though, which is the bulk of the difference anyway.

a game COULD decide to disable mouse input smoothing if its the high quality gaming mouse, but it'd do smoothing to fix up ugly jitters on the el-cheapo one. same for the composite here since this is elput... the compositor could filter events differently based on a database of known "bad/broken" mice or keyboards or whatever. you can display different images of the device in dialogs or in tooltips/hints or stuff overlayed in games showing the correct image and label for the button to press etc.) if you have a db of popular input devices thus providing a far better experience (game engines like unity, unreal etc would likely have such db's and do this for the game in an abstraction). without the info this can never be done.

(You said "we" would use this, not "app developers" - so how is Unity/Unreal getting the information?)

The el-cheapo mouse is probably using the same sensor module as a $20 logitech mouse in the first place. The worst tracking mouse I've had in recent memory was a high end laser-optical mouse - and the problem were resolved by a firmware update. Firmware query was a vendor specific usb command that we wouldn't be able to use anyway. So - what good does our DB do then? Who contributes to the DB? Who validates the submissions? Wireless mice can have all sorts of problems based on other equipment in the room, distance from the receiver - if one user says the device is terrible it could just be their environment.

Why not just have an "enable mouse smoothing" option in config that a user can toggle themselves if they want to. Then we don't get people whinging that EFL forces mouse smoothing on them when they don't want it.

Realistically, the snob gamer is going to bemoan the lack of such an option, and the average user won't notice if their mouse input is a little janky. Ask any gamer about mouse-acceleration in fps games, and then ask any non-gamer how they feel about it. They either have a really strong opinion and want to turn it off manually, or they have no idea what it is and just don't care.

If the end user's mouse is a turd they're probably just going to buy a new one anyway. They're not like touchpads that are bolted into your laptop, and good ones are cheap. Also, if the filtering is done on the efl client side then we'll open ourselves up to a mountain of bug reports that Enlightenment makes input suck for tuxracer, because people won't realize we're just doing filtering in EFL clients.

Overlaying stuff in game doesn't require this machinery at all, btw. What I've been suggesting all along is being able to get the seat of an event, and a list of devices in a seat. Then the game knows all the devices the user could use to present the signal. What if a user configures the game's input one way, then the next day buys the latest greatest controller and plugs it in. What's his configuration menu going to show - icons of the buttons at the time he set them up? It seems to me that knowing what's drawn on the button on the currently attached controllers is far more important...

libinput logical seats allow us to assign devices that really should be treated independently to their own seats so their input streams don't get munged together. No accidental sharing of button states or modifiers is possible then.

I really think we should be solving this with a lot more front-end work (controller assignment into seats in a config screen of some kind in E or whatever app is a direct drm/gl_drm using game) and a lost less back-end work (just give the client the seat, and a list of what's in that seat. then nobody can make the mistake of trying to split up a seat)

devilhorns edited edge metadata.Feb 10 2017, 5:09 PM

Just want to add a note here that I have a branch (https://git.enlightenment.org/core/efl.git/log/?h=devs/devilhorns/drm_evas_devices) which does add the event->device before raising of elput input events. The branch handles things the same as the way ecore_evas_wayland does now...

jpeg added a comment.Jul 11 2017, 6:30 PM

Hey @ManMower I guess this patch is no longer relevant and dev is now properly set on events? (I know there's still issues with dev being either a pointer or a seat device)

@jpeg I think you are right

jpeg added a comment.Jul 12 2017, 8:01 PM

@ohduna could you abandon this patch now? I believe we need to review what's done in upstream, including wl2 and elput and then see if this fits your needs.

jpeg commandeered this revision.Jul 12 2017, 8:06 PM
jpeg edited reviewers, added: ohduna; removed: jpeg.

I was told Duna will not be working on EFL anytime soon :)

jpeg abandoned this revision.Jul 12 2017, 8:06 PM
zmike added a comment.Jul 13 2017, 5:20 AM

This is still fundamentally broken in the current implementation: we cannot continue sending the pointer device in events. We must only send seat devices in order to maintain pointer state.