[Review] UI Layouts

[jojolepro]

Adds layouting systems.
Anchor based layouting: You define which anchor you want, that what offset in pixel you want from there.
e.g TopRight + x: 100, y: 75

Stretching: Stretch the size of a UiTransform to the size of his parents on one or two axes. Uses the screen size if no parent is available.

Stretch margins: Creates space on the sides of something stretched.

See examples/ui for a usage example.

Also, fixes text not being visible if the z isn't between [-1,1]. (Thanks @Xaeroxe )

Once the review process is done, I'll squash the commits.

---

This change is 

[Xaeroxe]

LGTM! Thanks a ton!

[jojolepro]

@AlisCode Proposed to integrate the percent scale mode.
e.g 0.05 width = 5% of screen width.

You can review this PR, but don't merge it. Thanks!

[jojolepro]

@AlisCode Proposed to integrate the percent scale mode.
e.g 0.05 width = 5% of screen width.

You can review this PR, but don't merge it. Thanks!

[jojolepro]

Except if we find a last minute bug, this is ready for one more review then merge. Be sure to test it yourself to make sure there's no weird corner cases I might have forgot to test. Be aware that a UiText is always aligned at the top left of his UiTransform.

[jojolepro]

Except if we find a last minute bug, this is ready for one more review then merge. Be sure to test it yourself to make sure there's no weird corner cases I might have forgot to test. Be aware that a UiText is always aligned at the top left of his UiTransform.

[torkleyy]

Reviewed 1 of 7 files at r1, 5 of 6 files at r3.
Review status: 6 of 7 files reviewed at latest revision, 3 unresolved discussions.

---

amethyst_ui/src/layout.rs, line 22 at r4 (raw file):

/// Follow a normal english Y,X naming.
#[derive(Debug, Clone)]
pub enum Anchor {

Could we make this a vector maybe? Like (0.0, 0.0) is the bottom left, (0.5, 1.0) top middle, etc.

---

amethyst_ui/src/layout.rs, line 119 at r4 (raw file):

    /// Adds a margin (spacing) on one or multiple axes.
    pub fn with_margin(mut self, x: f32, y: f32) -> Self {

Add to constructor? I think it's annoying to call extra methods here.

---

amethyst_ui/src/transform.rs, line 36 at r4 (raw file):

    pub calculated_y: f32,
    /// Calculated z position by the `UiParentSystem` and `UiLayoutSystem`.
    pub calculated_z: f32,

Do we want / need this? Should this be global_x and local_x maybe?

---

Comments from Reviewable

[torkleyy]

Reviewed 1 of 7 files at r1, 5 of 6 files at r3.
Review status: 6 of 7 files reviewed at latest revision, 3 unresolved discussions.

---

amethyst_ui/src/layout.rs, line 22 at r4 (raw file):

/// Follow a normal english Y,X naming.
#[derive(Debug, Clone)]
pub enum Anchor {

Could we make this a vector maybe? Like (0.0, 0.0) is the bottom left, (0.5, 1.0) top middle, etc.

---

amethyst_ui/src/layout.rs, line 119 at r4 (raw file):

    /// Adds a margin (spacing) on one or multiple axes.
    pub fn with_margin(mut self, x: f32, y: f32) -> Self {

Add to constructor? I think it's annoying to call extra methods here.

---

amethyst_ui/src/transform.rs, line 36 at r4 (raw file):

    pub calculated_y: f32,
    /// Calculated z position by the `UiParentSystem` and `UiLayoutSystem`.
    pub calculated_z: f32,

Do we want / need this? Should this be global_x and local_x maybe?

---

Comments from Reviewable

[Rhuagh]

Reviewed 1 of 7 files at r1, 4 of 6 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

amethyst_ui/src/layout.rs, line 22 at r4 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Could we make this a vector maybe? Like (0.0, 0.0) is the bottom left, (0.5, 1.0) top middle, etc.

The issue with that scheme is that it's not immediately apparent if 0.0 is top, bottom, left or right. I could go either way on this one.

---

amethyst_ui/src/layout.rs, line 119 at r4 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Add to constructor? I think it's annoying to call extra methods here.

Personally, I'd prefer the function and not expand the constructor too much. But again, I could go either way here.

---

amethyst_ui/src/layout.rs, line 150 at r4 (raw file):

    fn run(&mut self, (entities, mut transform, mut anchor, parent, screen_dim): Self::SystemData) {
        for (entity, mut tr, mut anchor) in (&*entities, &mut transform, &mut anchor).join() {
            if anchor.offset.is_none() {

This will only work if the viewport is never resized, I think ?

---

amethyst_ui/src/layout.rs, line 163 at r4 (raw file):

                let middle = (screen_dim.width() / 2.0, screen_dim.height() / 2.0);

                let new_pos_x = middle.0 + norm_offset.0 * screen_dim.width() + user_offset.0;

This only works for anchoring in the viewport, not on a parent ?

---

amethyst_ui/src/layout.rs, line 389 at r4 (raw file):

        // When you don't have a parent but do have stretch on, resize with screen size.
        for (entity, mut local) in (&*entities, &mut locals).join() {

I believe you can add &stretch to the join here, and not do stretch.get() below, to get a smaller result set from the join. The logic should be the same, just an inversion of the fetch.

---

amethyst_ui/src/transform.rs, line 36 at r4 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Do we want / need this? Should this be global_x and local_x maybe?

I'm in favor of those names.

---

Comments from Reviewable

[Rhuagh]

Reviewed 1 of 7 files at r1, 4 of 6 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

amethyst_ui/src/layout.rs, line 22 at r4 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Could we make this a vector maybe? Like (0.0, 0.0) is the bottom left, (0.5, 1.0) top middle, etc.

The issue with that scheme is that it's not immediately apparent if 0.0 is top, bottom, left or right. I could go either way on this one.

---

amethyst_ui/src/layout.rs, line 119 at r4 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Add to constructor? I think it's annoying to call extra methods here.

Personally, I'd prefer the function and not expand the constructor too much. But again, I could go either way here.

---

amethyst_ui/src/layout.rs, line 150 at r4 (raw file):

    fn run(&mut self, (entities, mut transform, mut anchor, parent, screen_dim): Self::SystemData) {
        for (entity, mut tr, mut anchor) in (&*entities, &mut transform, &mut anchor).join() {
            if anchor.offset.is_none() {

This will only work if the viewport is never resized, I think ?

---

amethyst_ui/src/layout.rs, line 163 at r4 (raw file):

                let middle = (screen_dim.width() / 2.0, screen_dim.height() / 2.0);

                let new_pos_x = middle.0 + norm_offset.0 * screen_dim.width() + user_offset.0;

This only works for anchoring in the viewport, not on a parent ?

---

amethyst_ui/src/layout.rs, line 389 at r4 (raw file):

        // When you don't have a parent but do have stretch on, resize with screen size.
        for (entity, mut local) in (&*entities, &mut locals).join() {

I believe you can add &stretch to the join here, and not do stretch.get() below, to get a smaller result set from the join. The logic should be the same, just an inversion of the fetch.

---

amethyst_ui/src/transform.rs, line 36 at r4 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Do we want / need this? Should this be global_x and local_x maybe?

I'm in favor of those names.

---

Comments from Reviewable

[Rhuagh]

Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

amethyst_ui/src/layout.rs, line 163 at r4 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

This only works for anchoring in the viewport, not on a parent ?

Ah, that's handled in the other system.

---

Comments from Reviewable

[Rhuagh]

Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

amethyst_ui/src/layout.rs, line 163 at r4 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

This only works for anchoring in the viewport, not on a parent ?

Ah, that's handled in the other system.

---

Comments from Reviewable

[Rhuagh]

Reviewed 1 of 6 files at r3.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

Comments from Reviewable

[Rhuagh]

Reviewed 1 of 6 files at r3.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

Comments from Reviewable

[torkleyy]

Reviewed 1 of 6 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

amethyst_ui/src/layout.rs, line 22 at r4 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

The issue with that scheme is that it's not immediately apparent if 0.0 is top, bottom, left or right. I could go either way on this one.

Later on the enum gets transformed into coords anyway.

---

amethyst_ui/src/layout.rs, line 119 at r4 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Personally, I'd prefer the function and not expand the constructor too much. But again, I could go either way here.

It' just that I think we want a margin rather often, so you have to call this method almost always.

---

Comments from Reviewable

[torkleyy]

Reviewed 1 of 6 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

amethyst_ui/src/layout.rs, line 22 at r4 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

The issue with that scheme is that it's not immediately apparent if 0.0 is top, bottom, left or right. I could go either way on this one.

Later on the enum gets transformed into coords anyway.

---

amethyst_ui/src/layout.rs, line 119 at r4 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Personally, I'd prefer the function and not expand the constructor too much. But again, I could go either way here.

It' just that I think we want a margin rather often, so you have to call this method almost always.

---

Comments from Reviewable

[jojolepro]

Review status: 3 of 8 files reviewed at latest revision, 6 unresolved discussions.

---

amethyst_ui/src/layout.rs, line 22 at r4 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Later on the enum gets transformed into coords anyway.

I prefer the way it is because its really obvious as to where the anchor is.

---

amethyst_ui/src/layout.rs, line 119 at r4 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

It' just that I think we want a margin rather often, so you have to call this method almost always.

I personally use margins minimally. I prefer using pixel size and positioning than margins.

---

amethyst_ui/src/layout.rs, line 150 at r4 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

This will only work if the viewport is never resized, I think ?

Fixed, thanks a lot!

---

amethyst_ui/src/layout.rs, line 163 at r4 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Ah, that's handled in the other system.

Done.

---

amethyst_ui/src/layout.rs, line 389 at r4 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

I believe you can add &stretch to the join here, and not do stretch.get() below, to get a smaller result set from the join. The logic should be the same, just an inversion of the fetch.

This happened because there was an else clause before that got removed I think. Done.

---

amethyst_ui/src/transform.rs, line 36 at r4 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

I'm in favor of those names.

Done.

---

Comments from Reviewable

[jojolepro]

Review status: 3 of 8 files reviewed at latest revision, 6 unresolved discussions.

---

amethyst_ui/src/layout.rs, line 22 at r4 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Later on the enum gets transformed into coords anyway.

I prefer the way it is because its really obvious as to where the anchor is.

---

amethyst_ui/src/layout.rs, line 119 at r4 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

It' just that I think we want a margin rather often, so you have to call this method almost always.

I personally use margins minimally. I prefer using pixel size and positioning than margins.

---

amethyst_ui/src/layout.rs, line 150 at r4 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

This will only work if the viewport is never resized, I think ?

Fixed, thanks a lot!

---

amethyst_ui/src/layout.rs, line 163 at r4 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Ah, that's handled in the other system.

Done.

---

amethyst_ui/src/layout.rs, line 389 at r4 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

I believe you can add &stretch to the join here, and not do stretch.get() below, to get a smaller result set from the join. The logic should be the same, just an inversion of the fetch.

This happened because there was an else clause before that got removed I think. Done.

---

amethyst_ui/src/transform.rs, line 36 at r4 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

I'm in favor of those names.

Done.

---

Comments from Reviewable

[torkleyy]

Reviewed 5 of 5 files at r5.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

amethyst_ui/src/layout.rs, line 119 at r4 (raw file):

Previously, jojolepro (Joël Lupien) wrote…

I personally use margins minimally. I prefer using pixel size and positioning than margins.

I still think it belongs to the constructor since there won't be many more things to add here.

---

Comments from Reviewable

[torkleyy]

Reviewed 5 of 5 files at r5.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

amethyst_ui/src/layout.rs, line 119 at r4 (raw file):

Previously, jojolepro (Joël Lupien) wrote…

I personally use margins minimally. I prefer using pixel size and positioning than margins.

I still think it belongs to the constructor since there won't be many more things to add here.

---

Comments from Reviewable

[jojolepro]

#601
The ui code contains a duplicate of the transform system, and thus contains the same bugfix.
The performance optimisation requested in that PR's reviews can't be applied before the new version of specs is released, because I can't go from an entity id back to the entity in without looking through all of them (yet).
I suggest we merge this when everyone is happy with it, and I modify the #601 PR when the new specs is available to optimise both parenting codes.

[jojolepro]

#601
The ui code contains a duplicate of the transform system, and thus contains the same bugfix.
The performance optimisation requested in that PR's reviews can't be applied before the new version of specs is released, because I can't go from an entity id back to the entity in without looking through all of them (yet).
I suggest we merge this when everyone is happy with it, and I modify the #601 PR when the new specs is available to optimise both parenting codes.

[jojolepro]

Review status: 6 of 8 files reviewed at latest revision, 6 unresolved discussions.

---

amethyst_ui/src/layout.rs, line 119 at r4 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

I still think it belongs to the constructor since there won't be many more things to add here.

Done.

---

Comments from Reviewable

[jojolepro]

Review status: 6 of 8 files reviewed at latest revision, 6 unresolved discussions.

---

amethyst_ui/src/layout.rs, line 119 at r4 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

I still think it belongs to the constructor since there won't be many more things to add here.

Done.

---

Comments from Reviewable

[torkleyy]

---

Reviewed 2 of 2 files at r6.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[torkleyy]

---

Reviewed 2 of 2 files at r6.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[Rhuagh]

Reviewed 3 of 5 files at r5, 2 of 2 files at r6.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

amethyst_ui/src/layout.rs, line 111 at r6 (raw file):

impl Stretched {
    /// Create a new `Stretched` component using the `Stretch` setting.
    pub fn new(stretch: Stretch,margin_x: f32,margin_y: f32) -> Self {

Formatting.

---

amethyst_ui/src/layout.rs, line 124 at r6 (raw file):

/// Used to initialize the `UiTransform` and `Anchored` offsets when using the `Anchored` component.
pub struct UiLayoutSystem{

Formatting

---

amethyst_ui/src/layout.rs, line 133 at r6 (raw file):

        UiLayoutSystem {
            screen_size: (0.0,0.0),
        }

Formatting

---

amethyst_ui/src/text.rs, line 358 at r6 (raw file):

                                .join()
                                .filter(|&(_, t)| {
                                    t.local_x <= self.mouse_position.0

Should this really be the local coordinates, is the mouse position also described in local coordinates?

---

Comments from Reviewable

[Rhuagh]

Reviewed 3 of 5 files at r5, 2 of 2 files at r6.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

amethyst_ui/src/layout.rs, line 111 at r6 (raw file):

impl Stretched {
    /// Create a new `Stretched` component using the `Stretch` setting.
    pub fn new(stretch: Stretch,margin_x: f32,margin_y: f32) -> Self {

Formatting.

---

amethyst_ui/src/layout.rs, line 124 at r6 (raw file):

/// Used to initialize the `UiTransform` and `Anchored` offsets when using the `Anchored` component.
pub struct UiLayoutSystem{

Formatting

---

amethyst_ui/src/layout.rs, line 133 at r6 (raw file):

        UiLayoutSystem {
            screen_size: (0.0,0.0),
        }

Formatting

---

amethyst_ui/src/text.rs, line 358 at r6 (raw file):

                                .join()
                                .filter(|&(_, t)| {
                                    t.local_x <= self.mouse_position.0

Should this really be the local coordinates, is the mouse position also described in local coordinates?

---

Comments from Reviewable

[jojolepro]

Review status: 5 of 8 files reviewed at latest revision, 4 unresolved discussions.

---

amethyst_ui/src/layout.rs, line 111 at r6 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Formatting.

Done.

---

amethyst_ui/src/layout.rs, line 124 at r6 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Formatting

Done.

---

amethyst_ui/src/layout.rs, line 133 at r6 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Formatting

Done.

---

amethyst_ui/src/text.rs, line 358 at r6 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Should this really be the local coordinates, is the mouse position also described in local coordinates?

Done. Thanks for that!

---

Comments from Reviewable

[jojolepro]

Review status: 5 of 8 files reviewed at latest revision, 4 unresolved discussions.

---

amethyst_ui/src/layout.rs, line 111 at r6 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Formatting.

Done.

---

amethyst_ui/src/layout.rs, line 124 at r6 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Formatting

Done.

---

amethyst_ui/src/layout.rs, line 133 at r6 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Formatting

Done.

---

amethyst_ui/src/text.rs, line 358 at r6 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Should this really be the local coordinates, is the mouse position also described in local coordinates?

Done. Thanks for that!

---

Comments from Reviewable

[torkleyy]

---

Reviewed 1 of 3 files at r7.
Review status: 6 of 8 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[torkleyy]

---

Reviewed 1 of 3 files at r7.
Review status: 6 of 8 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[torkleyy]

Reviewed 2 of 3 files at r7.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[torkleyy]

Reviewed 2 of 3 files at r7.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[jojolepro]

I just realized that changing transform.x to transform.local_x broke all projects using ui...

[jojolepro]

I just realized that changing transform.x to transform.local_x broke all projects using ui...

[Rhuagh]

Don't be afraid of doing breaking changes to unreleased code.

[Rhuagh]

Don't be afraid of doing breaking changes to unreleased code.

[jojolepro]

I meant I didn't update the examples :D pong doesn't compile

[jojolepro]

I meant I didn't update the examples :D pong doesn't compile

[Rhuagh]

Yeah, I noticed

[Rhuagh]

Yeah, I noticed

[jojolepro]

Fixed @Rhuagh

[jojolepro]

Fixed @Rhuagh

[Rhuagh]

---

Reviewed 2 of 3 files at r7, 2 of 2 files at r8.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[Rhuagh]

---

Reviewed 2 of 3 files at r7, 2 of 2 files at r8.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[Rhuagh]

bors r=torkleyy,Rhuagh

[Rhuagh]

bors r=torkleyy,Rhuagh

[jojolepro]

Anyone on mac can test and report if the text is inside or outside the top button?
cargo run --example ui

[jojolepro]

Anyone on mac can test and report if the text is inside or outside the top button?
cargo run --example ui

[bors]

Build succeeded

• continuous-integration/appveyor/branch
• continuous-integration/travis-ci/push

[bors]

Build succeeded

• continuous-integration/appveyor/branch
• continuous-integration/travis-ci/push