Add ECS bundle concept #364

Rhuagh · Sep 17, 2017

Opening this early to get feedback. Is this a viable concept or should I trash it immediately?

@Rhuagh Basically they are code wrappers to make the code less heavy and reduce the boiler plate?
I'd like to know how that handles conflicts between dependencies and component registration too.

jojolepro · Sep 17, 2017

@Rhuagh Basically they are code wrappers to make the code less heavy and reduce the boiler plate?
I'd like to know how that handles conflicts between dependencies and component registration too.

Well, they're a generification of with_renderer in the ApplicationBuilder, so instead of adding functions there for every single concept (with_audio, with_transform), we can add bundles instead.

Conflicts are handled by specs for the most part. Resources are overridden, component registration is essentially a NOP for any duplicate registration. System registration is a panic if the name given already exists. When it comes to dependencies and system names, each bundle handles that on it's own. Look at the TransformBundle for example, it has support for dependencies.

Rhuagh · Sep 18, 2017

Well, they're a generification of with_renderer in the ApplicationBuilder, so instead of adding functions there for every single concept (with_audio, with_transform), we can add bundles instead.

Conflicts are handled by specs for the most part. Resources are overridden, component registration is essentially a NOP for any duplicate registration. System registration is a panic if the name given already exists. When it comes to dependencies and system names, each bundle handles that on it's own. Look at the TransformBundle for example, it has support for dependencies.

An alternative would be to have the build() function take the ApplicationBuilder as argument and return it wrapped in a Result.

Rhuagh · Sep 18, 2017

An alternative would be to have the build() function take the ApplicationBuilder as argument and return it wrapped in a Result.

omni-viral · Sep 18, 2017

omni-viral reviewed Sep 18, 2017

View changes

omni-viral · Sep 18, 2017

src/app.rs

+    ///
+    pub fn with_bundle<B>(mut self, bundle: B) -> Result<Self>
+    where
+        B: for<'c> ECSBundle<'a, 'b, (&'c EventsLoop)>,


RenderSystem is special as it the only one that need access to EventsLoop during building.
What should we do if there will be new bundle that will require some other data from Application to build?

Yeah, it's not perfect, but I haven't figured out a good solution to that yet.

Updated, the bundle trait now works on the ApplicationBuilder directly.

omni-viral · Sep 18, 2017

src/app.rs

+    ///
+    pub fn with_bundle<B>(mut self, bundle: B) -> Result<Self>
+    where
+        B: for<'c> ECSBundle<'a, 'b, (&'c EventsLoop)>,


Parentheses are unnecessary. If you want to make a tuple with single element you need to make it like this (A,)

Refactored to use ApplicationBuilder directly.

Rhuagh · Sep 18, 2017

Refactored to use ApplicationBuilder directly.

omni-viral · Sep 18, 2017

omni-viral reviewed Sep 18, 2017

View changes

omni-viral · Sep 18, 2017

src/ecs/mod.rs

-    where
-        Self: Sized;
+/// Describes a bundle of ECS components, resources and systems
+pub trait ECSBundle<'a, 'b, T: ::state::State + 'a> {


This bound shouldn't be here. Please, remove it from ApplicationBuilder's functions where T: State is not required so that it's not enforced everywhere. I believe the only function that requires T: State is ApplicationBuilder::build

Well, ApplicationBuilder requires it at the moment, and that's a separate change from this PR imho.

I fixed it.

omni-viral · Sep 18, 2017

omni-viral approved these changes Sep 18, 2017

View changes

I like it

Looks good so far! What would you consider the remaining tasks on this PR?

Xaeroxe · Sep 18, 2017

Looks good so far! What would you consider the remaining tasks on this PR?

A bit of cleanup, and making a bundle for the pong stuff. And more documentation

Rhuagh · Sep 18, 2017

A bit of cleanup, and making a bundle for the pong stuff. And more documentation

Ready to go after rereview.

Rhuagh · Sep 18, 2017

Ready to go after rereview.

Xaeroxe · Sep 18, 2017

Xaeroxe approved these changes Sep 18, 2017

View changes

bors r+

Thanks @Rhuagh !

Timed out

bors · Sep 19, 2017

Timed out

Override.

Xaeroxe · Sep 19, 2017

Override.

torkleyy · Sep 19, 2017

torkleyy reviewed Sep 19, 2017

View changes

torkleyy · Sep 19, 2017

src/app.rs

-                .register_texture_asset()
-                .register_material_not_yet_asset(),
-        )
+    pub fn with_bundle<B>(self, bundle: B) -> Result<Self>


If a bundle offers multiple systems, how would one manage dependencies? Should maybe the documentation list the system names it adds?

I'd put all the information that the user could need in the documentation for each bundle.

torkleyy · Sep 19, 2017

src/ecs/audio/bundle.rs

+            ResourceId::new::<Option<Output>>(),
+        )
+        {
+            builder = builder.with_resource(default_output());


We should really have an entry API here.

torkleyy · Sep 19, 2017

src/ecs/audio/bundle.rs

+            builder = builder.with_resource(default_output());
+        }
+
+        let dj = match *builder.world.read_resource::<Option<Output>>() {


Should use map.

Not sure I follow.

Got it now.

torkleyy · Sep 19, 2017

src/ecs/audio/bundle.rs

+        };
+
+        if let Some(dj) = dj {
+            builder = builder.with_resource(dj).with(


That's where we need add_*.

torkleyy · Sep 19, 2017

src/ecs/input/bundle.rs

+
+/// Bundle for adding the InputHandler and input bindings
+///
+/// # Errors


Should be ## Errors

I just followed what @csherratt has done with the ApplicationBuilder etc.

torkleyy · Sep 19, 2017

src/ecs/rendering/mod.rs

@@ -1,9 +1,11 @@
 //! `amethyst` rendering ecs module
 
+pub use self::bundle::RenderBundle;


I think we should decide on whether we want to have a public module or reexported items. Given these bundles all have the same postfix, a conflict is impossible. I just see this inconsistency existed before the PR 😟

torkleyy · Sep 19, 2017

src/ecs/transform/bundle.rs

+/// Will register transform components, and the TransformSystem.
+/// TransformSystem will be registered with name "transform_system".
+///
+/// # Errors


Same as above

torkleyy · Sep 19, 2017

src/ecs/transform/bundle.rs

+///
+/// No errors will be returned by this bundle.
+///
+/// # Panics


Should be ## Panics

torkleyy · Sep 19, 2017

src/ecs/transform/bundle.rs

+///
+/// # Panics
+///
+/// Panics in TransformSystem registration if the bundle is applied twice in the same dispatcher.


TransformSystem

That's what he wrote?

Yep, just without backticks.

torkleyy · Sep 19, 2017

src/ecs/transform/bundle.rs

+    }
+
+    /// Set dependencies for the TransformSystem
+    pub fn with_dep(mut self, dep: &'a [&'a str]) -> Self {


This is something that makes the new bundle system more complex and gives the user less control. Also see my comment above. But well, let's see how it works out.

You are not forced to use the bundle system, you can do all it does on your own, and the code is open so you can see what goes on inside.
I feel this is a way of lowering the threshold to start using the engine, without giving anything up really.

Rhuagh added some commits Sep 17, 2017

Add ECS bundle concept

Loading status checks…

e9fabd8

Adding bundles for Audio and Transform, and updating examples to use …
…the bundles.

Loading status checks…

2675af4

Removing SystemExt

Loading status checks…

5234c66

Changed ECSBundle to use ApplicationBuilder directly

Loading status checks…

9be1f3a

Moving trait bound to the only place it is used.

Loading status checks…

e656388

omni-viral self-assigned this Sep 18, 2017

Adding a bundle for Input. Adding a bundle to the Pong example. Addin…
…g docs

Loading status checks…

6021f63

Rhuagh changed the title from [WIP] Add ECS bundle concept to Add ECS bundle concept Sep 18, 2017

bors bot added a commit that referenced this pull request Sep 18, 2017

Merge #364
364: Add ECS bundle concept r=Xaeroxe a=Rhuagh Opening this early to get feedback. Is this a viable concept or should I trash it immediately?

Loading status checks…

8c46a57

Xaeroxe merged commit 60107f5 into amethyst:develop Sep 19, 2017
1 of 3 checks passed

1 of 3 checks passed

bors Timed out

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

Rhuagh deleted the Rhuagh:bundles branch Sep 19, 2017

amethyst/amethyst

Join GitHub today

Add ECS bundle concept #364

Conversation

Rhuagh commented Sep 17, 2017

Rhuagh added some commits Sep 17, 2017

This comment has been minimized.

jojolepro commented Sep 17, 2017

This comment has been minimized.

Rhuagh commented Sep 18, 2017 • edited Edited 1 time Rhuagh edited Sep 18, 2017 (most recent)

This comment has been minimized.

Rhuagh commented Sep 18, 2017

omni-viral reviewed Sep 18, 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.

Rhuagh commented Sep 18, 2017

omni-viral reviewed Sep 18, 2017 View changes

This comment has been minimized.

omni-viral Sep 18, 2017 • edited Edited 1 time omni-viral edited Sep 18, 2017 (most recent)

This comment has been minimized.

This comment has been minimized.

omni-viral self-assigned this Sep 18, 2017

omni-viral approved these changes Sep 18, 2017 View changes

This comment has been minimized.

Xaeroxe commented Sep 18, 2017

This comment has been minimized.

Rhuagh commented Sep 18, 2017

Rhuagh changed the title from [WIP] Add ECS bundle concept to Add ECS bundle concept Sep 18, 2017

This comment has been minimized.

Rhuagh commented Sep 18, 2017

Xaeroxe approved these changes Sep 18, 2017 View changes

bors bot added a commit that referenced this pull request Sep 18, 2017

This comment has been minimized.

bors bot Sep 19, 2017

Timed out

bors bot commented Sep 19, 2017

Timed out

This comment has been minimized.

Xaeroxe commented Sep 19, 2017

Hide details View details Xaeroxe merged commit 60107f5 into amethyst:develop Sep 19, 2017 1 of 3 checks passed

1 of 3 checks passed

Rhuagh deleted the Rhuagh:bundles branch Sep 19, 2017

torkleyy reviewed Sep 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.

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.

Rhuagh Sep 19, 2017 • edited Edited 1 time Rhuagh edited Sep 19, 2017 (most recent)

Aceeri added a commit to Aceeri/amethyst that referenced this pull request Oct 18, 2017

Rhuagh commented Sep 18, 2017 •

edited

Edited 1 time

Rhuagh edited Sep 18, 2017 (most recent)

omni-viral reviewed Sep 18, 2017

View changes

omni-viral reviewed Sep 18, 2017

View changes

omni-viral Sep 18, 2017 •

edited

Edited 1 time

omni-viral edited Sep 18, 2017 (most recent)

omni-viral approved these changes Sep 18, 2017

View changes

Xaeroxe approved these changes Sep 18, 2017

View changes

Xaeroxe merged commit `60107f5` into amethyst:develop Sep 19, 2017
1 of 3 checks passed

torkleyy reviewed Sep 19, 2017

View changes

Rhuagh Sep 19, 2017 •

edited

Edited 1 time

Rhuagh edited Sep 19, 2017 (most recent)