Skip to content

/three

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

Add PBR material type #61

Merged
merged 25 commits into from Jul 29, 2017

Conversation

@alteous
Copy link
Member

commented Jul 28, 2017 •
edited

This PR implements a physically-based rendering pipeline based on the glTF reference renderer.

There is one example provided, namely pbr, which should output the following.

render
@alteous

alteous reviewed Jul 28, 2017
View changes

data/shaders/pbr_ps.glsl

bool available(int flag)
{
return (u_PbrFlags & flag) == flag;

This comment has been minimized.

Sign in to view
Copy link
@alteous

alteous Jul 28, 2017

Author Member

This probably isn't the best strategy for good parallel performance but I'm not sure what else to do.
@alteous

alteous reviewed Jul 28, 2017
View changes

examples/pbr.rs
extern crate gltf;
extern crate three;

fn load_mesh(mesh: gltf::mesh::Mesh, factory: &mut three::Factory) -> three::Mesh {

This comment has been minimized.

Sign in to view
Copy link
@alteous

alteous Jul 28, 2017 •
edited

Author Member

Native glTF support coming soon.
@alteous

alteous reviewed Jul 28, 2017
View changes

src/factory.rs
vertical_wrap_mode: WrapMode,
) -> Sampler {
use gfx::texture::Lod;
let info = gfx::texture::SamplerInfo {

This comment has been minimized.

Sign in to view
Copy link
@alteous

alteous Jul 28, 2017

Author Member

The Lod values were copied from gfx::texture::SamplerInfo::new().
@alteous

alteous reviewed Jul 28, 2017
View changes

src/render.rs
shadow_map1: (shadow1.clone(), shadow_sampler.clone()),
out_color: self.out_color.clone(),
out_depth: self.out_depth.clone(),
match *material {

This comment has been minimized.

Sign in to view
Copy link
@alteous

alteous Jul 28, 2017

Author Member

This match expression is bloated. It may be worth splitting up the cases in the future.
@alteous
Update anim example
Loading status checks…
7d77e51
@kvark

kvark approved these changes Jul 28, 2017
View changes

Copy link
Collaborator

left a comment

This is incredible, great work!
Just one thing I'd love to get addressed before we proceed.
src/factory.rs Outdated
}).collect()
}
let position_iter = shape.vertices.iter();
let normal_iter: Box<Iterator<Item = [gfx::format::I8Norm; 4]>> = {

This comment has been minimized.

Sign in to view
Copy link
@kvark

kvark Jul 28, 2017

Collaborator

I'd prefer us to avoid Box here, especially wrapping the iterator, since that would mean a virtual dispatch per each iteration...

This comment has been minimized.

Sign in to view
Copy link
@alteous

alteous Jul 28, 2017 •
edited

Author Member

Would itertools::Either be a good alternative? We would have to rely on the compiler moving the loop condition outside of the loop. There are a lot of branches to consider otherwise.

This comment has been minimized.

Sign in to view
Copy link
@alteous

alteous Jul 28, 2017

Author Member

So the code would like this:

let position_iter = shape.vertices.iter();
let normal_iter = if shape.normals.is_empty() {
    Either::Left(iter::repeat(NORMAL_Z))
} else {
    Either::Right(shape.normals.iter().map(...))
};
// etc.

This comment has been minimized.

Sign in to view
Copy link
@kvark

kvark Jul 28, 2017

Collaborator

Sounds good
@alteous

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Jul 28, 2017

Thanks - glad to be of service!
@kvark

kvark approved these changes Jul 29, 2017
View changes

Copy link
Collaborator

left a comment

Beautiful!
@kvark

This comment has been minimized.

Sign in to view
Copy link
Collaborator

commented Jul 29, 2017

bors r+

bors bot added a commit that referenced this pull request Jul 29, 2017

@bors
Merge #61 
61: Add PBR material type r=kvark

This PR implements a physically-based rendering pipeline based on the [glTF reference renderer](https://github.com/KhronosGroup/glTF-WebGL-PBR).

There is one example provided, namely `pbr`, which should output the following.

![render](https://user-images.githubusercontent.com/8549126/28728019-eae7b160-73be-11e7-833e-d37c02f4df2a.png)
Loading status checks…
40a3ce6
@kvark

This comment has been minimized.

Sign in to view
Copy link
Collaborator

commented Jul 29, 2017

Not too shabby for the first PR in this project! Please keep up :) 👍 👏
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Jul 29, 2017

Build succeeded

@bors bors bot merged commit 5507206 into three-rs:master Jul 29, 2017
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
bors Build succeeded
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kvark kvark referenced this pull request Aug 14, 2017

Closed

Physically Based Rendering #21

@alteous alteous deleted the alteous:pbr branch Nov 5, 2017

kvark pushed a commit that referenced this pull request Nov 10, 2017

@bors
Merge #61 
61: Add PBR material type r=kvark

This PR implements a physically-based rendering pipeline based on the [glTF reference renderer](https://github.com/KhronosGroup/glTF-WebGL-PBR).

There is one example provided, namely `pbr`, which should output the following.

![render](https://user-images.githubusercontent.com/8549126/28728019-eae7b160-73be-11e7-833e-d37c02f4df2a.png)
dc2734d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kvark kvark

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@alteous @kvark
You can’t perform that action at this time.