piston_window

piston_window copied to clipboard

I'm trying to merge a conrod specific waiting event loop to conrod, so examples can use it along with PistonWindow. https://github.com/PistonDevelopers/conrod/pull/831 It works, but @mitchmindtree brings up the fair point that it's a hacky solution in that, we can use PistonWindow with an alternative event loop, but the default event loop still gets instantiated by PistonWindow and just not used.

Aside from being a little inefficient, it leads to a confusing API, where two event loops could be called, and it's not at all clear that PistonWindow can be used this way.

My original solution was to separate the event loop from the window entirely, but it does break the current API, and requires applications to instantiate the event loop themselves.

@bvssvni you suggested before that maybe Conrod should just maintain a fork of PistonWindow, a "ConrodWindow". I feel that even if PistonWindow is intended to be convenient rather than flexible for all use cases, PistonWindow still comprises some code that arguably would be better shared rather than duplicated downstream. What would be the benefit to a fork?

That said, I'm not too opinionated on this, I laid out some alternatives here

I experimented with having the event loop in PistonWindow be generic, so that PistonWindow is generic on both the window backend, and the event loop. Then external event loops just have to match a trait that exposes a next method.

Does this sound like a reasonable approach? By setting the default type for the generic event loop to be the piston event loop, as it is for GlutinWindow for the window backend, the API for the common case can remain the same. Although adding a new type parameter to PistonWindow could also be a breaking change for some code.

The reason I don't like changes to piston_window is because:

• It depends on the OpenGL backend of Gfx
• It is not meant to be used by libraries, because it is not generic and therefore not reusable
• There is nothing wrong in principle with multiple window wrappers, this is how the Piston core is intended to be used - this cost is gained by the large number of libraries that can work across various backends
• Abstractions should be in the Piston core, not in window wrappers, and we try to keep the abstractions to a minimum

Considering that piston_window is only 300 loc, and most of it is just boilerplate, I think it is OK to just duplicate the code.

There is also the case that window wrappers can learn from each other. More experimenting means better design over time.

I argue for a conrod_window because:

1. It fixes the problem (don't forget that this is the most important part!)
2. Conrod can make it even easier to set up a new UI application
3. Maintenance can happen in parallel, more freedom to move in separate directions

@bvssvni I remember talking to you a long time ago and mentioning the idea that it would be really nice if there was a simple crate which just picked one Window backend and one Graphics backend and made it easy to get started. I believe I mentioned this to you at the time because I wanted to reduce the amount of boilerplate in conrod examples, and thought perhaps we could use it to reduce boilerplate in other examples across the ecosystem too. You mentioned you thought it was a good idea and then started the piston_window crate and linked me to it, so I switched the conrod examples to use this (to clarify, no other part of conrod has ever depended on piston_window).

Now from my understanding it seems that piston_window's purpose might have gradually changed over time to mean something else? Or perhaps it was always something else and I never realised? Would you mind explaining to me what your intentions for piston_window are today and what you imagine it would be useful for?

I don't mind seeing a conrod-specific Window+loop added to conrod's piston feature if that is what is necessary, but the reason I've been reluctant to do so is that I assumed these use-cases (like easily setting up examples) were the point of piston_window in the first place.

What if we change the event loop in pistoncore-event_loop to support both modes, and then add a flag to WindowSettings? I like this idea better than adding a generic parameter, but it still doesn't feel nice, since WindowSettings is not supposed to do anything about event loops.

The solution I like the most, at least for now, is to maintain separate window wrappers. With Eco this is easy. Read the blog post how to use Eco for more information.

Later, when we have thought about this some more, we might made some changes to the core. At this point I don't know what changes we should make, so I prefer an option that allows us to decouple the design constraints.

piston_window => games
conrod_window => UI applications

@mitchmindtree The reason piston_window was started was because of some Gfx design choices that introduced too much boilerplate (I believe there was a window trait at some point that made me start it). However, this is irrelevant for this particular problem, and piston_window stuck around because it was convenient. Of course Conrod influenced the design.

Ok, it sounds like the ConrodWindow approach makes the most sense right now, I agree it's not much code and the design could benefit from evolving separately.

I agree that adding to WindowSettings is better than adding a generic parameter, but also that it doesn't belong in WindowSettings either. As for modifying the pistoncore-event_loop to support both modes, I think that potentially pistoncore-event_loop could benefit from those changes, but right now Conrod doesn't need all the events and features it uses, and I also think there will inevitably be more use cases where the needs will diverge more and it's better to have precedent for replacing it than trying to fit all use cases into one component. I can foresee apps needing to customize their own event loops, if, for example, they use more than one thread, or potentially in use cases like this: https://github.com/PistonDevelopers/piston_window/issues/167

So @mitchmindtree, if it sounds good to you, I can make a pull request for Conrod to add ConrodWindow and remove the piston_window dependency, I'll add it under the piston feature.

I like the idea of adding some sort of flag or mode. It might even be useful to switch between both modes at runtime (i.e. a game's main menu vs real-time gameplay screen).

@christolliday sure, this sounds OK for now :+1: It would be nice if we can come up with a better name than ConrodWindow - I feel like the Conrod prefix shouldn't be necessary as it will be defined within the conrod crate.