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

Directional light and shadow #6

Merged
merged 52 commits into from
Jul 8, 2021

Conversation

superdump
Copy link

No description provided.

superdump and others added 30 commits June 19, 2021 17:30
Normal maps are not included here as they require tangents in a vertex attribute.
From 'fintelia' on the Bevy Render Rework Round 2 discussion:

"My understanding is that GPUs these days never use the "execute both branches
and select the result" strategy. Rather, what they do is evaluate the branch
condition on all threads of a warp, and jump over it if all of them evaluate to
false. If even a single thread needs to execute the if statement body, however,
then the remaining threads are paused until that is completed."
The StandardMaterial_ prefix is no longer needed
The uniform contains the view_projection matrix so this was incorrect.
Still looking along the light's direction.
This demonstrates peter panning problems with shadows.
@superdump superdump force-pushed the directional-light-and-shadow branch from 805f82d to bbb9a81 Compare July 2, 2021 07:49
@superdump
Copy link
Author

Updated, note that it depends on #18.

@cart
Copy link
Owner

cart commented Jul 7, 2021

Can we decouple this from #18? I'd like to hold off on taking dependencies on the crevice changes until we're certain they are the right path forward.

@superdump
Copy link
Author

Can we decouple this from #18? I'd like to hold off on taking dependencies on the crevice changes until we're certain they are the right path forward.

I don’t have much time now as I am back at work. If I do this then I will not use crevice at all for problematic structs and will instead just use manual padding, manual sizes, and bytemuck. Is this ok?

@superdump
Copy link
Author

Can we decouple this from #18? I'd like to hold off on taking dependencies on the crevice changes until we're certain they are the right path forward.

I don’t have much time now as I am back at work. If I do this then I will not use crevice at all for problematic structs and will instead just use manual padding, manual sizes, and bytemuck. Is this ok?

Never mind, I can't even take this approach because GpuLights is stored in a DynamicUniformVec which depends on the AsStd140 trait.

@superdump
Copy link
Author

Oddly this now seems to work without any changes to crevice from what we're already using.

@cart
Copy link
Owner

cart commented Jul 8, 2021

Pushed a few superficial changes:

  1. Transform::rotate had the wrong order (and i wanted to use it in the examples so i fixed it)
  2. Manually specifying the light direction instead of using the Transform felt wrong, so I changed it. People will want to use the existing editor rotate/translate gizmos to mess with lights. All engines I'm aware of do it this way. This also has the benefit of removing the need to keep direction normalized, which allows us to use ..Default::default() for directional lights.
  3. Moved bias default constants into PointLight and DirectionalLight types. This makes the constant more easily discoverable / self documenting. And it decouples PointLight and DirectionalLight biases, which shouldn't necessarily be the same.
  4. Simplified the logic in the examples (because I want them to be as minimal as possible). I'd love to remove the manual light matrix construction too. We should give users simpler tools for this (although we can consider waiting until we adopt cascaded shadow maps or something similar).

@cart
Copy link
Owner

cart commented Jul 8, 2021

I doubt these changes will be particularly contentious, so I'm merging in the interest of moving forward. But feel free to contest anything. These changes are easy to revert.

Great work here!

@cart cart merged commit bd13dce into cart:pipelined-rendering Jul 8, 2021
@superdump
Copy link
Author

Pushed a few superficial changes:
1. Transform::rotate had the wrong order (and i wanted to use it in the examples so i fixed it)

Ok. :)

2. Manually specifying the light direction instead of using the Transform felt wrong, so I changed it. People will want to use the existing editor rotate/translate gizmos to mess with lights. All engines I'm aware of do it this way. This also has the benefit of removing the need to keep direction normalized, which allows us to use `..Default::default()` for directional lights.

Yeah, I had the same thought with wanting to use transforms and visible gizmos for tweaking them, I just didn't change it from how it was done in current bevy. I had made the direction pub on the perspective-infinite-reverse-rh branch updates because the inability to use our standard ..Default::default() pattern was very irritating. I prefer this. Thank you!

3. Moved bias default constants into PointLight and DirectionalLight types. This makes the constant more easily discoverable / self documenting. And it decouples PointLight and DirectionalLight biases, which shouldn't necessarily be the same.

I agree. I did this in the perspective-infinite-reverse-rh branch as well.

4. Simplified the logic in the examples (because I want them to be as minimal as possible). I'd love to remove the manual light matrix construction too. We should give users simpler tools for this (although we can consider waiting until we adopt cascaded shadow maps or something similar).

Yes, I agree. When we have the bounding volumes types internally, we can do this. I could do it already with bevy_mod_bounding but that's an external plugin. 🙂

cart pushed a commit that referenced this pull request Jul 24, 2021
Directional light and shadow
cart pushed a commit that referenced this pull request Jul 24, 2021
Directional light and shadow
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants