Convert to specs v0.9 #250

zakarumych · 2017-06-30T09:33:33Z

Convert to specs v0.9.1

torkleyy

Great, thanks for the PR! I added some comments, and I'd ask you to also do some formatting and import reordering.

torkleyy · 2017-06-30T09:54:53Z

examples/03_renderable/main.rs

@@ -7,7 +7,7 @@ extern crate cgmath;
 use amethyst::{Application, ElementState, Event, State, Trans, VirtualKeyCode, WindowEvent};
 use amethyst::asset_manager::{AssetManager, DirectoryStore};
 use amethyst::project::Config;
-use amethyst::ecs::{Join, System, RunArg, World};
+use amethyst::ecs::{Join, System, World, WriteStorage, Fetch, FetchMut};


Please order imports alphabetically.

torkleyy · 2017-06-30T09:56:23Z

examples/04_pong/main.rs


+    type SystemData = (WriteStorage<'a, Ball>, WriteStorage<'a, Plank>, WriteStorage<'a, LocalTransform>, Fetch<'a, Camera>, Fetch<'a, Time>, Fetch<'a, InputHandler>, FetchMut<'a, Score>);


Would you mind adding a line break here?

torkleyy · 2017-06-30T09:56:59Z

src/asset_manager/asset_manager.rs

 use wavefront_obj::obj::{ObjSet, parse, Primitive};

-use ecs::{Allocator, Component, Entity, GatedStorage, MaskedStorage, Storage, VecStorage, World};
+use ecs::{Component, Entity, VecStorage, World, ReadStorage};


Same here, please move ReadStorage before VecStorage.

torkleyy · 2017-06-30T10:02:36Z

src/ecs/resources/broadcaster.rs

-//! fn main() {
-//!     use amethyst::ecs::Gate;
-//!
+//! fn main() {//!


I don't think the //! is necessary here.

torkleyy · 2017-06-30T10:03:07Z

src/ecs/resources/broadcaster.rs

-use ticketed_lock::ReadTicket;
-
-use ecs::{World, Component, EntityBuilder, GatedStorage, Allocator, MaskedStorage, Join};
+use ecs::{World, Component, EntityBuilder, Join, ReadStorage};


We didn't have it before, but it would be nice to sort these imports, too.

torkleyy · 2017-06-30T10:05:13Z

src/ecs/resources/broadcaster.rs

+            self.world.entities().join().collect::<Vec<_>>()
+        };
+        for entity in entities {
+            self.world.delete_entity(entity);
        }
        self.world.maintain();


I don't think this call is necessary, dunno why it is there.

I think I should move it to the end of Application::advance_frame

torkleyy · 2017-06-30T10:07:40Z

src/engine/app.rs

@@ -64,10 +65,11 @@ impl Application {
        assets.add_loader::<gfx_types::Factory>(factory);

        let trans_sys = TransformSystem::new();
-        planner.add_system::<TransformSystem>(trans_sys, "transform_system", 0);
+
+        let dispatcher_builder = dispatcher_builder.add_barrier();


As discussed on Gitter, it would be preferable to give the responsibility of adding "default" systems to the user.

torkleyy · 2017-06-30T10:09:10Z

src/engine/app.rs

-    pub fn new(initial_state: T, cfg: DisplayConfig) -> ApplicationBuilder<T> {
-        let pool = Arc::new(ThreadPool::new(num_cpus::get()));
+    pub fn new(initial_state: T, cfg: DisplayConfig) -> ApplicationBuilder<'a, T> {
+        let pool = Arc::new(ThreadPool::new( ::rayon::Configuration::new().num_threads(num_cpus::get())).unwrap());


Could you format the code in some way? Seems there is unnecessary whitespace. And please add an import statement inside the function.

Additionally, consider using expect over unwrap.

torkleyy · 2017-06-30T10:10:36Z

src/engine/app.rs

    {
-        self.planner.add_system::<S>(sys, name, pri);
+        self.dispatcher_builder = self.dispatcher_builder.add(sys, name, dep);


Not sure if we should just wrap the functionality. Could we just let the user pass in the dispatcher with all systems already added?

Same way we could just let the user to pass world with all Components added.
But there is interesting feature in @ebkalderons fork. SystemExtwhich can be used to add all related components. We could use it inApplicationBuilder`'s wrapping functions

Why not integrate it into Specs? I didn't know about that, but it would be helpful, yeah.

Oh well, it seems it isn't 100% compatible with our existing builders...

I think there should be two possibilities for user now
Use ApplicationBuilder and add Systems and register Components through it.
Create Application instance directly passing Dispatcher with all Systems and World with all Components added.

torkleyy

Thanks for addressing my comments! I added some more comments.

torkleyy · 2017-06-30T13:55:57Z

examples/04_pong/main.rs

@@ -3,8 +3,10 @@ extern crate amethyst;
 use amethyst::{Application, Event, State, Trans, VirtualKeyCode, WindowEvent};
 use amethyst::asset_manager::AssetManager;
 use amethyst::project::Config;
-use amethyst::ecs::{World, Join, VecStorage, Component, RunArg, System};
+use amethyst::ecs::{World, Join, VecStorage, Component, System, WriteStorage, Fetch, FetchMut};


Needs reordering.

torkleyy · 2017-06-30T13:57:21Z

examples/04_pong/main.rs

                        if plank.position + plank.dimensions[1] / 2. < 1. {
                            plank.position += plank.velocity * delta_time;
                        }
                    }
                    // If `S` is pressed and plank is in screen boundaries then move down
-                    if input.key_down(VirtualKeyCode::S) {
+                    if input.key_is_pressed(VirtualKeyCode::S) {


I think we need a rebase here.

torkleyy · 2017-06-30T13:59:49Z

src/engine/app.rs

-    pub fn with<S>(mut self, sys: S, name: &str, pri: Priority) -> ApplicationBuilder<T>
-        where S: System<()> + 'static
+    /// Adds a given thread-local system `sys`
+    /// All thread-local systems executed sequentially after all non-thread-local systems


Maybe add an "are" in here?

torkleyy · 2017-06-30T14:01:20Z

src/lib.rs

@@ -84,7 +84,7 @@ extern crate genmesh;
 extern crate imagefmt;
 extern crate num_cpus;
 extern crate specs;
-extern crate threadpool;
+extern crate rayon;


Sorry, have to nitpick here again - please move the rayon line up by one.

torkleyy · 2017-06-30T14:01:57Z

src/ecs/systems/transform.rs

@@ -3,7 +3,7 @@
 use cgmath::Matrix4;
 use fnv::{FnvHashMap as HashMap, FnvHashSet as HashSet};

-use ecs::{Join, Entity, RunArg, System};
+use ecs::{Entity, Join, WriteStorage, ReadStorage, Entities, System};


Needs reordering.

torkleyy · 2017-06-30T14:02:47Z

examples/05_assets/main.rs

@@ -214,6 +213,8 @@ fn main() {
    let path = format!("{}/examples/05_assets/resources/config.yml",
                       env!("CARGO_MANIFEST_DIR"));
    let cfg = DisplayConfig::load(path);
-    let mut game = Application::build(Example, cfg).done();
+    let mut game = Application::build(Example, cfg)
+        .with_thread_local::<TransformSystem>(TransformSystem::new())


Are you sure TransformSystem is thread-local?

Well. After you mention that I think it is not :)

torkleyy · 2017-06-30T14:03:02Z

examples/04_pong/main.rs

@@ -342,7 +335,8 @@ fn main() {
    let mut game = Application::build(Pong, cfg)
        .register::<Ball>()
        .register::<Plank>()
-        .with::<PongSystem>(PongSystem, "pong_system", 1)
+        .with::<PongSystem>(PongSystem, "pong_system", &[])
+        .with_thread_local::<TransformSystem>(TransformSystem::new())


Same here (see one of the comments below).

torkleyy · 2017-06-30T14:03:28Z

examples/03_renderable/main.rs

@@ -292,7 +285,8 @@ fn main() {
                       env!("CARGO_MANIFEST_DIR"));
    let cfg = DisplayConfig::load(path);
    let mut game = Application::build(Example, cfg)
-        .with::<ExampleSystem>(ExampleSystem, "example_system", 1)
+        .with::<ExampleSystem>(ExampleSystem, "example_system", &[])
+        .with_thread_local::<TransformSystem>(TransformSystem::new())


… issues

ebkalderon

@omni-viral Since you submitted ebkalderon#2, won't this PR conflict when I merge into develop again? I think we should accept this PR into the mainline and then I'll rebase on my end.

zakarumych · 2017-06-30T14:53:30Z

@ebkalderon There might be conflicts. You may rebase and reject ebkalderon/amethyst#2

ebkalderon · 2017-06-30T15:04:18Z

@omni-viral Sweet, I reverted it successfully. I will rebase on top of this PR once it's ready for merge!

torkleyy · 2017-06-30T15:12:38Z

@ebkalderon I'd like to review once more, if that's okay?

ebkalderon · 2017-06-30T15:15:21Z

Of course, go right ahead.

torkleyy

LGTM, thanks @omni-viral!

Xaeroxe

Looks great! Thanks for your work!

zakarumych · 2017-06-30T17:10:16Z

Thank you all for your feedback!
I'm glad I can help :)

torkleyy · 2017-06-30T17:47:40Z

src/engine/app.rs


        ApplicationBuilder {
            config: cfg,
            initial_state: initial_state,
-            planner: Planner::from_pool(World::new(), pool),
+            dispatcher_builder: DispatcherBuilder::new().with_pool(pool),
+            world: World::new(),


Hold on, shouldn't we give the user the chance to access the World before starting the game? Not exactly sure about it.

Yeah I think so. There is a space between when the Application is built and when we call .run() on it, so all it would take is adding a .world_mut() function to the Application.

Or passing in World instead of creating it inside the ApplicationBuilder.

Either or. Both are functionally the same I think.

Xaeroxe

@omni-viral I know this didn't exist prior to your pull request, but could you add a way for users to initialize the game world prior to executing .run() for the application? @torkleyy and I have both suggested some ways to do this, either is fine.

Xaeroxe

Perfect, thank you!

ebkalderon · 2017-06-30T18:28:39Z

@torkleyy Heads up! It's possible that this pull request might break your ongoing asset management work on #244. Please make sure to rebase against the latest develop if any breakage occurs.

torkleyy · 2017-06-30T20:16:59Z

@ebkalderon How is that? I don't even depend on amethyst crate so far, so I don't think I will have any problems rebasing.

torkleyy

I think we can merge now, thanks again @omni-viral!

zakarumych changed the title ~~Developer~~ Convert to specs v0.9 Jun 30, 2017

torkleyy reviewed Jun 30, 2017

View reviewed changes

viral added 5 commits June 30, 2017 17:35

specs v0.9 compiled

3abde41

examples on specs v.0.9

5882e0f

docs on specs v0.9

b535075

Remove adding default system . Add to . Fix formatting and commenting…

db1a1e9

… issues

Sort imports. Remove unncesessary

311b380

ebkalderon approved these changes Jun 30, 2017

View reviewed changes

TransformSystem is not thread-local. Sorting stuff

7b8bce2

torkleyy approved these changes Jun 30, 2017

View reviewed changes

Xaeroxe approved these changes Jun 30, 2017

View reviewed changes

torkleyy reviewed Jun 30, 2017

View reviewed changes

Xaeroxe suggested changes Jun 30, 2017

View reviewed changes

Add direct access to World

8ba1631

Xaeroxe approved these changes Jun 30, 2017

View reviewed changes

torkleyy approved these changes Jun 30, 2017

View reviewed changes

ebkalderon merged commit 4e3d397 into amethyst:develop Jun 30, 2017

zakarumych deleted the developer branch July 1, 2017 06:52

ebkalderon mentioned this pull request Jul 1, 2017

Fix appveyor CI failure due to dxguid #242

Closed

nchashch mentioned this pull request Jul 3, 2017

Release 0.5.0 #241

Closed

13 tasks


		type SystemData = (WriteStorage<'a, Ball>, WriteStorage<'a, Plank>, WriteStorage<'a, LocalTransform>, Fetch<'a, Camera>, Fetch<'a, Time>, Fetch<'a, InputHandler>, FetchMut<'a, Score>);

Convert to specs v0.9 #250

Convert to specs v0.9 #250

Conversation

zakarumych commented Jun 30, 2017

torkleyy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zakarumych Jun 30, 2017 • edited

Choose a reason for hiding this comment

torkleyy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torkleyy Jun 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebkalderon left a comment

Choose a reason for hiding this comment

zakarumych commented Jun 30, 2017 • edited

ebkalderon commented Jun 30, 2017

torkleyy commented Jun 30, 2017

ebkalderon commented Jun 30, 2017

torkleyy left a comment

Choose a reason for hiding this comment

Xaeroxe left a comment

Choose a reason for hiding this comment

zakarumych commented Jun 30, 2017

Choose a reason for hiding this comment

Xaeroxe Jun 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xaeroxe left a comment

Choose a reason for hiding this comment

Xaeroxe left a comment

Choose a reason for hiding this comment

ebkalderon commented Jun 30, 2017 • edited

torkleyy commented Jun 30, 2017

torkleyy left a comment

Choose a reason for hiding this comment

zakarumych Jun 30, 2017 •

edited

torkleyy Jun 30, 2017 •

edited

zakarumych commented Jun 30, 2017 •

edited

Xaeroxe Jun 30, 2017 •

edited

ebkalderon commented Jun 30, 2017 •

edited