Add UiButton and UiButtonBuilder #613

Hittherhod · Mar 29, 2018

It's not showing up properly on my Mac, but I'd like to get a chance to iterate over the API and defaults.

This change is

jojolepro · Mar 29, 2018

jojolepro requested changes Mar 29, 2018

View changes

Consider adding a "ToEntities" trait which can be re-used between ui components to get a Vec from a UiButton or UiComponentName.

Most of it looks good though :)

jojolepro · Mar 29, 2018

amethyst_ui/src/button.rs

+    /// Construct a new UiButtonBuilder.
+    /// This allows easy use of default values for text and button appearance and allows the user
+    /// to easily set other UI-related options.
+    pub fn new(name: &'a str, world: &'b mut World) -> Self {


I think that having the text is an essential parameter here.

jojolepro · Mar 29, 2018

amethyst_ui/src/button.rs

+            );
+            let text = UiText::new(font, "".to_string(), [0.0, 0.0, 0.0, 1.0], 32.0);
+            let grey = loader.load_from_data(
+                [0.82, 0.83, 0.83, 1.0].into(),


Why the 0.82 and 0.83?

Review status: 0 of 11 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

amethyst_ui/src/button.rs, line 33 at r1 (raw file):

Previously, jojolepro (Joël Lupien) wrote…

I think that having the text is an essential parameter here.

Will do

amethyst_ui/src/button.rs, line 46 at r1 (raw file):

Previously, jojolepro (Joël Lupien) wrote…

Why the 0.82 and 0.83?

That was just a RGB color I found that is light gray

Comments from Reviewable

Hittherhod · Mar 29, 2018

Review status: 0 of 11 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

amethyst_ui/src/button.rs, line 33 at r1 (raw file):

Previously, jojolepro (Joël Lupien) wrote…

I think that having the text is an essential parameter here.

Will do

amethyst_ui/src/button.rs, line 46 at r1 (raw file):

Previously, jojolepro (Joël Lupien) wrote…

Why the 0.82 and 0.83?

That was just a RGB color I found that is light gray

Comments from Reviewable

jojolepro · Mar 29, 2018

jojolepro approved these changes Mar 29, 2018

View changes

jojolepro · Mar 29, 2018

amethyst_ui/src/lib.rs

+use specs::Entity;
+/// Ui elements will define this trait to allow easier addition to other
+/// elements and to the world.
+pub trait ToEntities {


That's good, but I also just realized that there's the Into<Vec> trait that could be implemented directly on the components. I'm not sure which one is preferable.

I'm not sure either. I think I'm leaning more towards "ToEntities" because it feels more explicit in its usage. Into might be more flexible though, and it is standard. I'll defer that to the group.

Where is this even used?

It isn't used. @jojolepro said it might be nice to have a trait to deconstruct Ui elements into their constituent entities so I added it in after the initial request. In the UI example we don't do anything to the button, so this trait isn't currently used.

@jojolepro Why exactly do you think such an ToEntities trait is useful?

torkleyy · Mar 30, 2018

@jojolepro Why exactly do you think such an ToEntities trait is useful?

When you have multiple menu and want to change between them, you need to remove the first menu before adding the second. (maybe the multiple World idea was good, after all). To easily do so, you can create all your ui inside a vec like so

fn create_ui(....) -> Vec<Entity>{
  vec![
    UiButton::new(.......).to_entities(),
    ...
  ]
}

fn change_menu(){
  for e in old_entities{
    entities.delete(e);
  }
}

jojolepro · Mar 30, 2018

When you have multiple menu and want to change between them, you need to remove the first menu before adding the second. (maybe the multiple World idea was good, after all). To easily do so, you can create all your ui inside a vec like so

fn create_ui(....) -> Vec<Entity>{
  vec![
    UiButton::new(.......).to_entities(),
    ...
  ]
}

fn change_menu(){
  for e in old_entities{
    entities.delete(e);
  }
}

If they all have one ui node as parent, wouldn't that be better?

torkleyy · Mar 30, 2018

If they all have one ui node as parent, wouldn't that be better?

Technically I think there will always be a root entity and the rest as childs, so it could return only a single entity.

jojolepro · Mar 30, 2018

Technically I think there will always be a root entity and the rest as childs, so it could return only a single entity.

Could you walk the tree to get all the entities you want?

Hittherhod · Mar 30, 2018

Could you walk the tree to get all the entities you want?

If not, that functionality should be added.

torkleyy · Mar 30, 2018

If not, that functionality should be added.

We can't really walk from parent to child without iterating over all entities.

jojolepro · Mar 30, 2018

We can't really walk from parent to child without iterating over all entities.

Would you already know a fairly local parent (like, say, the menu) that you could use as the root of your search?

Hittherhod · Mar 31, 2018

Would you already know a fairly local parent (like, say, the menu) that you could use as the root of your search?

Well actually, you just made me realize we could use a "root" ui entity to delete a menu all at once.

When you delete an entity, all the childs are deleted automatically by the parent/layout system, so I would guess that by using UiButton::new(...).with_parent(root_menu) and then deleting the root_menu entity, the problem is fixed, and we don't need the ToEntities trait.

jojolepro · Mar 31, 2018

Well actually, you just made me realize we could use a "root" ui entity to delete a menu all at once.

When you delete an entity, all the childs are deleted automatically by the parent/layout system, so I would guess that by using UiButton::new(...).with_parent(root_menu) and then deleting the root_menu entity, the problem is fixed, and we don't need the ToEntities trait.

Okay. I'll delete the ToEntities trait.

Hittherhod · Mar 31, 2018

Okay. I'll delete the ToEntities trait.

torkleyy · Mar 31, 2018

torkleyy approved these changes Mar 31, 2018

View changes

Thank you, looks great now!

torkleyy · Mar 31, 2018

amethyst_ui/src/button.rs

+    /// Construct a new UiButtonBuilder.
+    /// This allows easy use of default values for text and button appearance and allows the user
+    /// to easily set other UI-related options.
+    pub fn new<S: ToString>(name: &'a str, text: S, world: &'b mut World) -> Self {


Can we pass the World to build instead of new?

jojolepro · Mar 31, 2018

jojolepro reviewed Mar 31, 2018

View changes

jojolepro · Mar 31, 2018

examples/ui/main.rs

+            })
+            .with_anchored(Anchored::new(Anchor::TopMiddle))
+            .with_parent(Parent {
+                entity: background.clone(),
             })
             .build();
 


Could you add another button showing a less heavy syntax?
e.g: UiButtonBuilder::new("btn2", "Simple Button").with_font(font.clone()).with_position(50.0,50.0).build(&mut world);

Rhuagh · Mar 31, 2018

Rhuagh reviewed Mar 31, 2018

View changes

Passing world to build instead of new, and adding a build method for LazyUpdate aswell as one for World would be good. And make the new() function take only the resources it needs rather than World (Loader, AssetStorage, AssetStorage)

Basically, my concern is that we should make it possible to use the builder without access to the full world (for creation from inside systems)

Rhuagh · Mar 31, 2018

Basically, my concern is that we should make it possible to use the builder without access to the full world (for creation from inside systems)

@Rhuagh Could you explain more about LazyUpdate? I've never seen it in use, so I'm not sure how to work with it.

Hittherhod · Apr 2, 2018

@Rhuagh Could you explain more about LazyUpdate? I've never seen it in use, so I'm not sure how to work with it.

https://github.com/amethyst/amethyst-rhusics/blob/ffbd88bb3650f65026d3cb6664a8475d535dddb4/examples/boxes/emission.rs#L79

Rhuagh · Apr 2, 2018

https://github.com/amethyst/amethyst-rhusics/blob/ffbd88bb3650f65026d3cb6664a8475d535dddb4/examples/boxes/emission.rs#L79

There's an example

Rhuagh · Apr 2, 2018

There's an example

Review status: 0 of 12 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

amethyst_ui/src/button.rs, line 33 at r4 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Can we pass the World to build instead of new?

Done.

examples/ui/main.rs, line 203 at r4 (raw file):

Previously, jojolepro (Joël Lupien) wrote…

Could you add another button showing a less heavy syntax?
e.g: UiButtonBuilder::new("btn2", "Simple Button").with_font(font.clone()).with_position(50.0,50.0).build(&mut world);

Done.

Comments from Reviewable

Hittherhod · Apr 2, 2018

Review status: 0 of 12 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

amethyst_ui/src/button.rs, line 33 at r4 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Can we pass the World to build instead of new?

Done.

examples/ui/main.rs, line 203 at r4 (raw file):

Previously, jojolepro (Joël Lupien) wrote…

Could you add another button showing a less heavy syntax?
e.g: UiButtonBuilder::new("btn2", "Simple Button").with_font(font.clone()).with_position(50.0,50.0).build(&mut world);

Done.

Comments from Reviewable

jojolepro · Apr 2, 2018

jojolepro approved these changes Apr 2, 2018

View changes

Rhuagh · Apr 2, 2018

Rhuagh requested changes Apr 2, 2018

View changes

Rhuagh · Apr 2, 2018

amethyst_ui/src/button.rs

+const DEFAULT_TXT_COLOR: [f32; 4] = [0.0, 0.0, 0.0, 1.0];
+const DEFAULT_FONT_NAME: &'static str = "font/square.ttf";
+
+/// Container that wraps the resources we need to initialize button defaults


#[derive(SystemData)]

What does SystemData do?

Makes it so you can use the struct directly in a system.

Rhuagh · Apr 2, 2018

amethyst_ui/Cargo.toml

@@ -27,4 +27,4 @@ shrev = "0.8.1"
 specs = "0.10"
 unicode-normalization = "0.1"
 unicode-segmentation = "1.2"
-winit = "0.10"
+winit = "0.11"


shred-derive = "0.3"

Rhuagh · Apr 2, 2018

amethyst_ui/src/lib.rs

@@ -22,17 +22,19 @@ extern crate unicode_segmentation;
 extern crate winit;


#[macro_use]
extern crate shred_derive;

Done. I needed to add shred as well to make the compiler be quiet.

Rhuagh · Apr 2, 2018

amethyst_ui/src/button.rs

+impl <'a> UiButtonResources<'a> {
+    /// Grab the resources we need from the world. 
+    pub fn from_world(world: &World) -> UiButtonResources {
+        UiButtonResources {


Self::fetch(&world.res, 0)

Rhuagh · Apr 2, 2018

amethyst_ui/src/button.rs

+    }
+
+    /// Create the UiButton based on provided configuration parameters.
+    pub fn build(mut self, world: &mut World) -> UiButton {


Make another of the "Resources" structs, that contain the storages required for creating a button (WriteStorage<'a, UiImage> etc), and make the build function take that resources struct. Make the resources struct contain Entities<'a> so you can create entities.

How do I create replicate the current World centric functionality with Storages and Entities? Does this new Resources struct also need to derive SystemData?

Hittherhod · Apr 2, 2018

Hittherhod reviewed Apr 2, 2018

View changes

Continuing conversation points raised by @Rhuagh

Hittherhod · Apr 2, 2018

amethyst_ui/src/button.rs

+const DEFAULT_TXT_COLOR: [f32; 4] = [0.0, 0.0, 0.0, 1.0];
+const DEFAULT_FONT_NAME: &'static str = "font/square.ttf";
+
+/// Container that wraps the resources we need to initialize button defaults


What does SystemData do?

Hittherhod · Apr 2, 2018

amethyst_ui/src/button.rs

+    }
+
+    /// Create the UiButton based on provided configuration parameters.
+    pub fn build(mut self, world: &mut World) -> UiButton {


How do I create replicate the current World centric functionality with Storages and Entities? Does this new Resources struct also need to derive SystemData?

Hittherhod · Apr 2, 2018

amethyst_ui/src/lib.rs

@@ -22,17 +22,19 @@ extern crate unicode_segmentation;
 extern crate winit;


Done. I needed to add shred as well to make the compiler be quiet.

@Rhuagh I've added changes to use the WriteStorage resources structure. How would this work with a LazyUpdate?

Hittherhod · Apr 4, 2018

@Rhuagh I've added changes to use the WriteStorage resources structure. How would this work with a LazyUpdate?

Rhuagh · Apr 5, 2018

Rhuagh reviewed Apr 5, 2018

View changes

Rhuagh · Apr 5, 2018

amethyst_ui/src/button.rs

+    /// Create the UiButton based on provided configuration parameters.
+    pub fn build_from_world(self, world: &World) -> UiButton {
+        self.build(UiButtonBuilderResources::from_world(world))
+    }
 
     /// Lazily build the UiButton. Need to call `World::maintain` to have the values actually added.
     pub fn lazy_build(mut self, res: UiButtonLazyResources) -> UiButton {


You can remove this, it's no longer needed, and LazyUpdate might disappear soon (or atleast be redesigned since), since it has issues.

@Rhuagh Any feedback on the changes? I've removed the LazyUpdate structure. I want to add an example of having the button text update if you click on the button, but my click events aren't registering (the "you clicked on this element" isn't getting printed). @jojolepro Could this be another Mac specific issue?

Hittherhod · Apr 7, 2018

@Rhuagh Any feedback on the changes? I've removed the LazyUpdate structure. I want to add an example of having the button text update if you click on the button, but my click events aren't registering (the "you clicked on this element" isn't getting printed). @jojolepro Could this be another Mac specific issue?

@Hittherhod You are not adding the MouseReactive component to the button's image.
You are not reading the UiEvents either.

jojolepro · Apr 7, 2018

@Hittherhod You are not adding the MouseReactive component to the button's image.
You are not reading the UiEvents either.

Where do I read the UiEvents? I was trying to modify the UiEventHandlerSystem in main.rs, but it's not pushed because I didn't want to add a broken commit.

Hittherhod · Apr 7, 2018

Where do I read the UiEvents? I was trying to modify the UiEventHandlerSystem in main.rs, but it's not pushed because I didn't want to add a broken commit.

Actually, the example is in fact reading for events :P my bad
https://github.com/jojolepro/amethyst/blob/develop/examples/ui/main.rs#L295

You are still missing MouseReactive tho

jojolepro · Apr 7, 2018

Actually, the example is in fact reading for events :P my bad
https://github.com/jojolepro/amethyst/blob/develop/examples/ui/main.rs#L295

You are still missing MouseReactive tho

So, I added the MouseReactive and I see HoverStart and HoverStop events, but I'm not seeing click events.

Hittherhod · Apr 7, 2018

So, I added the MouseReactive and I see HoverStart and HoverStop events, but I'm not seeing click events.

I see the clicks
:works_on_my_pc: :)

jojolepro · Apr 7, 2018

I see the clicks
:works_on_my_pc: :)

Okay. I'll refrain from adding that because I can't test it, but it'd be a nice to have for another PR.

Hittherhod · Apr 7, 2018

Okay. I'll refrain from adding that because I can't test it, but it'd be a nice to have for another PR.

Do you mind screenshotting yours?

jojolepro · Apr 7, 2018

Do you mind screenshotting yours?

Nevermind. I get Click events, but it's only in a specific section. I think it's probably something to do with the Retina display.

Hittherhod · Apr 7, 2018

Nevermind. I get Click events, but it's only in a specific section. I think it's probably something to do with the Retina display.

We're waiting for a bugfix in winit that triggers the retina bugs.

Rhuagh · Apr 8, 2018

We're waiting for a bugfix in winit that triggers the retina bugs.

Rhuagh · Apr 10, 2018

Rhuagh approved these changes Apr 10, 2018

View changes

Theres a compile issue in amethyst_input/src/local_virtual_key_code.rs:157:5, Caret is defined twice, which cause compilation to fail.

Rhuagh · Apr 10, 2018

Theres a compile issue in amethyst_input/src/local_virtual_key_code.rs:157:5, Caret is defined twice, which cause compilation to fail.

@Rhuagh Fixed it. Something weird happened with rebasing on upstream and Caret was defined at the end of the KeyCodes section as well as in the correct alphabetical ordering. I tried to keep things current throughout the pull request, but I think next time I'll just do a final one at the end.

Hittherhod · Apr 10, 2018

@Rhuagh Fixed it. Something weird happened with rebasing on upstream and Caret was defined at the end of the KeyCodes section as well as in the correct alphabetical ordering. I tried to keep things current throughout the pull request, but I think next time I'll just do a final one at the end.

Doesn't seem to be fixed really. Could you also try to clean up the commit history?

torkleyy · Apr 10, 2018

Doesn't seem to be fixed really. Could you also try to clean up the commit history?

Ah the build is just failing because of Travis.

torkleyy · Apr 10, 2018

Ah the build is just failing because of Travis.

Sure. I’ll try to squash things to just include the major fixes.

…

On Tue, Apr 10, 2018 at 7:43 AM Thomas Schaller ***@***.***> wrote: Ah the build is just failing because of Travis. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#613 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABVdnk890ayzXv6CtmZgpJ6NI7wZCqGVks5tnJrpgaJpZM4TAc6d> .

Hittherhod · Apr 10, 2018

Sure. I’ll try to squash things to just include the major fixes.

…

On Tue, Apr 10, 2018 at 7:43 AM Thomas Schaller ***@***.***> wrote: Ah the build is just failing because of Travis. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#613 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABVdnk890ayzXv6CtmZgpJ6NI7wZCqGVks5tnJrpgaJpZM4TAc6d> .

@Rhuagh I tried to clean it up so that it only reports the significant updates rather than every little change.

Hittherhod · Apr 11, 2018

@Rhuagh I tried to clean it up so that it only reports the significant updates rather than every little change.

jojolepro · Apr 11, 2018

jojolepro reviewed Apr 11, 2018

View changes

jojolepro · Apr 11, 2018

amethyst_ui/src/button.rs

+
+/// Container for all the resources the builder needs to make a new UiButton.
+#[derive(SystemData)]
+struct UiButtonBuilderResources<'a> {


Resources is a confusing name, could you use something like "parameters"?

I do not know how to remove a message from my phone, ignore this.

Looks good to me :)

jojolepro · Apr 11, 2018

Looks good to me :)

Is there some way to make this merge?

Hittherhod · Apr 13, 2018

Is there some way to make this merge?

@jojolepro @Rhuagh @torkleyy Since you've all marked this as approved can one of you 3 r+ this? I'm hesitant to do so myself as I haven't reviewed it yet but if there are any pending change requests can we change the review status to reflect that?

Xaeroxe · Apr 13, 2018

@jojolepro @Rhuagh @torkleyy Since you've all marked this as approved can one of you 3 r+ this? I'm hesitant to do so myself as I haven't reviewed it yet but if there are any pending change requests can we change the review status to reflect that?

bors r=torkleyy,Rhuagh

Rhuagh · Apr 13, 2018

bors r=torkleyy,Rhuagh

👎 Rejected by label

bors · Apr 13, 2018

👎 Rejected by label

bors r=torkleyy,Rhuagh

Rhuagh · Apr 13, 2018

bors r=torkleyy,Rhuagh

Build succeeded

bors · Apr 13, 2018

Build succeeded

Congratz!

jojolepro · Apr 13, 2018

Congratz!

jojolepro added type: improvement type: feature status: working project: ui labels Mar 29, 2018

jojolepro referenced this pull request Apr 3, 2018
Merged
Update winit, glutin and gfx_window_glutin. #620

Rhuagh added status: ready and removed status: working labels Apr 13, 2018

Hittherhod referenced this pull request Apr 15, 2018
Closed
Add buttons to UI #577

amethyst/amethyst

Join GitHub today

Add UiButton and UiButtonBuilder #613

Conversation

Hittherhod commented Mar 29, 2018 • edited by torkleyy Edited 1 time torkleyy edited Mar 29, 2018 (most recent)

jojolepro added type: improvement type: feature status: working project: ui labels Mar 29, 2018

jojolepro requested changes Mar 29, 2018 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Hittherhod commented Mar 29, 2018

jojolepro approved these changes Mar 29, 2018 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

torkleyy commented Mar 30, 2018

This comment has been minimized.

jojolepro commented Mar 30, 2018

This comment has been minimized.

torkleyy commented Mar 30, 2018

This comment has been minimized.

jojolepro commented Mar 30, 2018

This comment has been minimized.

Hittherhod commented Mar 30, 2018

This comment has been minimized.

torkleyy commented Mar 30, 2018

This comment has been minimized.

jojolepro commented Mar 30, 2018

This comment has been minimized.

Hittherhod commented Mar 31, 2018

This comment has been minimized.

jojolepro commented Mar 31, 2018

This comment has been minimized.

Hittherhod commented Mar 31, 2018

torkleyy approved these changes Mar 31, 2018 View changes

This comment has been minimized.

jojolepro reviewed Mar 31, 2018 View changes

This comment has been minimized.

Rhuagh reviewed Mar 31, 2018 View changes

This comment has been minimized.

Rhuagh commented Mar 31, 2018

This comment has been minimized.

Hittherhod commented Apr 2, 2018

This comment has been minimized.

Rhuagh commented Apr 2, 2018

This comment has been minimized.

Rhuagh commented Apr 2, 2018

This comment has been minimized.

Hittherhod commented Apr 2, 2018

jojolepro approved these changes Apr 2, 2018 View changes

Rhuagh requested changes Apr 2, 2018 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Hittherhod reviewed Apr 2, 2018 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jojolepro referenced this pull request Apr 3, 2018

This comment has been minimized.

Hittherhod commented Apr 4, 2018

Rhuagh reviewed Apr 5, 2018 View changes

This comment has been minimized.

This comment has been minimized.

Hittherhod commented Apr 7, 2018

This comment has been minimized.

jojolepro commented Apr 7, 2018 • edited Edited 1 time jojolepro edited Apr 7, 2018 (most recent)

This comment has been minimized.

Hittherhod commented Apr 7, 2018

This comment has been minimized.

jojolepro commented Apr 7, 2018

This comment has been minimized.

Hittherhod commented Mar 29, 2018 •

edited by torkleyy

Edited 1 time

torkleyy edited Mar 29, 2018 (most recent)

jojolepro requested changes Mar 29, 2018

View changes

jojolepro approved these changes Mar 29, 2018

View changes

torkleyy approved these changes Mar 31, 2018

View changes

jojolepro reviewed Mar 31, 2018

View changes

Rhuagh reviewed Mar 31, 2018

View changes

jojolepro approved these changes Apr 2, 2018

View changes

Rhuagh requested changes Apr 2, 2018

View changes

Hittherhod reviewed Apr 2, 2018

View changes

Rhuagh reviewed Apr 5, 2018

View changes

jojolepro commented Apr 7, 2018 •

edited

Edited 1 time

jojolepro edited Apr 7, 2018 (most recent)

jojolepro commented Apr 7, 2018 •

edited

Edited 1 time

jojolepro edited Apr 7, 2018 (most recent)