UI text input #489

Xaeroxe · Nov 8, 2017

This is a first implementation that's missing a couple features such as multi-line text and
double click to highlight a word.

This also rewrites pieces of the renderer so that rendering can be done in the main thread without requiring execution in the thread pool.

torkleyy · Nov 11, 2017

torkleyy reviewed Nov 11, 2017

View changes

torkleyy · Nov 11, 2017

amethyst_ui/src/focused.rs

@@ -0,0 +1,7 @@
+use specs::Entity;
+
+/// This resource with id 0 stores the currently focused UI element.


I don't think we need to describe the id anywhere. IDs are for scripting, there's no reason to use it for anything else.

All of our rendering passes currently have much improved performance when executing serially rather than in parallel. In addition if you had more passes than cores there was actually a chance this would draw out of order. So that bug was fixed in this PR. If a pass needs parallel execution it should be able to retrieve our ThreadPool from the specs::World.

However one remaining problem with this approach is how does a parallel pass knows when to flush its encoders? Right now there isn't a correct way for the pass to do this. One possible solution for this is to flush the encoder after every pass rather than at the end of all passes, which would work but also means potentially inferior batching. I'll have to play around with the approach and see if we lose too much FPS from doing that.

Xaeroxe · Nov 13, 2017

All of our rendering passes currently have much improved performance when executing serially rather than in parallel. In addition if you had more passes than cores there was actually a chance this would draw out of order. So that bug was fixed in this PR. If a pass needs parallel execution it should be able to retrieve our ThreadPool from the specs::World.

However one remaining problem with this approach is how does a parallel pass knows when to flush its encoders? Right now there isn't a correct way for the pass to do this. One possible solution for this is to flush the encoder after every pass rather than at the end of all passes, which would work but also means potentially inferior batching. I'll have to play around with the approach and see if we lose too much FPS from doing that.

Things that are now working in text editing:

Ctrl + shift + arrow key to select words
Using the left and right arrow keys without modifiers when text is highlighted
Home and End keys work correctly
Clipboard works correctly

Things that aren't working

Anything mouse related
Up and down arrow keys
Tabbing between fields

Xaeroxe · Nov 19, 2017

Things that are now working in text editing:

Ctrl + shift + arrow key to select words
Using the left and right arrow keys without modifiers when text is highlighted
Home and End keys work correctly
Clipboard works correctly

Things that aren't working

Anything mouse related
Up and down arrow keys
Tabbing between fields

I also learned this doesn't handle multi line text very well, because I made the assumption early on that glyphs == graphemes. While the two concepts are very similar graphemes such as a new line, are not glyphs.

Xaeroxe · Nov 20, 2017

I also learned this doesn't handle multi line text very well, because I made the assumption early on that glyphs == graphemes. While the two concepts are very similar graphemes such as a new line, are not glyphs.

I think tabbing between fields is something that should be implemented by another PR.

torkleyy · Nov 21, 2017

I think tabbing between fields is something that should be implemented by another PR.

Along with "focused" and "focus-able" components.

jojolepro · Nov 21, 2017

Along with "focused" and "focus-able" components.

@torkleyy @jojolepro these aren't even the hard part of this PR, seriously those pieces are easy and I basically already have it down how I'm going to handle them. Focusing components is already built on the data side I just haven't coded the input piece yet.

Honestly the thing that's got my head turning is multi-line text. That's going to be a bit complicated with the way this is built currently.

Xaeroxe · Nov 21, 2017

@torkleyy @jojolepro these aren't even the hard part of this PR, seriously those pieces are easy and I basically already have it down how I'm going to handle them. Focusing components is already built on the data side I just haven't coded the input piece yet.

Honestly the thing that's got my head turning is multi-line text. That's going to be a bit complicated with the way this is built currently.

Also, you know how when entering text into a single line field if the text is wider than the input box the box will auto-advance the rendered text to the cursor? This doesn't do that yet, and it really should. I think it'll be as as simple as using the glyph widths and advancing by those when needed, but it just hasn't been coded yet.

Xaeroxe · Nov 21, 2017

Also, you know how when entering text into a single line field if the text is wider than the input box the box will auto-advance the rendered text to the cursor? This doesn't do that yet, and it really should. I think it'll be as as simple as using the glyph widths and advancing by those when needed, but it just hasn't been coded yet.

torkleyy · Nov 25, 2017

torkleyy reviewed Nov 25, 2017

View changes

torkleyy · Nov 25, 2017

amethyst_ui/src/pass.rs

+                let rendered_string = if ui_text.password {
+                    // Build a string composed of black dot characters.
+                    let mut ret = String::with_capacity(ui_text.text.len());
+                    for _grapheme in ui_text.text.graphemes(true) {


Could just do ui_text.text.graphemes(true).map(|_| 'u{2022}').collect()

This has the disadvantage that the string isn't allocated with the correct capacity, thus potentially causing more allocations as the string gets resized.

torkleyy · Nov 25, 2017

amethyst_ui/src/pass.rs

+                    }
+                    ret
+                } else {
+                    ui_text.text.clone()


We don't need to clone; just return a reference above

I'm building a small Amethyst demo atm, it would be nice to have some text fields there. When do you plan on merging it?

torkleyy · Nov 25, 2017

I'm building a small Amethyst demo atm, it would be nice to have some text fields there. When do you plan on merging it?

@torkleyy do you need multi-line text? I can probably finish this much more quickly if I cut that

Xaeroxe · Nov 25, 2017

@torkleyy do you need multi-line text? I can probably finish this much more quickly if I cut that

I don't need multiline input, no. I'd like to finish the demo on Wednesday, the input fields would just be nice, not necessary.

torkleyy · Nov 25, 2017

I don't need multiline input, no. I'd like to finish the demo on Wednesday, the input fields would just be nice, not necessary.

There are two features missing from this that I'd really like to get in. Clicking on fields to focus and/or highlight them doesn't work, and if your text is wider than your box it just won't render.

The first feature I'm not completely sure what to do with, as we need double clicks for that. Winit doesn't emit double clicks, I'd like it to but it's non-trivial to make it emit double clicks because a double click event was considered out of scope for X11, so you have to retrieve it from elsewhere on the system. How you fetch that changes from machine to machine. We could just ignore the system double click time setting and just use 500 ms everywhere, but that feels sub-optimal. However that said it might be better than just doing nothing. I have not researched this very thoroughly so if anyone has any corrections to offer please tell me.

Getting the second feature in is likely going to require changes to gfx_glyph so that the text will still continue to render if it's too big, just only in the box allocated to it. We needed maximum text length anyways, so I added that early as a stop-gap to help avoid triggering this bug.

Xaeroxe · Nov 25, 2017

There are two features missing from this that I'd really like to get in. Clicking on fields to focus and/or highlight them doesn't work, and if your text is wider than your box it just won't render.

The first feature I'm not completely sure what to do with, as we need double clicks for that. Winit doesn't emit double clicks, I'd like it to but it's non-trivial to make it emit double clicks because a double click event was considered out of scope for X11, so you have to retrieve it from elsewhere on the system. How you fetch that changes from machine to machine. We could just ignore the system double click time setting and just use 500 ms everywhere, but that feels sub-optimal. However that said it might be better than just doing nothing. I have not researched this very thoroughly so if anyone has any corrections to offer please tell me.

Getting the second feature in is likely going to require changes to gfx_glyph so that the text will still continue to render if it's too big, just only in the box allocated to it. We needed maximum text length anyways, so I added that early as a stop-gap to help avoid triggering this bug.

So while this is still missing double click to highlight a word, and multi-line text it is otherwise ready and in my opinion a satisfactory first implementation. So I'm removing the WIP.

Ready for review!

Xaeroxe · Nov 26, 2017

So while this is still missing double click to highlight a word, and multi-line text it is otherwise ready and in my opinion a satisfactory first implementation. So I'm removing the WIP.

Ready for review!

r? @torkleyy

This is ready.

Xaeroxe · Nov 26, 2017

r? @torkleyy

This is ready.

Rhuagh · Nov 26, 2017

Rhuagh reviewed Nov 26, 2017

View changes

Just a general suggestion, next time we do a large refactor of something, try to keep it in it's own PR. Makes the PR smaller and more focused towards a specific feature.

Rhuagh · Nov 26, 2017

amethyst_renderer/src/pipe/pass.rs

-    type Apply: ParallelIterator<Item = ()>;
-}
-
+/// Used to fetch data from the game world for rendering in your pass.


the pass. maybe ?

Rhuagh · Nov 26, 2017

amethyst_renderer/src/pipe/pass.rs

 }
 
+/// A compiled pass.  These are created and managed by the `Renderer`.  You should never need to


This should not be used directly outside the renderer ?

Rhuagh · Nov 26, 2017

examples/renderable/main.rs

         .with::<ExampleSystem>(ExampleSystem, "example_system", &[])
+        .with_frame_limit(FrameRateLimitStrategy::Unlimited, 0)


Remove this maybe?

I was using this to benchmark the example's performance so I could see how text rendering impacted the FPS. I decided to leave it because it's probably nice to show off the frame rate anyways on an example called renderable. What do you think?

As long as peoples computers don't burn to cinders I'm fine with it :P

Well, mine didn't :D

Rhuagh · Nov 26, 2017

amethyst_ui/src/text.rs

+    /// The font used for rendering.
+    pub font: FontHandle,
+    /// If true this will be rendered as dots instead of the text.
+    pub password: bool,


Not sure password is the correct field name for this. hidden maybe? Or maybe an enum for how to render the text (Normal, Hidden, Dotted) or some such ? Not sure, just an idea.

What would be the difference between Hidden and Dotted?

Hidden = nothing, Dotted= the current behavior for password

What would be the use case for hidden in this context? I know sometimes that's used for password entry in console programs but I haven't seen anywhere else use it.

travis-ci failure: oci runtime error: exec failed: container_linux.go:265: starting container process caused "could not create session key: disk quota exceeded

Nice job travis. I'll wait for them to go run out and buy a hard drive I guess.

Xaeroxe · Nov 26, 2017

travis-ci failure: oci runtime error: exec failed: container_linux.go:265: starting container process caused "could not create session key: disk quota exceeded

Nice job travis. I'll wait for them to go run out and buy a hard drive I guess.

I think they run on EC2, so it's more like "gives Amazon a larger heap of money"

Rhuagh · Nov 26, 2017

I think they run on EC2, so it's more like "gives Amazon a larger heap of money"

Aceeri · Nov 27, 2017

Aceeri approved these changes Nov 27, 2017

View changes

Looks good! Should probably just open up another issue for multi-line text so we don't just keep adding features with this PR.

Aceeri · Nov 27, 2017

amethyst_ui/src/lib.rs

 pub use self::transform::UiTransform;
+
+/// How many times the cursor blinks per second while editing text.
+const CURSOR_BLINK_RATE: f32 = 2.0;


Probably make this a resource eventually.

When I asked if this should be hardcoded no one seemed to care to make it configurable, so a resource might be overkill.

Not a resource imho, possibly a configurable system parameter at some point.

It should be configurable, but let's leave it for now.

Aceeri · Nov 27, 2017

amethyst_ui/src/pass.rs

-        let ui_transform = &ui_transform;
-        let unit_mesh = &unit_mesh;
-
+    ) {
         // Populate and update the draw order cache.
         {


I feel like some of this could be improved with FlaggedStorage/TrackedStorage (performance wise), but it seems fine for now.

yeah I thought about that, it turns out the overhead from those storage types actually ends up being worse than this setup, but only because this setup is very specifically optimized for tracking changes to one field.

Aceeri · Nov 27, 2017

amethyst_ui/src/pass.rs

+                // Render background highlight
+                let brush = &mut self.glyph_brushes
+                    .get_mut(&ui_text.brush_id.unwrap())
+                    .unwrap()


Are all these unwraps safe?

yes they are, because the code beforehand proved the unwrap safe. The only reason option values are used here is because the structures can't be populated at initialization.

Aceeri · Nov 27, 2017

amethyst_ui/src/text.rs

+                                                .h_metrics()
+                                                .advance_width)
+                                            - mouse_x)
+                                            .abs()


This is some weird formatting...

rustfmt-nightly 0.2.16 official. But yeah it does look odd.

torkleyy · Nov 27, 2017

torkleyy approved these changes Nov 27, 2017

View changes

Great, thank you for your work!

torkleyy · Nov 27, 2017

amethyst_ui/src/bundle.rs

         let reader = world
             .read_resource::<EventChannel<Event>>()
             .register_reader();
         Ok(
             builder
-                .add(UiTextRenderer, "ui_text", self.deps)
+                .add(UiSystem::new(reader.clone()), "ui_system", &[])
                 .add(Processor::<FontAsset>::new(), "font_processor", &[])


It would be better if the processor was before the ui system

torkleyy · Nov 27, 2017

amethyst_ui/src/lib.rs

 pub use self::transform::UiTransform;
+
+/// How many times the cursor blinks per second while editing text.
+const CURSOR_BLINK_RATE: f32 = 2.0;


It should be configurable, but let's leave it for now.

torkleyy · Nov 27, 2017

amethyst_ui/src/pass.rs

+            GlyphBrush<'static, Resources, Factory>,
+            WeakHandle<FontAsset>,
+        ),
+    >,


Ugh, a typedef would probably be better here.

It's 100% internal and this is the only place it's written out. Would you still have me use a typedef for this?

It just doesn't look very nice in code, that's all.

torkleyy · Nov 27, 2017

amethyst_ui/src/pass.rs

-                    };
-                    let vbuf = match mesh.buffer(PosTex::ATTRIBUTES) {
-                        Some(vbuf) => vbuf.clone(),
+        let vbuf = match mesh.buffer(PosTex::ATTRIBUTES) {


In a few weeks we can use ? for this :)

I've rebased, addressed review comments, and have two approvals. This is also over 24 hours old so I'm going to go ahead and merge this.

@Rhuagh if we want to change the field name still we can do that in a follow-up PR.

bors r+

Xaeroxe · Nov 27, 2017

I've rebased, addressed review comments, and have two approvals. This is also over 24 hours old so I'm going to go ahead and merge this.

@Rhuagh if we want to change the field name still we can do that in a follow-up PR.

bors r+

This is also over 24 hours old so I'm going to go ahead and merge this.

While that's true, it wasn't ready for review for 24 hours. That said, I don't have any concerns, so it's fine by me.

torkleyy · Nov 27, 2017

This is also over 24 hours old so I'm going to go ahead and merge this.

While that's true, it wasn't ready for review for 24 hours. That said, I don't have any concerns, so it's fine by me.

It seems you made a commit after bors merged your branch. Are you sure it gets included?

torkleyy · Nov 27, 2017

It seems you made a commit after bors merged your branch. Are you sure it gets included?

@torkleyy Yes the commits are being included correctly. The timestamps were a little off because my local machine disagrees with Github on what time it is by a few minutes but the correct commits are there.

I'll make sure to time it from the review opening period in the future, that's a good note.

Xaeroxe · Nov 27, 2017

@torkleyy Yes the commits are being included correctly. The timestamps were a little off because my local machine disagrees with Github on what time it is by a few minutes but the correct commits are there.

I'll make sure to time it from the review opening period in the future, that's a good note.

Build succeeded

bors · Nov 27, 2017

Build succeeded

Xaeroxe added project: rendering status: working type: feature labels Nov 8, 2017

Xaeroxe referenced this pull request Nov 15, 2017
Merged
Useful time additions. #491

Xaeroxe changed the title from [WIP] Ui text input to [WIP] UI text input Nov 17, 2017

Xaeroxe changed the title from [WIP] UI text input to UI text input Nov 26, 2017

Xaeroxe added status: ready and removed status: working labels Nov 26, 2017

Rearrange system graph and use type def for brush cache

Loading status checks…

ff6305b

bors bot merged commit ff6305b into amethyst:develop Nov 27, 2017
3 checks passed

3 checks passed

bors Build succeeded

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Xaeroxe deleted the Xaeroxe:ui-text-input branch Nov 27, 2017

Aceeri added this to the 0.6.0 milestone Nov 28, 2017

amethyst/amethyst

Join GitHub today

UI text input #489

Conversation

Xaeroxe commented Nov 8, 2017 • edited Edited 4 times Xaeroxe edited Nov 26, 2017 (most recent) Xaeroxe edited Nov 26, 2017 Xaeroxe edited Nov 25, 2017 Xaeroxe edited Nov 19, 2017

Xaeroxe added project: rendering status: working type: feature labels Nov 8, 2017

torkleyy reviewed Nov 11, 2017 View changes

This comment has been minimized.

This comment has been minimized.

Xaeroxe commented Nov 13, 2017 • edited Edited 1 time Xaeroxe edited Nov 13, 2017 (most recent)

Xaeroxe referenced this pull request Nov 15, 2017

Xaeroxe changed the title from [WIP] Ui text input to [WIP] UI text input Nov 17, 2017

This comment has been minimized.

Xaeroxe commented Nov 19, 2017

This comment has been minimized.

Xaeroxe commented Nov 20, 2017

This comment has been minimized.

torkleyy commented Nov 21, 2017

This comment has been minimized.

jojolepro commented Nov 21, 2017

This comment has been minimized.

Xaeroxe commented Nov 21, 2017

This comment has been minimized.

Xaeroxe commented Nov 21, 2017

torkleyy reviewed Nov 25, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

torkleyy commented Nov 25, 2017

This comment has been minimized.

Xaeroxe commented Nov 25, 2017

This comment has been minimized.

torkleyy commented Nov 25, 2017

This comment has been minimized.

Xaeroxe commented Nov 25, 2017

Xaeroxe changed the title from [WIP] UI text input to UI text input Nov 26, 2017

Xaeroxe added status: ready and removed status: working labels Nov 26, 2017

This comment has been minimized.

Xaeroxe commented Nov 26, 2017

This comment has been minimized.

Xaeroxe commented Nov 26, 2017

Rhuagh reviewed Nov 26, 2017 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.

This comment has been minimized.

This comment has been minimized.

Xaeroxe commented Nov 26, 2017

This comment has been minimized.

Rhuagh commented Nov 26, 2017

Aceeri approved these changes Nov 27, 2017 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.

This comment has been minimized.

torkleyy approved these changes Nov 27, 2017 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.

Xaeroxe commented Nov 8, 2017 •

edited

Edited 4 times

Xaeroxe edited Nov 26, 2017 (most recent)

Xaeroxe edited Nov 26, 2017

Xaeroxe edited Nov 25, 2017

Xaeroxe edited Nov 19, 2017

torkleyy reviewed Nov 11, 2017

View changes

Xaeroxe commented Nov 13, 2017 •

edited

Edited 1 time

Xaeroxe edited Nov 13, 2017 (most recent)

torkleyy reviewed Nov 25, 2017

View changes

Rhuagh reviewed Nov 26, 2017

View changes

Aceeri approved these changes Nov 27, 2017

View changes

torkleyy approved these changes Nov 27, 2017

View changes