Skip to content
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

[Merged by Bors] - Rebase of existing PBR work #1554

Closed
wants to merge 58 commits into from
Closed

Conversation

mtsr
Copy link
Contributor

@mtsr mtsr commented Mar 4, 2021

This is a rebase of StarArawns PBR work from #261 with IngmarBitters work from #1160 cherry-picked on top.

I had to make a few minor changes to make some intermediate commits compile and the end result is not yet 100% what I expected, so there's a bit more work to do.

@alice-i-cecile alice-i-cecile added help wanted A-Rendering Drawing game state to the screen labels Mar 4, 2021
@alice-i-cecile
Copy link
Member

Ping @StarArawn @IngmarBitter: are you okay if we close out the old PRs and work off of this?

@StarArawn
Copy link
Contributor

StarArawn commented Mar 4, 2021

@mtsr @alice-i-cecile If this is the preferred PR I'm fine with closing mine out. I don't really see a commit history with my commits, but I'm cool with that I suppose for the sake of getting the ball rolling on this.

@mtsr
Copy link
Contributor Author

mtsr commented Mar 4, 2021

@StarArawn I'm sorry about that. I did a rebase, but somewhere the author got mixed up. I can see if I can fix it.

If you check, you'll see your third commit it worked the way it should. It's the first two that got messed up.

@mtsr
Copy link
Contributor Author

mtsr commented Mar 5, 2021

I fixed the author on your commits, @StarArawn .

crates/bevy_pbr/src/light.rs Outdated Show resolved Hide resolved
crates/bevy_pbr/src/material.rs Outdated Show resolved Hide resolved
crates/bevy_pbr/src/material.rs Outdated Show resolved Hide resolved
crates/bevy_pbr/src/material.rs Outdated Show resolved Hide resolved
@cart cart mentioned this pull request Mar 6, 2021
@Ratysz Ratysz mentioned this pull request Mar 6, 2021
@mtsr

This comment has been minimized.

vec3 diffuse = diffuseColor * Fd_Burley(roughness, NdotV, NoL, LoH);

light_accum +=
((diffuse + specular) * light.color.xyz) * (light.color.w * NdotL);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless you're going to multiply the incoming light intensity by PI, you'd need to do that here so that the light intensities behave the way artists expect. Also, you could just remove the 1 / PI parts from the equations above and just leave a comment in them saying they'd be cancelled out anyway.
https://seblagarde.wordpress.com/2012/01/08/pi-or-not-to-pi-in-game-lighting-equation/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you on the bevy discord, by any chance? Would make it easier to ask some questions that don't really need to be archived here. My nick is the same there.

Copy link
Contributor Author

@mtsr mtsr Mar 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@mtsr mtsr Mar 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added what I think is the physically correct term in ac1e23c

I'll read the linked article and see if I can figure it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't find anything like this term PI in either the filament document or the code and my math is not good enough to reproduce the derivations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got a better grasp on this now, but I'd love for somebody else to chime in on how we want the light to behave. I agree that if we end up multiplying by PI we might as well remove the 1.0 / PI instead.

@mtsr

This comment has been minimized.

@mtsr

This comment has been minimized.

@mtsr

This comment has been minimized.

@mockersf
Copy link
Member

mockersf commented Mar 7, 2021

running through the 3d examples:

  • some could benefit from better light placement (3d_scene, msaa, orthographic, alien_cake_addict)
  • applying a texture is broken (texture, load_gltf) (missed renaming in forward.frag)

@mockersf
Copy link
Member

mockersf commented Mar 7, 2021

example wireframe (from #562) seems to not work as expected:
wireframe

@cart
Copy link
Member

cart commented Mar 20, 2021

bors r+

bors bot pushed a commit that referenced this pull request Mar 20, 2021
This is a rebase of StarArawns PBR work from #261 with IngmarBitters work from #1160 cherry-picked on top.

I had to make a few minor changes to make some intermediate commits compile and the end result is not yet 100% what I expected, so there's a bit more work to do.

Co-authored-by: John Mitchell <[email protected]>
Co-authored-by: Ingmar Bitter <[email protected]>
@bors
Copy link
Contributor

bors bot commented Mar 20, 2021

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Rebase of existing PBR work [Merged by Bors] - Rebase of existing PBR work Mar 20, 2021
@bors bors bot closed this Mar 20, 2021
bors bot pushed a commit that referenced this pull request Mar 26, 2021
This PR adds normal maps on top of PBR #1554. Once that PR lands, the changes should look simpler.

Edit: Turned out to be so little extra work, I added metallic/roughness texture too. And occlusion and emissive.

Co-authored-by: Carter Anderson <[email protected]>
@IngmarBitter
Copy link
Contributor

you guys did great work on this! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen
Projects
None yet
Development

Successfully merging this pull request may close these issues.