Transform System Rewrite/Update #403

Aceeri · Oct 11, 2017

Cleans up the transform system with some new specs features, adds unit tests.

Still plan on adding some tests using quickcheck for some more "fuzzy" testing to catch the odd edge cases.

Fixes #336

Rhuagh · Oct 11, 2017

Rhuagh requested changes Oct 11, 2017

View changes

Rhuagh · Oct 11, 2017

examples/05_assets/main.rs

@@ -13,8 +13,8 @@ use amethyst::config::Config;
 use amethyst::ecs::World;
 use amethyst::ecs::input::InputBundle;
 use amethyst::ecs::rendering::{AmbientColor, Factory, LightComponent, MaterialComponent,
-                               MeshComponent, MeshContext, RenderBundle, TextureComponent,
-                               TextureContext};
+MeshComponent, MeshContext, RenderBundle, TextureComponent,


This looks like broken formatting, are you using the latest rustfmt-nightly? And running cargo +nightly fmt --all ?

I don't really know what went on here either, I ran the latest nightly but I'll try again this afternoon.

Rhuagh · Oct 11, 2017

src/ecs/transform/systems.rs

+            .with(Transform::default())
+            .build();
+
+        dispatcher.dispatch(&mut world.res);


Why not just do RunNow on the system? Feels like unnecessary overhead to use a real Dispatcher.

Was just easier to set up, overhead doesn't really matter that much in these tests anyways.

How is it easier? It's more lines.

Mainly because I know it by memory,.

system.run_now(&mut world.res) ;)

How is this coming along? Any progress? Is it done or ?

Rhuagh · Oct 14, 2017

How is this coming along? Any progress? Is it done or ?

@Rhuagh Got held up for a couple days this week, working on it this weekend.

Aceeri · Oct 14, 2017

@Rhuagh Got held up for a couple days this week, working on it this weekend.

Cool

Rhuagh · Oct 14, 2017

Cool

Been fuzzy testing and I think I've fixed all the issues so far, so we will probably be able to merge this by the end of the week. Going to rebase all my changes later this afternoon.

Aceeri · Oct 17, 2017

Been fuzzy testing and I think I've fixed all the issues so far, so we will probably be able to merge this by the end of the week. Going to rebase all my changes later this afternoon.

Seems @nchashch wants to review this before merge.

torkleyy · Oct 17, 2017

Seems @nchashch wants to review this before merge.

Yep, just updating you all on it.

Aceeri · Oct 17, 2017

Yep, just updating you all on it.

Did you rebase against your local develop our the upstream develop?

Rhuagh · Oct 18, 2017

Did you rebase against your local develop our the upstream develop?

It would probably be easier to rewrite this PR at this point. Please re-open if you disagree with me @Aceeri

Xaeroxe · Oct 18, 2017

It would probably be easier to rewrite this PR at this point. Please re-open if you disagree with me @Aceeri

I disagree, the commits earlier are just an error in git and I just have to drop the commits due to duplicates. Currently fixing the history.

Aceeri · Oct 19, 2017

I disagree, the commits earlier are just an error in git and I just have to drop the commits due to duplicates. Currently fixing the history.

Issue with commits has been fixed.

Aceeri · Oct 19, 2017

Issue with commits has been fixed.

Do you still consider this to be WIP?

Xaeroxe · Oct 19, 2017

Do you still consider this to be WIP?

Not really, it could use some review though, entirely possible I've missed something.

Aceeri · Oct 19, 2017

Not really, it could use some review though, entirely possible I've missed something.

I'll take a look later today.

Rhuagh · Oct 19, 2017

I'll take a look later today.

Rhuagh · Oct 19, 2017

Rhuagh requested changes Oct 19, 2017

View changes

Rhuagh · Oct 19, 2017

amethyst_core/src/transform/systems.rs

@@ -169,17 +185,29 @@ impl<'a> System<'a> for TransformSystem {
             index += 1;
         }
 
-        self.dirty.clear();
+        println!("with parent updated: {:?}", parented);


Debug prints should be removed?

Rhuagh · Oct 19, 2017

amethyst_core/src/transform/systems.rs

         self.dead.clear();
-        self.swapped.clear();
     }
 }
 
 #[cfg(test)]


Yay, tests!

torkleyy · Oct 19, 2017

torkleyy requested changes Oct 19, 2017

View changes

Nice, but I have a few concerns.

torkleyy · Oct 19, 2017

amethyst_core/src/transform/systems.rs

-                    let _ = entities.delete(entity);
-                    self.dead.insert(entity);
+        {
+            for (entity, parent) in (&*entities, parents.open().1).join() {


We need retain!

But isn't this too heavy given that it would be very rare?

It is only running on modified parents, so I don't think it should be that heavy.

I'm not sure yet if FlaggedStorage is user friendly enough tbh.

Yeah, the issue is you'd have to expose the underlying UnprotectedStorage methods onto Storage if you wanted it to be more straightforward.

torkleyy · Oct 19, 2017

amethyst_core/src/transform/systems.rs

+                initted.push(entity.id());
+            }
+
+            println!("Initialized: {:?}", initted);


Another debugging println

torkleyy · Oct 19, 2017

amethyst_core/src/transform/systems.rs

+            for (_entity, local, global, _) in (&*entities, locals_flagged, &mut globals, !&parents).join() {
+                global.0 = local.matrix();
+                noparents.push(_entity.id());
+                #[cfg(debug_assertions)]


Should use debug_assert and global.0.is_finite().

torkleyy · Oct 19, 2017

amethyst_core/src/transform/systems.rs

+        (&mut locals).open().1.clear_flags();
+        (&mut parents).open().1.clear_flags();
+
+        for bit in (&self.frame_init).iter() {


No need for .iter() I think

Our BitSetLikes don't implement IntoIterator, but they probably should so: slide-rs/hibitset#18

torkleyy · Oct 19, 2017

amethyst_core/src/transform/components/parent.rs

+
+use specs::{Component, DenseVecStorage, Entity, FlaggedStorage};
+
+/// Component for defining a parent entity.


This documentation should exactly document on which entity one has to place the component. (It's confusing whether this is a "has" relation or an "is parent of").

Rename it to HasParent maybe ?

Or ChildOf

This was addressed.

torkleyy · Oct 21, 2017

torkleyy reviewed Oct 21, 2017

View changes

torkleyy · Oct 21, 2017

amethyst_core/src/transform/systems.rs

 
                             // Swap took place, re-try this index.
                             continue;
                         }
                     }
 
-                    if local.is_dirty() || child.is_dirty() || self.dirty.contains(&child.parent())
-                    {
+                    // Kill the entity is the parent is dead.


"is" should be if

torkleyy · Oct 21, 2017

amethyst_core/src/transform/systems.rs

-                }
+            for (_entity, local, global, _) in (&*entities, locals_flagged, &mut globals, !&parents).join() {
+                global.0 = local.matrix();
+                debug_assert!(global.0 == global.0, format!("Entity {:?} had NaN transform.", _entity));
             }


Should be is_finite

I don't think [[f32; 4]; 4] has methods implemented by default? f32 has is_finite, but not this.

Ah right. I wonder if this is valid then? I'm not sure if the total equality check catches everything is_finite does

It doesn't check for Infinity, but I'm not sure if it matters if we allow that or not.

If that's the case this check doesn't make too much sense.

Rhuagh · Oct 22, 2017

Rhuagh approved these changes Oct 22, 2017

View changes

torkleyy · Oct 22, 2017

torkleyy approved these changes Oct 22, 2017

View changes

I think this is great!

Xaeroxe · Oct 22, 2017

Xaeroxe requested changes Oct 22, 2017

View changes

#403 (comment)

I'd just like to re-iterate @torkleyy 's expressed concerns. Even if he's approved the PR I'm still requesting these changes be made.

Xaeroxe · Oct 22, 2017

Xaeroxe approved these changes Oct 22, 2017

View changes

LGTM, Thanks!

Also needs some fixes in the GLTF system (it uses Child atm, needs to be changed to Parent).

Rhuagh · Oct 22, 2017

Also needs some fixes in the GLTF system (it uses Child atm, needs to be changed to Parent).

bors r+

Rhuagh · Oct 22, 2017

bors r+

👎 Rejected by label

bors · Oct 22, 2017

👎 Rejected by label

bors r+

Rhuagh · Oct 22, 2017

bors r+

Build succeeded

bors · Oct 22, 2017

Build succeeded

Aceeri added project: main crate status: working type: improvement labels Oct 11, 2017

nchashch self-requested a review Oct 15, 2017

Xaeroxe closed this Oct 18, 2017

Aceeri reopened this Oct 19, 2017

Aceeri changed the title from [WIP] Transform System Rewrite/Update to Transform System Rewrite/Update Oct 19, 2017

Rhuagh added the status: ready label Oct 22, 2017

Rhuagh removed the status: working label Oct 22, 2017

bors bot merged commit 8db08a8 into amethyst:develop Oct 22, 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

amethyst/amethyst

Join GitHub today

Transform System Rewrite/Update #403

Conversation

Aceeri commented Oct 11, 2017 • edited by torkleyy Edited 1 time torkleyy edited Oct 17, 2017 (most recent)

Aceeri added project: main crate status: working type: improvement labels Oct 11, 2017

Rhuagh requested changes Oct 11, 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.

Rhuagh commented Oct 14, 2017

This comment has been minimized.

Aceeri commented Oct 14, 2017

This comment has been minimized.

Rhuagh commented Oct 14, 2017

nchashch self-requested a review Oct 15, 2017

This comment has been minimized.

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

This comment has been minimized.

torkleyy commented Oct 17, 2017

This comment has been minimized.

Aceeri commented Oct 17, 2017

This comment has been minimized.

Rhuagh commented Oct 18, 2017

This comment has been minimized.

Xaeroxe commented Oct 18, 2017

Xaeroxe closed this Oct 18, 2017

This comment has been minimized.

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

Aceeri reopened this Oct 19, 2017

This comment has been minimized.

Aceeri commented Oct 19, 2017

This comment has been minimized.

Xaeroxe commented Oct 19, 2017

This comment has been minimized.

Aceeri commented Oct 19, 2017

Aceeri changed the title from [WIP] Transform System Rewrite/Update to Transform System Rewrite/Update Oct 19, 2017

This comment has been minimized.

Rhuagh commented Oct 19, 2017

Rhuagh requested changes Oct 19, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

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

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

This comment has been minimized.

torkleyy Oct 19, 2017 • edited Edited 1 time torkleyy edited Oct 19, 2017 (most recent)

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

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

Rhuagh approved these changes Oct 22, 2017 View changes

torkleyy approved these changes Oct 22, 2017 View changes

Xaeroxe requested changes Oct 22, 2017 View changes

Xaeroxe approved these changes Oct 22, 2017 View changes

This comment has been minimized.

Rhuagh commented Oct 22, 2017

Aceeri added some commits Sep 11, 2017

This comment has been minimized.

Aceeri commented Oct 11, 2017 •

edited by torkleyy

Edited 1 time

torkleyy edited Oct 17, 2017 (most recent)

Rhuagh requested changes Oct 11, 2017

View changes

Aceeri commented Oct 17, 2017 •

edited

Edited 1 time

Aceeri edited Oct 17, 2017 (most recent)

Aceeri commented Oct 19, 2017 •

edited

Edited 1 time

Aceeri edited Oct 19, 2017 (most recent)

Rhuagh requested changes Oct 19, 2017

View changes

torkleyy requested changes Oct 19, 2017

View changes

Aceeri Oct 20, 2017 •

edited

Edited 1 time

Aceeri edited Oct 20, 2017 (most recent)

torkleyy Oct 19, 2017 •

edited

Edited 1 time

torkleyy edited Oct 19, 2017 (most recent)

torkleyy reviewed Oct 21, 2017

View changes

Rhuagh approved these changes Oct 22, 2017

View changes

torkleyy approved these changes Oct 22, 2017

View changes

Xaeroxe requested changes Oct 22, 2017

View changes

Xaeroxe approved these changes Oct 22, 2017

View changes