Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Breaking Change] Replace elmesque crate with primitive graphics widgets (see comment for more details). #626

Merged
merged 124 commits into from
Dec 12, 2015

Conversation

mitchmindtree
Copy link
Contributor

This is a WIP - Do not merge just yet.

This is quite a hefty change, and in turn is leaking into quite a few areas:

  • Add the widget::primitive module, hosting all primitive widgets. This module should contain all the primitive widgets a widget designer could possibly need to implement their own complex widgets.
  • Added an example demonstrating each of the primitive widgets.
  • Widgets will now compose themselves of primitive graphics widgets rather than drawing their own Elements or Forms. This allows us to remove the elmesque crate and use our own graphics layout instead, in turn providing a far simpler (and more efficient) graphics backend.
  • Theme will now support custom widget styling (rather than hard-coded style types).
  • Added methods to the Sizeable trait that allow setting a Widget's dimensions to match the dimensions of some other Widget.
  • Added a TitleBar widget, which Canvas will now use instead of its own custom TitleBar type.
  • Added a Widget::picking_passthrough(bool) method which allows a user to indicate whether their widget should be pick-able by the mouse or other widget picking methods. This is useful for primitive graphics widgets that are only designed to be used as graphical elements, and that are not themselves interactive.
  • Fixed a bug in Graph::scroll_offset where the behaviour would be incorrect if two widgets shared both Edge::Depth and Edge::Position.
  • Removed Label widget in favour of new Text primitive widget that handles automatic line-wrapping, line-spacing, etc.
  • Moved color.rs module from elmesque into conrod.
  • Removes rustc-serialize as the first step towards using serde (perhaps as a feature).

All-in-all I think this should be a massive API improvement, and allow conrod to be a lot simpler to work with regarding graphics (elmesque has been a common point of confusion). It should also be much faster than elmesque as drawing will no longer require near as many allocations.

Closes #421.
Closes #466.
Closes #506.
Closes #509.
Closes #522.
Closes #555.
Makes a start on #576.
Closes #581.
Closes #582.
Closes #616.
Closes #617.
Fixes #628.
Closes #629.

@mitchmindtree
Copy link
Contributor Author

The Theme serialisation will also need a re-do due to the addition of handling 3rd party Widget::Styles.

Perhaps this is an opportunity to move away from the rustc_serialize crate in favour of serde.

@bvssvni
Copy link
Member

bvssvni commented Nov 15, 2015

I think moving to serde now is a good idea.

@pyrossh
Copy link

pyrossh commented Nov 17, 2015

Nice! Me like performance! Hulk Smash! :hurtrealbad:

@mitchmindtree
Copy link
Contributor Author

One issue that I've come across regarding the use of Widgets as primitive graphical elements of other Widgets is that it is easy to make some slightly different assumptions about the behaviour of their relationships, compared to regular widget relationships.

Say that we have _a -> b_ where our child _b_ is a Widget being used as a graphical element of the parent Widget _a. In this case, we'll say that _b is a Rectangle widget being used as a graphical element for a Canvas (a container kind of widget) _a_. This allows:

  • Use of the Positionable methods to position _b_ relative to _a_, i.e. .middle_of(canvas_idx).
  • Use of the Sizeable methods to size _b_ relative to _a_, i.e. .dim_of(canvas_idx).
  • _a_ doesn't need a draw method, as _b_ takes care of the drawing.
  • _a_ is the implied depth parent of _b_ as _b_ was placed upon _a_ using the .middle_of method.

There are a few issues that are easy to overlook here:

  1. If our Canvas _a_ is instantiated as scrollable by a user, then our Rectangle _b's position will be offset by _a's scrolling, because we described _b_ as a depth child of _a... a disaster! What we actually want is for _b to stay fixed to _a_ (even though it is a depth child of _a) as _b is actually a graphical element of _a_ and not just another child widget.
  2. When a user moves their mouse over our Canvas _a, the internal pick_widget method always picks _b instead of _a, as _b covers the entirety of _a's area... another disaster! What we actually want is for _b to be considered a graphical element of _a, so that whenever pick_widget picks _b at the top, it actually returns the index for _a_.
  3. Say we want to place a TitleBar widget _c_ at the top of our Canvas _a. We could say that, if our Canvas _a has some TitleBar _c, then we want its kid_area to be the remaining area directly under the TitleBar _c. This means that widgets placed upon the Canvas _a_ will be correctly placed in the area under the TitleBar _c. Now, in _a's Widget::update, we need to actually intantiate and position our TitleBar _c. Intuitively we want to write title_bar.mid_top_of(canvas_idx).width_of(canvas_idx), which should make sense. However, remember that _c is a depth child of _a, meaning that when we say .mid_top_of, we're actually saying that we want to position it at the top of _a's kid_area, and not the actual top of _a_ as we assumed... disaster! We need conrod to know that _c_ is actually a graphical element of _a, and that this implies it should actually be positioned upon _a's total area.

I think this clearly suggests that we need some way of describing that a widget can be a graphical element of some other widget. The question is, what is the ideal way of representing this within conrod? I think there are a few options:

  • The first solution that I thought of was to have each Node::Widget contain its own internal Graph allowing for each widget to have its own complex graphics widgets layout that is clearly a part of the widget itself. Despite this pretty clearly solving the above issues, I think it might introduce a fair bit of unnecessary complexity. It adds another dimension of traversal by adding the addition of Graphs within Graphs. We would also likely lose cache coherence; at the moment all widgets sit within the Graph's single contiguous node slice, however this solution would require allocating a new slice for every widget that has its own group of graphical elements (this would be the case for most widgets), likely slowing down graph traversal considerably.

  • I then thought it might be possible to instead add unique fields that describe our desired behaviour to the widget Container stored within each Node::Widget within the Graph. For example, there could be:

    • attach_scroll_offset_to (None by default): when Some(other_idx) would describe that a widget should collect its scroll_offset via some other widget's depth position within the graph rather than its own. This would allow us to attach a child graphics widget to some parent, solving the problem described in the 1st issue above.
    • picking_passthrough (false by default): describing whether or not a widget should be ignored within the picking algorithms. This would fix the 2nd issue.
    • place_on_kid_area (true by default): describing whether or not a widget should be placed upon the parent's kid_area. If false, it would instead be placed upon the parent's total area. This would fix the 3rd issue.

    This is nice in that we could offer a user control over the behaviour of each individual issue described above. My concern is that the majority of the time, the behaviour of all of these are related to a single question, "is the widget a graphical element for some other widget or not?". Thus perhaps instead of these 3 fields we could add just one; graphical_element_of which is None by default, and Some(idx) where idx is the index of the widget for which our widget is a graphical element. This brings upon the realisation that, what we are actually describing here is a relationship between two separate widgets. This leads to the next possible solution...

  • Add a Graphic variant to the Edge enum, which describes the exact relationship I mentioned at the start of this comment: _a -> b_ where our child _b_ is a Widget being used as a graphical element of the parent Widget _a_. I think this is currently my favourite option, as it seems to be the simplest, clearest way of describing the graphical relationship between two widgets while still using the same graph and in turn, the same contiguous slice of memory.

API-wise, there are a couple of options for allowing the user to describe this relationship:

  • we could provide a .graphics_for(other_idx) builder method on the Widget trait, which would allow a user to specify that the widget they're currently instantiating is simply a graphical element of the widget at the given index (I think this is my preferred option).
  • add an alternative to .set(idx, &mut ui), such as .set_graphics_for(other_idx, idx, &mut ui).
  • always assume that primitive widgets are graphical elements of their parent widgets? This might be the simplest option, however it probably isn't a safe assumption to make as I can already imagine situations in which a user might expect a primitive to behave just like a regular widget.

@masche842
Copy link

@mitchmindtree I like where this is going! As mentioned in #616 I played around with this today.
It has been easy to set up a custom backend for piston2d-graphics and I think I'm fine with this dependency (for me it will only behave as an interface anyway as I can't use one of the implemented backends).
But there's a thing that confuses me: graphics already defines methods for rectangles, ellipses, etc.. But in your current revision (861726a) you do reimplement them (for the ellipses even with fixed resolution). Is this just an interim stage because elmesque doesn't offer the means for describing these primitives as Element?

This leads to my second question: I then tried to get rid of elmesque to get a feeling for what has to be done. I ended up calling widget::draw in ui::draw, assembling DrawState from Cache. This does the job but feels kind of messy. Do you already have an architecture in mind that minimizes the necessary changes in widgets?

I'm really looking forward to that pull request being finished & merged, would be a pleasure to help!

@bvssvni
Copy link
Member

bvssvni commented Nov 18, 2015

@mitchmindtree Thanks for the info! It was clear and well written. I think adding a Graphics edge variant sounds good.

mitchmindtree added a commit to mitchmindtree/conrod that referenced this pull request Nov 22, 2015
…s_for method to describe when a widget is solely a graphical element of some other widget. Addresses comment in PistonDevelopers#626
@mitchmindtree
Copy link
Contributor Author

Sorry about the wait everyone, will hopefully have this done in the next day or two! 🙏

@bvssvni
Copy link
Member

bvssvni commented Nov 29, 2015

@mitchmindtree No worries! Take your time.

@mitchmindtree
Copy link
Contributor Author

As I had to re-implement the text drawing, I thought I might as well have a go at getting auto-wrapping multi-line text going - here's a WIP:

screen shot 2015-12-02 at 9 08 45 pm

Still some tweaking to go, it supports left-aligned, centred and right-aligned text with custom line spacing.

@pyrossh
Copy link

pyrossh commented Dec 2, 2015

Nice! Could this be possibly used to create a fast Text Editor for Rust or will the Immediate mode GUI make it more slow and memory intensive?

@mitchmindtree
Copy link
Contributor Author

@pyros2097 it should be! Conrod is all retained under the hood (it re-uses allocations, strings, textures etc like regular GUIs) and provides ways for drawing lazily, so I don't see why we shouldn't be able to reach speeds close to that of regular retained GUIs, but we'll see 😅

@bvssvni
Copy link
Member

bvssvni commented Dec 3, 2015

@mitchmindtree This is going to be the most epic PR ever!

@bvssvni bvssvni mentioned this pull request Dec 3, 2015
@bvssvni
Copy link
Member

bvssvni commented Dec 6, 2015

This PR needs rebasing.

@mitchmindtree
Copy link
Contributor Author

@masche842 That would be awesome if you do get a chance (the minimalised Graphics abstraction, that is)! I think a lot of people would appreciate it. That way we could possibly make piston-graphics an optional default cargo feature instead, so that by default it's still easy to get started using piston, but there's an option to not use it whatsoever if that is what the user requires 👍

@mitchmindtree
Copy link
Contributor Author

Should be fiiiiinally ready 🎉

@bvssvni
Copy link
Member

bvssvni commented Dec 11, 2015

Wow!

@mitchmindtree
Copy link
Contributor Author

About to head to sleep but will merge and publish this when I wake up if no one beats me to it (or has any issues) 👍

…ly compare instantiation order in the following sort
mitchmindtree added a commit that referenced this pull request Dec 12, 2015
[Breaking Change] Replace elmesque crate with primitive graphics widgets (see comment for more details).
@mitchmindtree mitchmindtree merged commit 2c96eb1 into PistonDevelopers:master Dec 12, 2015
@mitchmindtree
Copy link
Contributor Author

Published as 0.24.0!

@mitchmindtree mitchmindtree deleted the graphics_widgets branch December 12, 2015 00:59
@ebfull
Copy link
Contributor

ebfull commented Dec 12, 2015

Awesome work! 🎉

@masche842
Copy link

Thanks @mitchmindtree for this huge and awesome PR!
I think conrod profits and will profit a lot from this! 👍 👍 👍

As discussed before I added a sketch for a simple backend implementation here: #658

@sjmackenzie
Copy link

Epic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants