First draft of a glutin backend by Drakulix · Pull Request #9 · Smithay/smithay

Drakulix · 2017-03-07T17:58:56Z

So here is a good amount of code I have been experimenting and playing with and that needs a lot of discussion, which is why I am uploading it (please don't merge yet, it doesn't even compile).

had to enable the "dlopen" feature of wayland-server because otherwise things become broken when compiling with glutin (probably because glutin is still using wayland-client 0.7).
I am thinking about dropping the whole backend/graphics module right now. The OpenglRenderer is currently only coping traits and types from glutin and glium. I also made sure there is a way to layer gfx-rs on top of that, but currently there is a lot of code duplication happening right now. I feel there is probably need for a general opengl context library, that just describes how a GlContext should look like and that every library uses. But that will not happen any time soon and I don't find it troublesome, if every backend just provides it's own rendering api. We can perhaps layer a very simple optional one for drawing windows on top of all of them. But it is fact, that every rendering backend will work totally different and it does not seem reasonable to merge all OpenGL backends together.
On the other hand the input trait seems to work out, but it requires a lot of additional discussion about the api:
- What kind of devices can be up to the compositor to handle? I have no idea, how for example gamepad input works on linux. Is that even abstracted or do all games just talk to the raw device?
- How does on describe touch events?
- Do we save Modifiers? (I think we should)
- I feel like everybody is using it's own types again and that a generic input library just describing how input should look, is needed here. The format in existing libraries is so different, that handling each backend individually makes much less sense here (compared to graphics).
- I am not sure, if I should continue with the ID's or should just make InputDevice generic. But I think this approach avoids much code duplication when dealing with multiple backends at once. If you don't care about the internal identifier of the different backends you should be able to handle all of them with a single handler.
how difficult would it be to run closures on the event loop for every tick? Getting multi-threaded with opengl contexts is not much fun.

elinorbgr · 2017-03-07T18:47:18Z

Woaw, that's quite some work you've done here!

I haven't yet looked in detail at your code, but first of all I have some reactions to your PR message:

had to enable the "dlopen" feature of wayland-server

Yeah, that's because wayland-client and wayland-server depend of wayland-sys, and glutin depends on wayland-client[dlopen]. But I would expect this to just work™ nonetheless. The version mismatch is indeed probably the cause of our trouble.

What kind of devices can be up to the compositor to handle?

The core wayland protocol only handles mouses, keyboards and touchscreens. Other input devices (like gamepads) are afaik often handled directly by the clients which need them, using specialized libs.

Any way, for now lets just stick to these 3.

How does on describe touch events? [and regarding different input libraries]

My opinion on handling inputs in general is that, as the core goal is to run a compositor on the bare OS, libinput should be the final target, and we could design the input backend api using it as an inspiration. The goal is not to perfectly match it, but it to be the lowest-overhead backend.

Do we save Modifiers? (I think we should)

libxkbcommon does it for us. So I'd say it'll not be the role of the input backend, but of the libxkbcommon "middlewayre".

I think I cannot give answers to your other points without reading your code, so I'll do that right now.

elinorbgr · 2017-03-07T19:33:35Z

Regarding the glutin backend and the event loop, actually things are likely to get much simpler with the upcoming new api of winit/glutin: there will be 2 objects, the Window which is used to draw, and the EventsLoop used to fetch the events. This will likely help us separate the "input" part from the "graphics" part.

elinorbgr · 2017-03-07T19:41:19Z

+    fn is_current(&self) -> bool;
+    unsafe fn make_current(&self);
+    fn get_api(&self) -> Api;
+    fn get_pixel_format(&self) -> PixelFormat;


What is you general feeling about this trait?

I'm not extremely used to OpenGL, but fom what I've seen in glutin/glium, it looks like it pretty much covers the fundamental abstraction of "an OpenGL canvas to draw on".

On a nitpick, we should not directly expose glutin's types, but rather expose our own and do the translation in the impl of the backend (possibly via a From<_> impl).

It's okay-ish. I am not happy with copying most of the stuff from glutin and glium. It works, but I think we should drop it completely. A backend should just expose it's own rendering backend. This would avoid copying even more types, which is necessary, not just a nitpick, because I would like to make glutin a feature.

Isn't this part of glutin/glium a direct expression of how OpenGL works?

I mean, for example I expect all graphics backends (glutin/x11/wayland/native) to be directly usable with OpenGL wrappers such as glium and gfx...

So I guess my question here is: what in this trait is glutin/glium-specific?

gluim does not need get_api or get_pixel_format, actually no real library needs it, glutin just provides both of them. glx does not even need any of these, except get_proc_address. It just depends on the gl context being the current one and also expects that not to change over time and figures out the rest itself via low-level c calls internally. However anybody using raw GL might want functionally such as get_api to ease development.
I am just not happy with how directly linked this backend is to gliums requirements. It does not feel very generic or useful, if you just copy types and traits from glutin.

However you are right, it should be quite easy to put this over any backend type, we are currently aiming at. And I feel like it is already very complete.

I see you concerns. I didn't realise half of these methods where only required by glium.

I think the scope we should target for this backend is the union of the requirements of the various opengl libraries that may be used with smithay, as long as it does not imply any significant overhead (be it a runtime cost or a development cost).

But overall, I'd rather be in favour of providing too many methods than too few.

I am also in favour of implementing traits of said opengl libraries. This will ease integration but most importantly always make sure at least the advertised libraries are working on the trait. If the situation gets to complicated at any time, we can still discuss dropping it.

Yes, we can completely add a glium and a gfx cargo features (or others), each of them activating the boilerplate to crate a glium / gfx context from the OpenGLRenderer trait. 👍

elinorbgr · 2017-03-07T19:46:01Z

+use std::hash::Hash;
+
+#[derive(Hash, PartialEq, Eq, Clone, Copy)]
+pub enum InputDevice<T: PartialEq + Eq + Hash + Clone + Copy>


I'm not a big fan of this "ID" thing regarding the input devices...

I'd rather follow the "seat" route of libinput (which matches pretty closely the wayland protocol, probably by design): there can be one or more logical Seats, but each of them has at most 1 of each input device type.

In this case, we could rather have a SeatHandler trait, with a type of which an instance is created for each seat, so the trait would probably have to include a constructor method.

In the glutin case, there would be only one seat.

So we could not distinguish the devices on one seat? How would that work out with different keyboards and layouts. I still try to understand how xkbcommon would handle that stuff.

I think there would simply be one xkbcommon context per seat. And the idea is that seat is a logical representation, and libinput has a system-wide configuration setup apparently.

So basically, it would be up to the computer user to decide whether their two keyboard should be treated as a single one, or appear as two different logical seats.

You can find some explanation about libinput's seats here: https://wayland.freedesktop.org/libinput/doc/latest/seats.html

And the udev config for libinput: https://wayland.freedesktop.org/libinput/doc/latest/udev_config.html

From a protocol point of view, switching keyboard layout implies sending a new wl_keyboard::keymap event to all clients, and these clients are probably supposed to load a new keymap themselves.

Thats some very interesting info. Can we as a libinput user actually detect, which keyboard is sending a key and just switch the layout if the other is used?

If the two keyboards has been configured libinput-side to be on different seats, yes.

My take on this would be:

Have the InputBackends expose seats like libinput (glutin backend would have a single seat, wayland backend would just forward the seats it receives, x11 backedn I don't know)

Have the WlSeat helper expect input organized as seats

Allow the user of smithay to map them to each other as they please

This way, a compositor built on smithay could merge two seats if it wants, and juggle with the keymaps if needed.

Then, in a second time, we could provide in smithay a simple helper to match the two sides of this pipeline 1-1, for compositors that just want a simple case. But it's important to still expose the very low APIs. I don't want smithay to be very opinionated as how compositors should behave.

If the two keyboards has been configured libinput-side to be on different seats, yes.

Again the same problem. I see this is not fixable easily. I don't really get why the protocol supports different keymaps per window, but not between different keyboards (on the same seat).

My take on this would be

Seems reasonable, lets start with that.

It's not really "different keymap per window", but rather "the compositor advertizes the current keymap to the clients, but the clients are free to use their own". The wl_keyboard::keymap event is only informative. Client are free to use their own keymap config.

https://wayland.freedesktop.org/libinput/doc/latest/group__event.html#ga8a49a089181960f7536344f7637ea777

^ It is actually possible to differentiate between devices. And it is necessary for device configuration. I guess we need a way to access devices or more callbacks now.

elinorbgr · 2017-03-07T19:48:05Z

+
+    //TODO render stuff
+
+    //TODO put input handling on the event loop


This will probably be blocked on some work in wayland-server, I need to implement the support of secondary event FDs (needed for libinput support for example) as well as a mechanism to send messages to an event loop.

Drakulix · 2017-03-07T20:07:53Z

Regarding the glutin backend and the event loop, actually things are likely to get much simpler with the upcoming new api of winit/glutin: there will be 2 objects, the Window which is used to draw, and the EventsLoop used to fetch the events. This will likely help us separate the "input" part from the "graphics" part.

I am hoping the EventsLoop will be Send, but I expect some platforms (especially OSX) to break that.

elinorbgr · 2017-03-07T20:12:13Z

As per winit's API, the EventsLoop must be Send : https://github.com/tomaka/winit/blob/master/tests/events_loop.rs#L3

Drakulix · 2017-03-07T22:05:42Z

Any idea how quickly this will be available in glutin?

Anyway I know what I need to do for now and will ping you once I got some new stuff to show of.

elinorbgr · 2017-03-07T22:15:35Z

Any idea how quickly this will be available in glutin?

I don't know, all I can say is that we can follow rust-windowing/glutin#864

Drakulix · 2017-03-09T17:14:07Z

Okay I have pushed a second version. It is still missing some stuff I want to implement, but at first the changes:

OpenglRenderer should now be completely fleshed out
InputHandler now follows libinput's design very closely, I will most likely start some libinput bindings soon, now I got a grasp of the api. It is a little simplified, but I think for the better. Could you look over it and the relevant libinput documentation and tell me your thoughts? The docs are:
Key differences to how libinput does things:
- Mouse coordinates are absolute and no raw device data is given, just the translated screen coordinates
- Time only guarantees order of the events, it can not be actually used for measuring time between any events. This is potentially limiting, but this is what wlc does and I never had a problem with this. This makes other backends like glutin far easier to manage. If needed this can be changed.
- Slot is just an ID right now. We might want to change that to a WlSeat?

One interesting note here is, that - if I am not mistaken - libinput has no real multi-touch support outside of gestures. The api does not allow to differentiate between different fingers like glutin's api does.

The remaining libinput events are not a very high priority, but I also plan to add them later down the line.

Documentation is obviously missing and will be added.

The glutin translation should mostly be ready, but is of course (like most of the code) untested. glium's Backend trait is also not yet provided, also something on my todo list.

elinorbgr · 2017-03-09T21:47:03Z

Regarding the API we are converging to for GraphicsBackend and InputBackend traits, I really like the way it's going! 👍

Regarding touch events, and especially multitouch: my understanding is that libinput_event_touch_get_slot() returns a different ID for each finger touching the touchscreen. This would coincide with what the wayland protocol does: https://github.com/wayland-project/wayland/blob/master/protocol/wayland.xml#L2219

Mouse coordinates are absolute and no raw device data is given, just the translated screen coordinates

I don't know what's best between this and relative events. We want to support "teleporting" the pointer if the compositors wants to (and the backend has this capability, which I believe native and x11 have, and possibly glutin on x11 too). But let's keep that this way for now, we can change it later.

Time only guarantees order of the events, it can not be actually used for measuring time between any events. [...] If needed this can be changed.

Okay. As for the previous point, this'll do for now, and we can increase the guarantees on that afterwards if needed.

Slot is just an ID right now. We might want to change that to a WlSeat?

Hmm, not sure what you would mean by that. WlSeat is an object instantiated in the wayland protocol, and thus linked to client...

elinorbgr · 2017-03-09T21:48:09Z

+    fn on_pointer_button(&mut self, seat: &Seat, time: u32, button: MouseButton, state: MouseButtonState);
+    fn on_pointer_scroll(&mut self, seat: &Seat, time: u32, axis: Axis, source: AxisSource, amount: f64);
+    fn on_touch(&mut self, seat: &Seat, time: u32, event: TouchEvent);
 }


We need a callback to advertise the capabilities of a seat (keyboard/mouse/touch), the wayland protocol requires this.

Do we need a callback? I would suggest promoting Seat to a real type and add this via a method. So you can do something like: seat.capabilities() or even seat.capabilities.contain(Capability::Mouse) or something along those lines.

That could be a way too, but we'll need to properly handle the case where a seat gains or loses a capability during its lifetime. In this context, the callback is the best guarantee I have in mind that the handler will be notified about this change.

What about doing seat.capabilities and on_seat_changed as a new callback. I feel like capabilities is not something all backends will have and emulation is pretty annoying. This way we could also add other properties to seat's of the other backends.

elinorbgr · 2017-03-09T21:51:08Z

-                    },
                    Event::Focused(focused) => if focused {
-                        handler.on_input_attached(InputDevice::Keyboard(()));
+                        handler.on_seat_created(&0u32);


I'm not sure destroying/recreating the seat each time focus is gained/lost is a proper thing to do.

I'd rather say that the glutin backend has a single everlasting seat, which is advertized a single time in the set_handler method, and never destroyed.

This was done mostly because I was too lazy to do this outside of process_events and because it helps testing, if you don't need to restart to trigger this event. In theory a compositor should be able to handle this, but I see your point. I will change it.

elinorbgr · 2017-03-09T21:54:24Z

I'm also in favour a creating an InputBackend trait, even if its definition is as simple as

pub trait InputBackend<H: InputHandler> {
    pub fn set_handler(&mut self, handler: H);
    pub fn get_handler(&mut self) -> &mut H;
    pub fn clear_handler(&mut self);
}

In order to allow writing code generic over input backends, as the GraphicBackend trait allows.

elinorbgr · 2017-03-09T21:56:17Z

Hmm, github is handling poorly my comments on your second commit, why does it show them as on an "outdated diff"?

Drakulix · 2017-03-09T23:48:09Z

Hmm, github is handling poorly my comments on your second commit, why does it show them as on an "outdated diff"?

Really no idea. Seems very weird.

Regarding touch events, and especially multitouch: my understanding is that libinput_event_touch_get_slot() returns a different ID for each finger touching the touchscreen. This would coincide with what the wayland protocol does: https://github.com/wayland-project/wayland/blob/master/protocol/wayland.xml#L2219

I totally looked over that! I have probably mistaken it for libinput_event_touch_get_seat_slot(). I will fix this.

I don't know what's best between this and relative events. We want to support "teleporting" the pointer if the compositors wants to (and the backend has this capability, which I believe native and x11 have, and possibly glutin on x11 too). But let's keep that this way for now, we can change it later.

I would suggest adding platform specific behavior to the different InputBackends instead. Absolute coordinates are easier to handle in most cases, you should just be able to do glutin_input.set_pointer(x, y) if supported.

Okay. As for the previous point, this'll do for now, and we can increase the guarantees on that afterwards if needed.

I will document this for later. Supporting this is no issue for libinput at all, just a little more difficult on glutin. Do we want microsecond precision? Duration would allow that easily.

Hmm, not sure what you would mean by that. WlSeat is an object instantiated in the wayland protocol, and thus linked to client...

Didn't realize this was client only. I have no much knowledge of the wayland protocol, I just thought we are maybe linking libinput's with wayland's seats somehow. I guess an id or a full blown type implementing Eq and Hash is enough in that case?

elinorbgr · 2017-03-10T07:15:25Z

I would suggest adding platform specific behavior to the different InputBackends instead. Absolute coordinates are easier to handle in most cases, you should just be able to do glutin_input.set_pointer(x, y) if supported.

Yes, in most cases absolute coordinates are the simpler. But things like mouse-grabbing protocol extensions (for games mostly) can require to process the relative motion without moving the pointer at all on the screen... Anyway, this is not a priority, but something we'll be able to add/change afterwards, depending on how it appears effectively needed.

I guess an id or a full blown type implementing Eq and Hash is enough in that case?

For the seats, probably, yes. I'm in favour of a type with Eq and Hash, and doing the same for the touch ids as well.

Drakulix · 2017-03-10T19:46:45Z

Third draft:

Renamed InputBackendHandler to InputHandler
Added InputBackend - allows setting an InputHandler. Encourages to use a Box internally to not be bound to a single handler type. I can imagine use cases, where you want to exchange handlers and making the trait generic over a certain handler makes this more difficult.
set_cursor_position api. Relative to the client input area. Hopefully will allow us to work on wayland in the future, when the pointer grabbing/wrapping api has been worked out. Returns a result to handle unsupported platforms (equivalent to what glutin does).
Promoted Seat to a type. Visibility issues did drive me to create a NewIdType trait. Name is ugly, location is ugly, do you have a better idea? I really want crate visibility in rust, but that is only on nightly right now, right?
Added TouchSlot, already handled by the glutin backend
glutin does now send seat events on setting and clearing the InputHandler. Note that this api means, that we maybe maybe need to cache seats on other apis and send them to new InputHandlers. Alternative would be to provide one handler on construction of the InputBackends only.

Whats missing:

Handle time better most likely with Duration. How does libinput's time variable work? Is it absolute since the beginning of the context? Is it relative to the last event? The docs don't tell and I will not start emulation of that on glutin, until I know.
Provide device callbacks for configuration. (see https://wayland.freedesktop.org/libinput/doc/latest/group__device.html) Most likely backend specific device types, so we need some more generics.
Seat capabilities and callbacks.
Impl glium::Backend on to of OpenglRenderer and hide it behind a glium feature.
Rest of the libinput events
Docs

Drakulix · 2017-03-12T22:14:14Z

Alright the fourth draft fixes all remaining issues, that I wanted to solve before merging.
The rest (as seen above) makes sense to postpone until we have actual libinput bindings to experiment with (which is my next goal, if you don't have any objections).

Please give me some final feedback and once everything is good, I will rewrite and cleanup the (ugly) commit history of this branch and open it for merging.

Drakulix · 2017-03-13T14:40:52Z

Could you restart the nightly travis job? @vberger
Should work now, after the broken nightly yesterday

elinorbgr · 2017-03-13T15:37:16Z

Sure!

elinorbgr · 2017-03-13T16:24:54Z


 [dependencies]
-wayland-server = "0.8.4"
+wayland-server = { version = "0.8.4", features = ["dlopen"] }


Update to 0.8.6 for fixing the missing symbol error.

Also, we should make the "dlopen" feature conditioned on the glutin feature of smithay

Any clue on how to do this without renaming the glutin feature?

Maybe we can accept to rename the feature into something explicit and adopt some convention, I'm thinking something like:

for the backends: backend_glutin, backend_x11, backend_wayland, backend_tty

for the opengl apis: renderer_glium, renderer_gfx, (maybe renderer_vulkano some time in the future ? ^^)

And in general, a pattern like <function>_<implementation name> for every feature of this kind.

elinorbgr · 2017-03-13T16:33:41Z

+    ///
+    /// The linked `GlutinWindowedRenderer` will error with a lost Context and should
+    /// not be used anymore as well.
+    pub fn process_new_events(&mut self) -> Result<(), GlutinInputError>


I've started a trend of calling these kings of methods using the word dispatch, as they are called in the wayland libs. To fit this pattern, I'd name this method dispatch_pending_events.

elinorbgr · 2017-03-13T16:38:33Z

+        {
+            if let Some(ref mut handler) = self.handler {
+                match event {
+                    Event::KeyboardInput(state, key_code/*TODO: Is this really the keycode? glutins docs don't tell*/, _) => handler.on_keyboard_key(&self.seat, self.time_counter, key_code as u32, state.into(), 1),


For the wayland backend, of glutin, this is the raw keycode (truncated from u16 to u8). It's probably the same for others, afaik.

Also, this 1 means we do not track simultaneous key presses? See my comment on the definition of the trait.

We don't at least for glutin. I have no idea, how to get that information. Also yes the name and description of the variable is an error, see below.

elinorbgr · 2017-03-13T16:41:57Z

+    ContextLost,
+    /// The buffers have already been swapped.
+    ///
+    /// This error can be returned when `swap_buffers` has been called multiple times


"... multiple times without any modification in between.", is that what you mean here?

elinorbgr · 2017-03-13T16:43:25Z

+pub struct PixelFormat {
+    /// Is the format hardware accelerated
+    pub hardware_accelerated: bool,
+    /// bits used for colors


"number of bits...", same for the other fields of this struct

elinorbgr · 2017-03-13T16:54:36Z

+    /// - check if events can arrive out of order.
+    /// - Make stronger time guarantees
+    fn on_pointer_move(&mut self, seat: &Seat, time: u32, to: (u32, u32));
+    /// Called when a new keyboard event was received.


this is a pointer button event

elinorbgr · 2017-03-13T16:54:50Z

+    /// - check if events can arrive out of order.
+    /// - Make stronger time guarantees
+    fn on_pointer_button(&mut self, seat: &Seat, time: u32, button: MouseButton, state: MouseButtonState);
+    /// Called when a new keyboard event was received.


this is a pointer scroll event

elinorbgr · 2017-03-13T16:54:59Z

+    /// - check if events can arrive out of order.
+    /// - Make stronger time guarantees
+    fn on_pointer_scroll(&mut self, seat: &Seat, time: u32, axis: Axis, source: AxisSource, amount: f64);
+    /// Called when a new keyboard event was received.


this is a touch event

Copy and paste ftw....

elinorbgr · 2017-03-13T16:56:38Z

+    }
+
+    fn clear_handler(&mut self) {
+        if let Some(ref mut handler) = self.handler {


if let Some(handler) = self.handler.take() { ... } and removing the self.handler = None;

elinorbgr · 2017-03-13T17:01:51Z

+
+trait NewIdType {
+    fn new(id: u32) -> Self;
+}


I'm not a huge fan of this trait, but I see the use. I've been using freestanding functions to do the same job in other projects, which is not really better.

Hopefully pub(restricted) will save us somewhere in the future.

Yes I hate that too, totally unreasonable position, purely dictated by the visibility rules.

elinorbgr · 2017-03-13T17:07:31Z

I like the structure of this overall. Great work! 👍

The only thing in your list that I would like to see added before merging is the advertising of seat capability.

Simply something like adding a struct like this:

pub struct SeatCapability {
    pub pointer: bool,
    pub keyboard: bool,
    pub touch: bool
}

and a callback like this on the InputHandler, advertizing the new set of capabilities whenever it changes (and it never changes on the glutin backend, all booleans being set on true).

fn new_capability(&mut self, seat: Seat, capability: SeatCapability)

Drakulix · 2017-03-13T17:50:27Z

Still don't know, if I like this capability thing.
My preferred way to implement this would be to have these two:

seat.capabilties()
on_seat_changed()

Like this we could also add:
seat.input_devices() or something similar.

The trait is getting quite big already.

We (mostly you 😄 ) should decide, if we want to hide some less often used properties/events in the Seat struct or expose all of them (on_device_created is definitely also needed for configuration) and if we want to track all of that additionally in the Seat struct (see the reasoning on passing seat by-ref).

Also about the devices:

I expect that device configuration will be very different (or even not available) for all backends. How should we best deal with that? Maybe make the InputBackend generic over an InputDevice type? This would mean you cannot use a single InputHandler anymore for all different backends.

elinorbgr · 2017-03-13T17:54:29Z

Hmm, so you suggest we could have the Seat type give access to the seat characteristics, and a callback whenever they change, letting the user figure out what changed and if they need to do something? I guess it would work, I don't have any strong feeling about that.

Regarding device configuration, to be honest I don't understand what kind of configuration exists and should be exposed to the user... ?

Drakulix · 2017-03-13T18:01:14Z

I just have the fear, that it is too annoying to figure out what exactly has changed. Then again, if you really care, you can most likely just do the whole initialization of the device configurations again anyway without caring about the exact differences. Similar things should be doable for capabilities.

Most important configuration flags are e.g. for enabling tab_to_click on a track pad, or to control its sensitivity.
This is what sway does with it in it's configuration file: https://github.com/SirCmpwn/sway/blob/master/sway/sway-input.5.txt

elinorbgr · 2017-03-13T18:13:53Z

Hmm, I see...

As you said, these configuration will very likely be dependant on the backend (actually, I think only the libinput backend will have them).

I think this kind of stuff could remain as inherent impls on the backend type, don't you think?

Drakulix · 2017-03-13T21:34:49Z

I think this kind of stuff could remain as inherent impls on the backend type, don't you think?

But you need callbacks for that stuff as well. Would that mean, that you have to register two different handlers on the libinput backend in that case?

elinorbgr · 2017-03-14T06:55:22Z

Callbacks too here? What are the events that would be callback-ed on libinput? Is it just "a new device has been connected/disconnected", or something more complex?

elinorbgr · 2017-03-14T09:02:04Z

Also, as I've merged the inclusion of rustfmt & clippy in the travis build, could you rebase this PR on master?

Drakulix · 2017-03-15T12:04:44Z

Sure.

elinorbgr · 2017-03-17T18:23:13Z

I've been looking at weston's internals for #12, and I think we should find a better name for the graphics backend traits: they are not renderers, but rather the renderers will be built upon them (and may be part of smithay at some point too).

Drakulix · 2017-03-18T00:39:32Z

What about "graphics backends" then?

elinorbgr · 2017-03-18T08:09:20Z

So GlGraphicsBackend, and CpuGraphicsBackend ? That can work yeah.

Drakulix · 2017-03-19T20:57:48Z

Okay! Next round @vberger . I hope I have not forgotten anything.

elinorbgr · 2017-03-20T07:07:21Z

 }

-impl<T: OpenglRenderer> Backend for T
+pub struct GliumGraphicBackend<T: OpenglGraphicsBackend>(T);


Why did you need to introduce this struct and trait?

elinorbgr · 2017-03-20T07:13:12Z

(Ugh, again github showing my review as "outdated".)

Anyway, I like it ! 👍 I just wonder why you had to introduce this wrapper for glium, but otherwise I think we can merge this as a good starting point.

We'll probably need to make these API change & evolve as we dig into the other functionalities and discover needs & constraints we had not anticipated anyway 😅

Drakulix · 2017-03-20T13:23:40Z

I cannot do impl<T: OpenglGraphicsBackend> Backend for T, because then we implement an external trait for an exported trait. That means users of smithay could implement OpenglGraphicsBackend and get a free implementation of Backend, which might be unexpected, if they import glium as well. Thats why Rust does not allow that. We need to implement Backend for a specific struct, that is why I went with the wrapper approach, because I did not want to do the same implementation for both glutin headless and windowed structs and for every future implementation of OpenglGraphicsBackend.

elinorbgr · 2017-03-20T13:30:04Z

Oh right, I see. Forgot about coherence rules. All is right then. 👍

Drakulix · 2017-03-20T13:32:12Z

Okay, let me fix the formatting and remove the WIP tag an then feel free to merge.

Drakulix · 2017-03-20T13:42:00Z

Okay formatting is not fixed, because your travis config still uses rustfmt 0.7.*, which I did format with rustfmt 0.8. I guess we need to force install rustfmt or use something like cargo-update.

elinorbgr · 2017-03-20T13:52:30Z

Oh. Yes, I guess we could add something like this to the travis.yml

- which cargo-install-update || cargo install cargo-update
- cargo install-update -a

Can you add a commit doing it in this PR?

elinorbgr · 2017-03-20T14:53:55Z

Hmm, I think we can remove the reporting of TODOs and FIXMEs. This is far too early for it... 😅

Drakulix · 2017-03-20T14:56:08Z

Okay done. Lets hope that's it for this PR.

elinorbgr · 2017-03-20T15:16:00Z

The failure is clippy itself not compiling... welp, I'll merge.

Drakulix · 2017-03-20T15:17:44Z

Maybe for the future (because this can happen with any new nightly) allow nightly builds to fail.

elinorbgr reviewed Mar 7, 2017

View reviewed changes

elinorbgr reviewed Mar 9, 2017

View reviewed changes

elinorbgr reviewed Mar 13, 2017

View reviewed changes

Drakulix added 6 commits March 18, 2017 17:14

Add InputConfig

8e92c9c

Update features

4c2e0f8

Rename Renderer to GraphicsBackend

55e56ff

Fix glium support

3120683

Added SeatCapabilities

d53a7fc

Updated example

369c8a9

Drakulix force-pushed the feature/glutin branch from e6c8929 to 369c8a9 Compare March 19, 2017 20:57

elinorbgr reviewed Mar 20, 2017

View reviewed changes

Fix formatting

ae37562

Drakulix changed the title ~~[WIP] First draft of a glutin backend~~ First draft of a glutin backend Mar 20, 2017

Drakulix added 2 commits March 20, 2017 15:43

Fix travis using outdated rustfmt

6d20256

Fix example formatting

a0b8f53

Turn of Fixme and Todo reporting

8a1971f

elinorbgr merged commit f40bd92 into Smithay:master Mar 20, 2017

Drakulix deleted the feature/glutin branch March 20, 2017 15:17


		//TODO render stuff

		//TODO put input handling on the event loop

Conversation

Drakulix commented Mar 7, 2017

Uh oh!

elinorbgr commented Mar 7, 2017

Uh oh!

elinorbgr commented Mar 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Drakulix Mar 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Drakulix commented Mar 7, 2017

Uh oh!

elinorbgr commented Mar 7, 2017

Uh oh!

Drakulix commented Mar 7, 2017

Uh oh!

elinorbgr commented Mar 7, 2017

Uh oh!

Drakulix commented Mar 9, 2017

Uh oh!

elinorbgr commented Mar 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elinorbgr commented Mar 9, 2017

Uh oh!

elinorbgr commented Mar 9, 2017

Uh oh!

Drakulix commented Mar 9, 2017

Uh oh!

elinorbgr commented Mar 10, 2017

Uh oh!

Drakulix commented Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Drakulix Mar 7, 2017 •

edited

Loading

Drakulix commented Mar 10, 2017 •

edited

Loading

Drakulix commented Mar 12, 2017 •

edited

Loading