/amethyst forked from amethyst/amethyst

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rendering stuff #3

Merged
merged 19 commits into from Jul 24, 2017

Conversation

Reviewers

@ebkalderon ebkalderon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@omni-viral @ebkalderon
@omni-viral

omni-viral commented Jul 8, 2017

  • Tweak VertexFormat trait. Adding an ability to identify kind of data an attribute belongs
  • Add missing stuff to EffectBuilder
  • Link unknown outputs in effect::pso::Link
  • Add Pass variant with function that receives models
  • Slice models into chunks and feed 'em to encoders
    Old mechanism draw only first encoders.len() models
  • Add DrawFlat pass
  • Fix issue with black textures
    by actually passing texture data on texture object creation
  • Modify sphere example to render flat sphere using DrawFlat pass
@ebkalderon

This comment has been minimized.

Show comment
Hide comment
@ebkalderon

ebkalderon Jul 8, 2017

Owner

@omni-viral Thank you for the comprehensive PR! Looks very exciting. I will review it carefully and report back to you once I'm back in the States.
Owner

ebkalderon commented Jul 8, 2017

@omni-viral Thank you for the comprehensive PR! Looks very exciting. I will review it carefully and report back to you once I'm back in the States.
@omni-viral

This comment has been minimized.

Show comment
Hide comment
@omni-viral

omni-viral Jul 8, 2017

For now only one model could be rendered as ClearTarget pass get executed for each model 🦀
It needed to break passed into three kinds.

  • PrePass
    All PrePasses will get called first by main thread
  • MainPass
    Then MainPasses will get called for each model in scene in parallel with multiple encoders.
    Should differentMainPasses draw calls be allowed to intersect? Probably not.
  • PostPass
    And PostPasses will get called last by main thread

omni-viral commented Jul 8, 2017 •
edited
Edited 1 time
  • @omni-viral omni-viral edited Jul 11, 2017 (most recent)

For now only one model could be rendered as ClearTarget pass get executed for each model 🦀
It needed to break passed into three kinds.

  • PrePass
    All PrePasses will get called first by main thread
  • MainPass
    Then MainPasses will get called for each model in scene in parallel with multiple encoders.
    Should differentMainPasses draw calls be allowed to intersect? Probably not.
  • PostPass
    And PostPasses will get called last by main thread
@ebkalderon

ebkalderon requested changes Jul 21, 2017
View changes

Wow, this is a surprisingly comprehensive pull request! I am really impressed with your work here. That said, there are some nitpicks to address, but most are fairly minor.

amethyst_renderer/src/pipe/effect/mod.rs
+
+ /// Adds a texture to this `Effect`
+ pub fn with_texture(mut self, name: &'a str) -> Self
+ {

This comment has been minimized.

Show comment
Hide comment
@ebkalderon

ebkalderon Jul 21, 2017

Owner

This brace shouldn't be on its own line.
@ebkalderon

ebkalderon Jul 21, 2017

Owner

This brace shouldn't be on its own line.
amethyst_renderer/src/pipe/effect/pso.rs
- meta.const_bufs.push(meta_cbuf);
- desc.constant_buffers[info.slot as usize] = Some(d);
+ for info in info.constant_buffers.iter() {
+ // println!("Link constant {:?}/{:?}", info.name, cbuf);

This comment has been minimized.

Show comment
Hide comment
@ebkalderon

ebkalderon Jul 21, 2017

Owner

This commented code should probably be removed before merging.
@ebkalderon

ebkalderon Jul 21, 2017

Owner

This commented code should probably be removed before merging.
amethyst_renderer/src/pipe/effect/pso.rs
+ for info in info.constant_buffers.iter() {
+ // println!("Link constant {:?}/{:?}", info.name, cbuf);
+ if let Some(res) = meta_cbuf.link_constant_buffer(info, cbuf) {
+ // println!("Linked {:?}", res);

This comment has been minimized.

Show comment
Hide comment
@ebkalderon

ebkalderon Jul 21, 2017

Owner

This commented code should probably be removed before merging.
@ebkalderon

ebkalderon Jul 21, 2017

Owner

This commented code should probably be removed before merging.
amethyst_renderer/src/pipe/effect/pso.rs
- res.map_err(|e| InitError::GlobalConstant(info.name.as_str(), Some(e)))?;
- meta.globals.push(meta_global);
+ for info in info.globals.iter() {
+ // println!("Link global {:?}/{:?}", info.name, global);

This comment has been minimized.

Show comment
Hide comment
@ebkalderon

ebkalderon Jul 21, 2017

Owner

This commented code should probably be removed before merging.
@ebkalderon

ebkalderon Jul 21, 2017

Owner

This commented code should probably be removed before merging.
amethyst_renderer/src/pipe/effect/pso.rs
+ for info in info.globals.iter() {
+ // println!("Link global {:?}/{:?}", info.name, global);
+ if let Some(res) = meta_global.link_global_constant(info, global) {
+ // println!("Linked {:?}", res);

This comment has been minimized.

Show comment
Hide comment
@ebkalderon

ebkalderon Jul 21, 2017

Owner

This commented code should probably be removed before merging.
@ebkalderon

ebkalderon Jul 21, 2017

Owner

This commented code should probably be removed before merging.
amethyst_renderer/src/pass/shaders/fragment/flat.glsl
+
+void main() {
+ color = texture(albedo, vertex.tex_coord);
+}

This comment has been minimized.

Show comment
Hide comment
@ebkalderon

ebkalderon Jul 21, 2017

Owner

There is a missing newline at the end of this file.
@ebkalderon

ebkalderon Jul 21, 2017

Owner

There is a missing newline at the end of this file.

This comment has been minimized.

Show comment
Hide comment
@omni-viral

omni-viral Jul 24, 2017

Added
@omni-viral

omni-viral Jul 24, 2017

Added
amethyst_renderer/src/pass/shaders/fragment/pbm.glsl
+ vec3 color = ambient + lighted + emission;
+
+ out_color = vec4(color, 1.0);
+}

This comment has been minimized.

Show comment
Hide comment
@ebkalderon

ebkalderon Jul 21, 2017

Owner

There is a missing newline at the end of this file.
@ebkalderon

ebkalderon Jul 21, 2017

Owner

There is a missing newline at the end of this file.

This comment has been minimized.

Show comment
Hide comment
@omni-viral

omni-viral Jul 24, 2017

Added
@omni-viral

omni-viral Jul 24, 2017

Added
amethyst_renderer/src/pass/shaders/vertex/basic.glsl
+ vertex.tangent = mat3(model) * tangent;
+ vertex.tex_coord = tex_coord;
+ gl_Position = proj * view * vertex.position;
+}

This comment has been minimized.

Show comment
Hide comment
@ebkalderon

ebkalderon Jul 21, 2017

Owner

There is a missing newline at the end of this file.
@ebkalderon

ebkalderon Jul 21, 2017

Owner

There is a missing newline at the end of this file.

This comment has been minimized.

Show comment
Hide comment
@omni-viral

omni-viral Jul 24, 2017

Added
@omni-viral

omni-viral Jul 24, 2017

Added
amethyst_renderer/src/pass/shaders/fragment/pbm.glsl
@@ -0,0 +1,132 @@
+

This comment has been minimized.

Show comment
Hide comment
@ebkalderon

ebkalderon Jul 21, 2017

Owner

Empty line not necessary here.
@ebkalderon

ebkalderon Jul 21, 2017

Owner

Empty line not necessary here.

This comment has been minimized.

Show comment
Hide comment
@omni-viral

omni-viral Jul 24, 2017

Moved
@omni-viral

omni-viral Jul 24, 2017

Moved
amethyst_renderer/src/pass/shaders/vertex/basic.glsl
@@ -0,0 +1,27 @@
+

This comment has been minimized.

Show comment
Hide comment
@ebkalderon

ebkalderon Jul 21, 2017

Owner

Empty line not necessary here.
@ebkalderon

ebkalderon Jul 21, 2017

Owner

Empty line not necessary here.

This comment has been minimized.

Show comment
Hide comment
@omni-viral

omni-viral Jul 24, 2017

Moved
@omni-viral

omni-viral Jul 24, 2017

Moved
@omni-viral

This comment has been minimized.

Show comment
Hide comment
@omni-viral

omni-viral Jul 22, 2017

I think I'm done with refactoring and changes 😄

omni-viral commented Jul 22, 2017

I think I'm done with refactoring and changes 😄
@ebkalderon

This comment has been minimized.

Show comment
Hide comment
@ebkalderon

ebkalderon Jul 24, 2017

Owner

@omni-viral Looks pretty good to me. Thank you for addressing pretty much all my concerns, and I appreciate all your hard work! 🎉
Owner

ebkalderon commented Jul 24, 2017

@omni-viral Looks pretty good to me. Thank you for addressing pretty much all my concerns, and I appreciate all your hard work! 🎉

@ebkalderon ebkalderon merged commit bfa719c into ebkalderon:renderer Jul 24, 2017

@omni-viral omni-viral deleted the omni-viral:renderer_inc branch Jul 24, 2017

ebkalderon added a commit that referenced this pull request Aug 2, 2017

@omni-viral @ebkalderon
Rendering stuff (#3) 
* New DrawFlat pass. Example sphere works with DrawFlat

* Three pass kinds. Prep, Main and Post

* Implement some physically based rendering

* Revert unnecessary change

* Correct DrawFlat. Use size of vertex type from parameter.
Fix sphere example. Clear draw buffer.
Remove unnecessary 'use's

* Add new vertex format 'PosNormTangTex' which holds tangent vector along with normal.

* Correct default normal texture

* Accept tangent vector in shaders

* Correct fresnel calculation in pbm.glsl

* Use normal map in pbm.glsl
Use PosNormTangTex in material example

* Improve vertex attribute query by 'WithField' trait

* Make linking easier for users of 'Effect'

* Bound in 'Slice' instance in MeshBuilder::finish

* Fix data linking

* Move parts of shader algorithm into functions

* Apply passes in stage in sequence they were added. Use next slice of encoders for each pass (reusing last encoder from previous slice)

* Apply all parallelable passes with single for_each

* A little refactoring.
4f33bd0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment