Add UI framework #441

Xaeroxe · Oct 22, 2017

Work done in this PR:

Add UiTransform Component
Add UiImage component
Add font loading
Add UiText component
Add UiDraw pass

torkleyy · Oct 22, 2017

torkleyy reviewed Oct 22, 2017

View changes

torkleyy · Oct 22, 2017

amethyst_ui/src/pass.rs

+const FRAG_SRC: &[u8] = include_bytes!("frag.glsl");
+
+#[derive(Copy, Clone)]
+#[allow(dead_code)] // This is used by the shaders


Don't we need C layout then?

Yes, thank you

torkleyy · Oct 22, 2017

torkleyy reviewed Oct 22, 2017

View changes

torkleyy · Oct 22, 2017

amethyst_ui/src/transform.rs

+///
+/// TODO: Eventually this should be either replaced by a citrine type, or citrine may just
+/// populate it.
+pub struct UiTransform {


Maybe we should limit this to the size and take the position from Transform?

I feel it's probably important to keep the concepts separate as this is in pixels, whereas Transform uses unspecified units.

Okay, true. But if we already have that advantage, we should really do

pub enum Dimension { /// In pixels Absolute(f32), /// Multiplied with window width / height Relative(f32), }

Also, I think Bounds would be more accurate.

I'm thinking amethyst_ui might just be glue between citrine and our renderer, this could probably be replaced by just using Position and Bounds components and hooking those up to the amethyst renderer.

@torkleyy I was mostly just trying to sidestep the design decisions we talked about on gitter before. This structure's purpose is to be replaced, it's meant to be slightly sub-optimal.

Example doesn't work yet, probably have to fix my broken shaders.

Xaeroxe · Oct 22, 2017

Example doesn't work yet, probably have to fix my broken shaders.

torkleyy · Oct 23, 2017

torkleyy reviewed Oct 23, 2017

View changes

torkleyy · Oct 23, 2017

amethyst_renderer/src/pass/pbm/interleaved.rs

-            light,
-        ): (
-            Option<Fetch<'a, Camera>>,
+        (active, camera, ambient, mesh_storage, tex_storage, material_defaults,


I'm not sure about this formatting, but well

That formatting was merged in from @Rhuagh 's PR, but I still have more code to write here so I'll give it a rustfmt pass again.

That's weird. It shouldn't do that

So what's the current status of this PR @Xaeroxe ?

torkleyy · Oct 25, 2017

So what's the current status of this PR @Xaeroxe ?

Still requires additional work, I won't have time to work on it for a few days though.

Xaeroxe · Oct 25, 2017

Still requires additional work, I won't have time to work on it for a few days though.

(If anyone wants to push this to completion I gladly accept PRs on my PR)

Xaeroxe · Oct 25, 2017

(If anyone wants to push this to completion I gladly accept PRs on my PR)

Z-ordering tested and works 👍
Had to add resizing to blend targets to fix resize issue.

I'm now starting work on a text component.

Xaeroxe · Oct 28, 2017

Z-ordering tested and works 👍
Had to add resizing to blend targets to fix resize issue.

I'm now starting work on a text component.

Text component is ready. This is still super rough around the edges, font loading in particular is kind of hairy, but it does indeed work!

Xaeroxe · Oct 29, 2017

Text component is ready. This is still super rough around the edges, font loading in particular is kind of hairy, but it does indeed work!

Pong now displays score :D Font loading has also been drastically simplified.

Xaeroxe · Oct 29, 2017

Pong now displays score :D Font loading has also been drastically simplified.

Broken by upstream changes, fixing now.

Xaeroxe · Oct 29, 2017

Broken by upstream changes, fixing now.

So while there's a lot of things I want to add here and there's many improvements to be made, this is useful in its current form, so I'm going to remove the WIP. We can merge this as is, and I'll work on adding the features I want in subsequent PRs.

Xaeroxe · Oct 29, 2017

So while there's a lot of things I want to add here and there's many improvements to be made, this is useful in its current form, so I'm going to remove the WIP. We can merge this as is, and I'll work on adding the features I want in subsequent PRs.

Since we've determined we don't want to fix pass initialization until the gfx-hal rewrite is complete

r? @Rhuagh @torkleyy @Aceeri

Xaeroxe · Oct 30, 2017

Since we've determined we don't want to fix pass initialization until the gfx-hal rewrite is complete

r? @Rhuagh @torkleyy @Aceeri

@Xaeroxe Do we want to keep the placeholder position component for now and just integrate the citrine Position one later?

Aceeri · Oct 30, 2017

@Xaeroxe Do we want to keep the placeholder position component for now and just integrate the citrine Position one later?

torkleyy · Oct 30, 2017

torkleyy reviewed Oct 30, 2017

View changes

torkleyy · Oct 30, 2017

amethyst_ui/src/bundle.rs

+
+/// UI bundle
+///
+/// Will register all necessary components and systems needed for UI, along with any resources.


We should really list the stuff here. (I know this hasn't been done in the past)

torkleyy · Oct 30, 2017

amethyst_ui/src/pass.rs

+use specs::{Entities, Entity, Fetch, Join, ParJoin, ReadStorage};
+
+use super::*;
+use amethyst_renderer::{Mesh, MeshHandle};


Could we move all the amethyst_renderer imports to the top?

I'll give this a full review tomorrow.

torkleyy · Oct 30, 2017

I'll give this a full review tomorrow.

@Aceeri Probably yes, since citrine has a little ways to go before integrating into here and this is better than the nothing we have in Amethyst right now.

Xaeroxe · Oct 30, 2017

@Aceeri Probably yes, since citrine has a little ways to go before integrating into here and this is better than the nothing we have in Amethyst right now.

Aceeri · Oct 31, 2017

Aceeri approved these changes Oct 31, 2017

View changes

Awesome stuff! Now I just need to get off my ass and start working on the layout stuff again.

Aceeri · Oct 31, 2017

amethyst_renderer/src/system.rs

@@ -164,6 +163,7 @@ where
 {
     fn run_now(&mut self, res: &'a Resources) {
         self.do_asset_loading(AssetLoadingData::<'a>::fetch(res, 0));
+        self.do_window_management(WindowData::<'a>::fetch(res, 0));
         self.do_render(RenderData::<'a, P>::fetch(res, 0));


Minor nitpicking but these methods should probably just be render and manage_window or something. do_... seems redundant given they are methods.

I was just following the convention set from before, but sure I can do that.

torkleyy · Oct 31, 2017

torkleyy requested changes Oct 31, 2017

View changes

Great work @Xaeroxe ! I looked into it very closely, which is why you'll have 26 comments to address now 😬

torkleyy · Oct 31, 2017

amethyst_ui/src/text.rs

+    type Storage = DenseVecStorage<Self>;
+}
+
+/// This system renders `UiTex`t.


pahaha thanks

torkleyy · Oct 31, 2017

amethyst_ui/src/transform.rs

+    ///
+    /// Do not try and replace this while the inner function  is being called.  Whatever you put
+    /// here would be overwritten.
+    pub resize_fn: Option<Box<FnMut(&mut UiTransform, (f32, f32)) + Send + Sync>>,


It might be worth considering to move this into a separate component. Additionally, maybe we can make it an enum which includes standard strategies? Except @Aceeri provides some citrine stuff handling it, that is.

That's not a bad idea @torkleyy , thanks!

torkleyy · Oct 31, 2017

examples/04_pong/pong.rs

+        resize_fn: None,
+    };
+    let p2_size_fn = |transform: &mut UiTransform, (width, _height)| {
+        transform.x = (width / 2.) + 100. - transform.width / 2.;


Yeah, this isn't very nice. I guess some sort of basic layouting would be nice; not suggesting to do it in this PR though.

Personally I like how flexible this approach is and how easy it makes prototyping but this particular implementation did end up kind of hairy.

torkleyy · Oct 31, 2017

examples/04_pong/systems/winner.rs

@@ -34,10 +37,20 @@ impl<'s> System<'s> for WinnerSystem {
             let did_hit = if ball_x <= ball.radius {
                 // Right player scored on the left side.
                 score_board.score_right += 1;
+                for (transform, text) in (&ui_transform, &mut text).join() {
+                    if "P2" == transform.id {


Entities should be used for this. While having a name for entities (which I btw. planned to implement in Specs: you would be able to just do world.create_entity().name("Blah") then) is nice, I don't think we should recommend it for component identification purposes.

I guess I like the getElementById from JavaScript and I was trying to mimic that here. It's nice being able to just keep things in your head and request for static values rather than having to store the entity, but storing the entity is also more efficient, so I'll go with that.

torkleyy · Oct 31, 2017

examples/09_ui/main.rs

+
+const SPHERE_COLOUR: [f32; 4] = [0.0, 0.0, 1.0, 1.0]; // blue
+const AMBIENT_LIGHT_COLOUR: Rgba = Rgba(0.01, 0.01, 0.01, 1.0); // near-black
+const POINT_LIGHT_COLOUR: Rgba = Rgba(1.0, 1.0, 1.0, 1.0); // white


Just a note, I think the Rgba structure is redundant.

This was copy pasted from the sphere example, should I change it there too?

No, it's not related to this PR.

torkleyy · Oct 31, 2017

amethyst_ui/src/pass.rs

+        let proj_vec = vec2(
+            2. / screen_dimensions.width(),
+            -2. / screen_dimensions.height(),
+        ).extend(-2.)


This is confusing to me.

Yeah I'm not sure using extend here was the best idea, so I'll fix this.

Move it to the shader imho, (the extend part)

torkleyy · Oct 31, 2017

amethyst_ui/src/shaders/vertex.glsl

+    vertex.position *= vec4(dimension, 1, 1);
+    vertex.position += vec4(coord, 0, 0);
+    vertex.position *= proj_vec;
+    vertex.position += vec4(-1, 1, 0, 0);


Why aren't you just using a matrix?

This is more efficient, less multiplying by 1 and adding 0.

I'm not sure if we can measure GPU performance in terms of how often a multiplication is happening, but rather how typical and thus optimized it is for a certain situation. So I don't know what's more efficient here, it's just unconventional.

Vec4 operations are gonna be faster than s matrix for the most part

So I did this because @omni-viral and @Rhuagh told me the projection matrix was excessive.

You can even reduce it further using vec2 and swizzle operators if you want.

torkleyy · Oct 31, 2017

amethyst_ui/src/pass.rs

+                    if let Some(mesh) = mesh_storage.get(unit_mesh) {
+                        let vbuf = match mesh.buffer(PosTex::ATTRIBUTES) {
+                            Some(vbuf) => vbuf.clone(),
+                            None => continue,


If there's no unit_mesh, we can just return.

The same applies for the check above, let's return and decrease the indention.

torkleyy · Oct 31, 2017

amethyst_ui/src/text.rs

+        &mut self.text
+    }
+
+    /// The RGBA color with a maximum of 1.0 and a minimum of 0.0 for each channel of the text.


Thanks for the clarification!

torkleyy · Oct 31, 2017

amethyst_ui/src/text.rs

+    );
+
+    fn run(&mut self, (transform, mut text, loader, tex_storage, font_storage): Self::SystemData) {
+        for (transform, text) in (&transform, &mut text).join() {


I'm not sure if this is a more general problem, but if a text doesn't have a transform, it will be silently ignored. No need to fix it here, just wanted to note.

Please filter and flat_map this, otherwise the indention is too much.

r? @torkleyy

Xaeroxe · Oct 31, 2017

r? @torkleyy

Rhuagh · Oct 31, 2017

Rhuagh reviewed Oct 31, 2017

View changes

Rhuagh · Oct 31, 2017

amethyst_ui/src/pass.rs

+        // Populate and update the draw order cache.
+        // TODO: Replace all of this with code taking advantage of specs::TrackedStorage.
+        // TrackedStorage doesn't exist yet but it will in a later version of specs.
+        cached_draw_order.retain(|&(_z, entity)| {


Is it really worth it to cache this data and not just recalculate it per frame ? The loop below is best case n^2, because position will potentially go through all items in the cache, plus a sort that's n*log(n). Wouldn't it be faster to use binary_search_by and do insertion sort and create the list from scratch every frame? That would be a guaranteed n*log(n).

@Rhuagh Yeah you're right this eventually turned out to be not quite what was desired. I have a couple ideas on how to fix it though. First is to introduce a private AtomicBool to the UiTransform indicating if it was added to the cache or not, taking our n^2 operation down to just n.

Caching the sorting is valuable imo because sorting an already sorted collection is very cheap.

I agree, it's just when the cache update is more expensive than rebuilding the whole thing I get suspicious :P TrackedStorage and insertion sort on z would turn it into nlogn worst case

r? @Rhuagh

I've tightened up the caching a lot, it uses bitset to be much more efficient now.

Xaeroxe · Oct 31, 2017

r? @Rhuagh

I've tightened up the caching a lot, it uses bitset to be much more efficient now.

Rhuagh · Oct 31, 2017

Rhuagh approved these changes Oct 31, 2017

View changes

Much better, thanks

Rhuagh · Oct 31, 2017

Much better, thanks

@torkleyy After adding the new and improved cache I've determined TrackedStorage would actually be a detriment here instead of an advantage as the new cache is actually faster, (mostly because it takes advantage of things we can only do with this particular setup)

Xaeroxe · Nov 1, 2017

@torkleyy After adding the new and improved cache I've determined TrackedStorage would actually be a detriment here instead of an advantage as the new cache is actually faster, (mostly because it takes advantage of things we can only do with this particular setup)

torkleyy · Nov 1, 2017

You are cloning ui_transform's bitset 3 times, I think you could reduce that to 1.

torkleyy · Nov 1, 2017

What I don't like about FlaggedStorage is that components get flagged on iteration, not on change. That's my main motivation for TrackedStorage.

torkleyy · Nov 1, 2017

torkleyy approved these changes Nov 1, 2017

View changes

I commented on the commit which changes the caching, otherwise I think it's great.

Since bitset cloning is pretty cheap I'm going to

bors r+

Xaeroxe · Nov 1, 2017

Since bitset cloning is pretty cheap I'm going to

bors r+

bors r-

Xaeroxe · Nov 1, 2017

bors r-

Canceled

bors · Nov 1, 2017

Canceled

@torkleyy has informed me that bit set clones are not cheap. I'll fix this in the morning

Xaeroxe · Nov 1, 2017

@torkleyy has informed me that bit set clones are not cheap. I'll fix this in the morning

bors r+

Xaeroxe · Nov 1, 2017

bors r+

Build succeeded

bors · Nov 1, 2017

Build succeeded

The ui example shows me a dark blue sphere with nothing else. Closing the window causes a panic "InvalidFramebufferOperation".

The pong example doesn't display any text either.

remram44 · Nov 1, 2017

The ui example shows me a dark blue sphere with nothing else. Closing the window causes a panic "InvalidFramebufferOperation".

The pong example doesn't display any text either.

Oh right, the blend targets still don't work on Mac. I forgot about that. I'll fix it before we release. That's not a problem with anything UI related, it's a problem with using a rendering pass with blending, interestingly this only occurs on OSX.

Xaeroxe · Nov 1, 2017

Oh right, the blend targets still don't work on Mac. I forgot about that. I'll fix it before we release. That's not a problem with anything UI related, it's a problem with using a rendering pass with blending, interestingly this only occurs on OSX.

I wrote up #473 to address this.

Xaeroxe · Nov 1, 2017

I wrote up #473 to address this.

Xaeroxe added diff: medium project: rendering status: working type: feature and removed diff: medium labels Oct 22, 2017

Xaeroxe added some commits Oct 28, 2017

Merge upstream and do some UI text work

Loading status checks…

59a568e

Add text component

Loading status checks…

c31452e

Improve font loading and add score to pong

Loading status checks…

3402fb3

Merge branch 'develop' of github.com:amethyst/amethyst into ui

ba51a89

Fix to use upstream APIs

Loading status checks…

5ab1db5

Xaeroxe changed the title from [WIP] UI to Add UI framework Oct 29, 2017

Xaeroxe added status: ready and removed status: working labels Oct 29, 2017

Xaeroxe added some commits Oct 29, 2017

Use functional syntax

Loading status checks…

949c327

Fix comment

Loading status checks…

f4c08ae

Normalize strings to make accent characters happy

Loading status checks…

d6aceb5

Remove do_ prefix from RenderSystem

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits

Loading status checks…

bd6e7f2

torkleyy self-requested a review Oct 31, 2017

Add TODO

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits

Loading status checks…

4d8ebbb

Address review comments

Loading status checks…

537fffd

Improve caching

Loading status checks…

b7aa2a3

Remove TrackedStorage todo, as it won't help here anymore

Loading status checks…

949a449

bors bot added a commit that referenced this pull request Nov 1, 2017

Merge #441
441: Add UI framework r=Xaeroxe a=Xaeroxe Work done in this PR: - Add UiTransform Component - Add UiImage component - Add font loading - Add UiText component - Add UiDraw pass

Loading status checks…

99ae8cf

bors bot added a commit that referenced this pull request Nov 1, 2017

Merge #441
441: Add UI framework r=Xaeroxe a=Xaeroxe Work done in this PR: - Add UiTransform Component - Add UiImage component - Add font loading - Add UiText component - Add UiDraw pass

Loading status checks…

3299504

Reduce BitSet cloning

Loading status checks…

b8d0103

bors bot merged commit b8d0103 into amethyst:develop Nov 1, 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 branch Nov 1, 2017

remram44 referenced this pull request Nov 1, 2017
Closed
UI Framework #365
4 of 4 tasks complete

amethyst/amethyst

Join GitHub today

Add UI framework #441

Conversation

Xaeroxe commented Oct 22, 2017 • edited Edited 5 times Xaeroxe edited Oct 29, 2017 (most recent) Xaeroxe edited Oct 29, 2017 Xaeroxe edited Oct 29, 2017 Xaeroxe edited Oct 29, 2017 Xaeroxe edited Oct 22, 2017

Xaeroxe added diff: medium project: rendering status: working type: feature and removed diff: medium labels Oct 22, 2017

torkleyy reviewed Oct 22, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

torkleyy reviewed Oct 22, 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.

Aceeri Oct 22, 2017 • edited Edited 1 time Aceeri edited Oct 22, 2017 (most recent)

This comment has been minimized.

This comment has been minimized.

Xaeroxe commented Oct 22, 2017

torkleyy reviewed Oct 23, 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 Oct 25, 2017

This comment has been minimized.

Xaeroxe commented Oct 25, 2017

This comment has been minimized.

Xaeroxe commented Oct 25, 2017 • edited Edited 1 time Xaeroxe edited Oct 25, 2017 (most recent)

Xaeroxe and others added some commits Oct 21, 2017

This comment has been minimized.

Xaeroxe commented Oct 28, 2017

Xaeroxe added some commits Oct 28, 2017

This comment has been minimized.

Xaeroxe commented Oct 29, 2017

This comment has been minimized.

Xaeroxe commented Oct 29, 2017

This comment has been minimized.

Xaeroxe commented Oct 29, 2017

This comment has been minimized.

Xaeroxe commented Oct 29, 2017

Xaeroxe changed the title from [WIP] UI to Add UI framework Oct 29, 2017

Xaeroxe added status: ready and removed status: working labels Oct 29, 2017

Xaeroxe added some commits Oct 29, 2017

Xaeroxe added some commits Oct 29, 2017

This comment has been minimized.

Xaeroxe commented Oct 30, 2017 • edited Edited 1 time Xaeroxe edited Oct 30, 2017 (most recent)

This comment has been minimized.

Aceeri commented Oct 30, 2017

torkleyy reviewed Oct 30, 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 Oct 30, 2017

This comment has been minimized.

Xaeroxe commented Oct 30, 2017

Aceeri approved these changes Oct 31, 2017 View changes

This comment has been minimized.

This comment has been minimized.

torkleyy self-requested a review Oct 31, 2017

torkleyy requested changes Oct 31, 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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Xaeroxe commented Oct 22, 2017 •

edited

Edited 5 times

Xaeroxe edited Oct 29, 2017 (most recent)

Xaeroxe edited Oct 29, 2017

Xaeroxe edited Oct 29, 2017

Xaeroxe edited Oct 29, 2017

Xaeroxe edited Oct 22, 2017

torkleyy reviewed Oct 22, 2017

View changes

torkleyy reviewed Oct 22, 2017

View changes

Aceeri Oct 22, 2017 •

edited

Edited 1 time

Aceeri edited Oct 22, 2017 (most recent)

torkleyy reviewed Oct 23, 2017

View changes

Xaeroxe commented Oct 25, 2017 •

edited

Edited 1 time

Xaeroxe edited Oct 25, 2017 (most recent)

Xaeroxe commented Oct 30, 2017 •

edited

Edited 1 time

Xaeroxe edited Oct 30, 2017 (most recent)

torkleyy reviewed Oct 30, 2017

View changes

Aceeri approved these changes Oct 31, 2017

View changes

torkleyy requested changes Oct 31, 2017

View changes