Specs aware Pipeline #317

omni-viral · Aug 29, 2017

Polymorphic `Pipeline`

What does polymorphic Pipeline do?
Just the same as the old Pipeline did, applies Passes.
What is the difference? Pass gets type parameter.

What is it for?

Pass with type parameter can receive different data to draw on frame.
One pass can get each entity with Mesh and Material, all Lights and single camera and draw shaded meshes.
Another can get only Mesh, Material pairs prepare everything to be drawn with deferred shading technique by next pass that gets called for each Light.
Special pass can draw animated 2d sprites from spritesheets. It requires no mesh but texture and additional data to pick sprite from spritesheet.

Integration with `specs`

This Pipeline defines input Data that implements specs::SystemData and may be fetched by RenderSystem.

Complications

In polymorphic Pipeline Pass is not object-safe trait and implementers can't be stored in Boxed trait objects. Therefore heterogeneous sequences are used.
Heterogeneous sequences require a little bit more effort to work with. But hetseq provides all functionality we need.

Woops. The recent rustfmt merge probably messed this up. Do you mind fixing it?

Xaeroxe · Aug 29, 2017

Woops. The recent rustfmt merge probably messed this up. Do you mind fixing it?

I will

omni-viral · Aug 29, 2017

I will

Got some compile errors :/

error[E0243]: wrong number of type arguments: expected 1, found 0
  --> src/ecs/rendering/systems.rs:73:45
   |
73 | impl<'a, 'b> SystemExt<'a, (&'b EventsLoop, PipelineBuilder, Option<DisplayConfig>)>
   |                                             ^^^^^^^^^^^^^^^ expected 1 type argument
error[E0243]: wrong number of type arguments: expected 1, found 0
  --> src/ecs/rendering/systems.rs:22:11
   |
22 |     pipe: Pipeline,
   |           ^^^^^^^^ expected 1 type argument
error[E0243]: wrong number of type arguments: expected 1, found 0
  --> src/ecs/rendering/systems.rs:78:50
   |
78 |         (events, pipe, config): (&'b EventsLoop, PipelineBuilder, Option<DisplayConfig>),
   |                                                  ^^^^^^^^^^^^^^^ expected 1 type argument
error[E0243]: wrong number of type arguments: expected 1, found 0
   --> src/app.rs:242:15
    |
242 |         pipe: PipelineBuilder,
    |               ^^^^^^^^^^^^^^^ expected 1 type argument

Can you fix these please?

Xaeroxe · Aug 29, 2017

Got some compile errors :/

error[E0243]: wrong number of type arguments: expected 1, found 0
  --> src/ecs/rendering/systems.rs:73:45
   |
73 | impl<'a, 'b> SystemExt<'a, (&'b EventsLoop, PipelineBuilder, Option<DisplayConfig>)>
   |                                             ^^^^^^^^^^^^^^^ expected 1 type argument
error[E0243]: wrong number of type arguments: expected 1, found 0
  --> src/ecs/rendering/systems.rs:22:11
   |
22 |     pipe: Pipeline,
   |           ^^^^^^^^ expected 1 type argument
error[E0243]: wrong number of type arguments: expected 1, found 0
  --> src/ecs/rendering/systems.rs:78:50
   |
78 |         (events, pipe, config): (&'b EventsLoop, PipelineBuilder, Option<DisplayConfig>),
   |                                                  ^^^^^^^^^^^^^^^ expected 1 type argument
error[E0243]: wrong number of type arguments: expected 1, found 0
   --> src/app.rs:242:15
    |
242 |         pipe: PipelineBuilder,
    |               ^^^^^^^^^^^^^^^ expected 1 type argument

Can you fix these please?

Amethyst is not compiling. I've worked only in renderer crate yet.

omni-viral · Aug 29, 2017

Amethyst is not compiling. I've worked only in renderer crate yet.

torkleyy · Aug 31, 2017

torkleyy reviewed Aug 31, 2017

View changes

torkleyy · Aug 31, 2017

amethyst_renderer/src/pipe/stage.rs

+}
+
+
+


Could you please reduce the number of subsequent empty lines to 1? I can see why some people use two of them, but 4?

If this is biggest issue in this PR I call it success 😄

Can we update amethyst to use this new PR properly prior to merging? I like it when Travis is happy.

Xaeroxe · Aug 31, 2017

Can we update amethyst to use this new PR properly prior to merging? I like it when Travis is happy.

@Xaeroxe of course. Supporting new feature in RenderSystem and other parts of main crate will be step two. But will be done prior merging.

omni-viral · Aug 31, 2017

@Xaeroxe of course. Supporting new feature in RenderSystem and other parts of main crate will be step two. But will be done prior merging.

Xaeroxe · Sep 8, 2017

Xaeroxe approved these changes Sep 8, 2017

View changes

I love it! This looks to have better performance and be more robust. Thanks!

Things to resolve before I can r+ this:

Resolve merge conflicts
Update amethyst to use this correctly.

Xaeroxe · Sep 8, 2017

Things to resolve before I can r+ this:

Resolve merge conflicts
Update amethyst to use this correctly.

Aceeri · Sep 8, 2017

Aceeri requested changes Sep 8, 2017

View changes

Aceeri · Sep 8, 2017

amethyst_renderer/src/light.rs

+
+impl Component for Light {
+    type Storage = DenseVecStorage<Self>;
+}


Probably want to fix these missing newlines.

Aceeri · Sep 8, 2017

amethyst_renderer/src/mesh.rs

@@ -150,3 +152,7 @@ where
         })
     }
 }
+
+impl Component for Mesh {
+    type Storage = VecStorage<Self>;


Any reason for Vec over DenseVec?

It's just the same MeshComponent uses now.
I haven't dive deep into "what Storage to use?" yet.

Aceeri · Sep 8, 2017

amethyst_renderer/src/pipe/pass.rs

-pub trait PassCompiler {
-    fn compile(&self, effect: NewEffect) -> Result<Effect>;
+pub trait PassData<'a> {
+    type Data: SystemData<'a> + Send;


Are these "parent" traits to SystemData for extensibility? Or is it just restricting the types more?

Xaeroxe · Sep 9, 2017

Xaeroxe approved these changes Sep 9, 2017

View changes

Looks great! I'm excited to see it in action!

This pr breaks pr #334 ,making is unusable.

jojolepro · Sep 15, 2017

This pr breaks pr #334 ,making is unusable.

@jojolepro This PR is going to have to be rebased anyways, given that #334 is closer to completion let's merge it first.

Xaeroxe · Sep 15, 2017

@jojolepro This PR is going to have to be rebased anyways, given that #334 is closer to completion let's merge it first.

#334 Got closed as it would be useless to make it just to see it replaced soon after.
Hype for the new pipeline! 👍

jojolepro · Sep 16, 2017

#334 Got closed as it would be useless to make it just to see it replaced soon after.
Hype for the new pipeline! 👍

Rhuagh · Sep 16, 2017

Rhuagh reviewed Sep 16, 2017

View changes

Rhuagh · Sep 16, 2017

src/ecs/rendering/systems.rs

+    for RenderSystem<P>
+    where B: PipelineBuild<Pipeline=P>,
+          P: Pipeline,
+{
     /// Create new `RenderSystem`


Can't you remove the P bound here, and just do for RenderSystem<B::Pipeline> where B: PipelineBuild?

jojolepro · Sep 16, 2017

jojolepro reviewed Sep 16, 2017

View changes

As this is absolutely massive and I don't want to spend a week reading the code and checking every single thing I'm unsure of, I put in comments my questions, mostly about naming. Hope this helps.

jojolepro · Sep 16, 2017

amethyst_renderer/Cargo.toml

 [dev-dependencies]
-genmesh = "0.4"
+genmesh = "0.4"


Should there be a new line on the last line? Git put a red symbol.

jojolepro · Sep 16, 2017

amethyst_renderer/examples/material.rs

@@ -103,7 +107,10 @@ fn main() {
             _ => (),
         });
 
-        renderer.draw(&scene, &pipe, delta);
+        let data = List::new().push(List::new().push(


A list in a list? Mind explaining why you need to have them like this?

It is List of List because Passes are in List in Stages which are in List in Pipeline. The data for Passes is arranged the same way.
Although examples from amethyst_renderer crate aren't refactored for latest changes in Pipeline.

It's a heterogenous list.

@Rhuagh yeah. Thank you. I forgot to mention that.

jojolepro · Sep 16, 2017

amethyst_renderer/examples/sphere.rs

@@ -70,7 +76,10 @@ fn main() {
             _ => (),
         });
 
-        renderer.draw(&scene, &pipe, delta);
+        let data = List::new().push(List::new().push(


jojolepro · Sep 16, 2017

amethyst_renderer/src/pass/shaded.rs

@@ -76,7 +82,44 @@ struct PointLight2 {
 
 unsafe impl Pod for PointLight2 {}


Naming? Doc? I have no idea what this is.

Not part of public API. Just implementation detail of the pass.

Well. If you're curious. This type us POD representation with padding of PointLight to pass into shader.

jojolepro · Sep 16, 2017

amethyst_renderer/src/prelude.rs

-pub use pipe::{Pipeline, PipelineBuilder, Stage, StageBuilder, Target};
-pub use scene::{Model, Scene};
+pub use pipe::{Pipeline, PipelineBuilder, Stage, StageBuilder, Target, TheStage, ThePipeline};
+//pub use scene::{Model, Scene};


Could be removed instead of commented?

jojolepro · Sep 16, 2017

src/ecs/rendering/systems.rs

-        world.register::<LightComponent>();
-        world.register::<MaterialComponent>();
-        world.register::<MeshComponent>();
+        world.register::<Light>();


Any reason to remove the "Component" part of the Component? It made it pretty easy to know the data we are manipulating.

Simplicity vs expressiveness basically. Just make sure that we are consistent across the engine with that to avoid confusion.

I have a plan to remove (Mesh/Material/Light)Component wrapping. But it is not in the scope of this PR. I'll remove those changes

(Please note that I didn't do a throughout check of the code)
There, have my seal of approval!
https://media.giphy.com/media/uTv1bQNRPFYTS/giphy.gif

jojolepro · Sep 16, 2017

(Please note that I didn't do a throughout check of the code)
There, have my seal of approval!
https://media.giphy.com/media/uTv1bQNRPFYTS/giphy.gif

Aceeri · Sep 16, 2017

Aceeri approved these changes Sep 16, 2017

View changes

Xaeroxe · Sep 17, 2017

Xaeroxe requested changes Sep 17, 2017

View changes

Documentation is a little bare in some places on 2nd review, otherwise this looks great, thanks!

Side note: if some of these generic bounds from the impl sections were instead added to the structure itself a lot of doc comments wouldn't be necessary.

Xaeroxe · Sep 17, 2017

Cargo.toml

@@ -37,9 +37,9 @@ num_cpus = "1.2"
 genmesh = "0.4"
 imagefmt = "4.0"
 shred = "0.4"
-specs = "0.9.5"
-rayon = "0.8"
+rayon = "0.7"


What's up with this downgrade?

We need to use same version specs does

Xaeroxe · Sep 17, 2017

amethyst_renderer/src/pass/flat.rs

@@ -17,14 +23,17 @@ static FRAG_SRC: &[u8] = include_bytes!("shaders/fragment/flat.glsl");
 
 /// Draw mesh without lighting
 #[derive(Clone, Debug, PartialEq)]
-pub struct DrawFlat<V> {
+pub struct DrawFlat<V, M, N, T> {


It would be nice to have a little documentation as to what all of these generic parameters are supposed to be

Xaeroxe · Sep 17, 2017

amethyst_renderer/src/pass/pbm.rs

@@ -20,18 +26,18 @@ static FRAG_SRC: &[u8] = include_bytes!("shaders/fragment/pbm.glsl");
 
 /// Draw mesh without lighting
 #[derive(Clone, Debug, PartialEq)]
-pub struct DrawPbm<V> {
+pub struct DrawPbm<V, A, M, N, T, L> {


I would also like to know what all of these parameters are at a glance

Xaeroxe · Sep 17, 2017

amethyst_renderer/src/pass/shaded.rs

@@ -20,17 +26,17 @@ static FRAG_SRC: &[u8] = include_bytes!("shaders/fragment/shaded.glsl");
 
 /// Draw mesh without lighting
 #[derive(Clone, Debug, PartialEq)]
-pub struct DrawShaded<V> {
+pub struct DrawShaded<V, A, M, N, T, L> {


Please document all of these generic parameters

Xaeroxe · Sep 17, 2017

amethyst_renderer/src/pipe/pass.rs

-            effect,
-            inner: self.0,
-        })
+    unsafe fn get(&self) -> (&mut Encoder, &mut Effect) {


I know this isn't part of the public API but just for the sake of future maintainers can we document why this is unsafe and what conditions need to be guaranteed by the developer before they call this?

@Xaeroxe I follow the idea to have minimum bounds possible. Therefore I almost never introduce bounds on struct definition.

omni-viral · Sep 17, 2017

@Xaeroxe I follow the idea to have minimum bounds possible. Therefore I almost never introduce bounds on struct definition.

That makes sense, can we still add doc comments for what those parameters are supposed to be?

Xaeroxe · Sep 17, 2017

That makes sense, can we still add doc comments for what those parameters are supposed to be?

torkleyy · Sep 17, 2017

torkleyy reviewed Sep 17, 2017

View changes

torkleyy · Sep 17, 2017

examples/06_material/resources/config.ron

@@ -0,0 +1,10 @@
+(
+  dimensions: None,


Let's use recommended formatting (so 4 spaces).

I've copied it from other examples. Should I fix formatting there too?

torkleyy · Sep 17, 2017

examples/06_material/main.rs

+    let config = DisplayConfig::load(&path);
+    let mut game = Application::build(Example)?
+        .with_renderer(
+            ThePipeline::build().with_stage(


I'm not sure we want a type named ThePipeline

I gave it the name just because Pipeline is occupied by trait 😛
If you have better idea you're welcome to share 😄

I guess my question is why do we have a trait for this? Under what circumstances would someone want to use a pipeline other than ours?

We have a trait to simplify bounds on type parameters.
Either it is P where P: Pipeline or Pipeline<S> where S: Stages.
Well. Before refactoring the second variant was much more sophisticated...

And I think that PIpeline<S> where S: Stages looks unclear rather than P: Pipeline.

jojolepro · Sep 18, 2017

jojolepro approved these changes Sep 18, 2017

View changes

Xaeroxe · Sep 19, 2017

Xaeroxe approved these changes Sep 19, 2017

View changes

LGTM! Thanks! I've run this locally and it seems to work pretty well. Good work!

bors r+

👎 Rejected by label

bors · Sep 19, 2017

👎 Rejected by label

bors r+

Xaeroxe · Sep 19, 2017

bors r+

Timed out

bors · Sep 19, 2017

Timed out

Sigh.. overriding.

Xaeroxe · Sep 19, 2017

Sigh.. overriding.

omni-viral requested review from ebkalderon, Xaeroxe and torkleyy Aug 29, 2017

Xaeroxe removed their request for review Aug 31, 2017

torkleyy added project: rendering status: working type: feature labels Sep 1, 2017

omni-viral changed the title from [draft] Polymorphic pipeline to [WIP] Specs aware Pipeline Sep 8, 2017

omni-viral requested a review from Xaeroxe Sep 9, 2017

omni-viral referenced this pull request Sep 15, 2017
Closed
Window resize fix #334

omni-viral changed the title from [WIP] Specs aware Pipeline to Specs aware Pipeline Sep 16, 2017

omni-viral requested a review from Xaeroxe Sep 16, 2017

omni-viral added a commit to omni-viral/amethyst that referenced this pull request Sep 16, 2017

Addressing issues in PR #317

51dd794

Xaeroxe referenced this pull request Sep 18, 2017
Closed
UI Framework #365
4 of 4 tasks complete

omni-viral referenced this pull request Sep 19, 2017
Closed
Crashes on Intel HD cpu driver (cannot reproduce) #344

omni-viral added some commits Sep 19, 2017

Polypipe squashed again

Loading status checks…

d286f14

Clear warnings

Loading status checks…

3269fd8

Xaeroxe added status: ready and removed status: working labels Sep 19, 2017

Xaeroxe merged commit 1294bef 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

omni-viral deleted the omni-viral:polypipe branch Sep 26, 2017

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

Specs aware Pipeline (#317)
* Polypipe squashed again * Clear warnings

48483bc

amethyst/amethyst

Join GitHub today

Specs aware Pipeline #317

Conversation

omni-viral commented Aug 29, 2017 • edited Edited 1 time omni-viral edited Sep 8, 2017 (most recent)

Polymorphic Pipeline

What is it for?

Integration with specs

Complications

This comment has been minimized.

Xaeroxe commented Aug 29, 2017

This comment has been minimized.

omni-viral commented Aug 29, 2017

omni-viral requested review from ebkalderon, Xaeroxe and torkleyy Aug 29, 2017

This comment has been minimized.

Xaeroxe commented Aug 29, 2017

This comment has been minimized.

omni-viral commented Aug 29, 2017

torkleyy reviewed Aug 31, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Xaeroxe commented Aug 31, 2017

Xaeroxe removed their request for review Aug 31, 2017

This comment has been minimized.

omni-viral commented Aug 31, 2017

torkleyy added project: rendering status: working type: feature labels Sep 1, 2017

omni-viral changed the title from [draft] Polymorphic pipeline to [WIP] Specs aware Pipeline Sep 8, 2017

Xaeroxe approved these changes Sep 8, 2017 View changes

This comment has been minimized.

Xaeroxe commented Sep 8, 2017 • edited by omni-viral Edited 1 time omni-viral edited Sep 16, 2017 (most recent)

Aceeri requested changes Sep 8, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

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

This comment has been minimized.

omni-viral requested a review from Xaeroxe Sep 9, 2017

Xaeroxe approved these changes Sep 9, 2017 View changes

This comment has been minimized.

jojolepro commented Sep 15, 2017

This comment has been minimized.

Xaeroxe commented Sep 15, 2017

omni-viral referenced this pull request Sep 15, 2017

This comment has been minimized.

jojolepro commented Sep 16, 2017

omni-viral changed the title from [WIP] Specs aware Pipeline to Specs aware Pipeline Sep 16, 2017

Rhuagh reviewed Sep 16, 2017 View changes

This comment has been minimized.

omni-viral requested a review from Xaeroxe Sep 16, 2017

jojolepro reviewed Sep 16, 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.

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

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jojolepro Sep 16, 2017 • edited Edited 1 time jojolepro edited Sep 16, 2017 (most recent)

This comment has been minimized.

omni-viral added a commit to omni-viral/amethyst that referenced this pull request Sep 16, 2017

This comment has been minimized.

jojolepro commented Sep 16, 2017 • edited Edited 1 time jojolepro edited Sep 16, 2017 (most recent)

Aceeri approved these changes Sep 16, 2017 View changes

Xaeroxe requested changes Sep 17, 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.

omni-viral commented Aug 29, 2017 •

edited

Edited 1 time

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

Polymorphic `Pipeline`

Integration with `specs`

torkleyy reviewed Aug 31, 2017

View changes

Xaeroxe approved these changes Sep 8, 2017

View changes

Xaeroxe commented Sep 8, 2017 •

edited by omni-viral

Edited 1 time

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

Aceeri requested changes Sep 8, 2017

View changes

omni-viral Sep 9, 2017 •

edited

Edited 1 time

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

Xaeroxe approved these changes Sep 9, 2017

View changes

Rhuagh reviewed Sep 16, 2017

View changes

jojolepro reviewed Sep 16, 2017

View changes

omni-viral Sep 19, 2017 •

edited

Edited 1 time

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

jojolepro Sep 16, 2017 •

edited

Edited 1 time

jojolepro edited Sep 16, 2017 (most recent)

jojolepro commented Sep 16, 2017 •

edited

Edited 1 time

jojolepro edited Sep 16, 2017 (most recent)

Aceeri approved these changes Sep 16, 2017

View changes

Xaeroxe requested changes Sep 17, 2017

View changes