Remove ExtractedView::hdr, add ExtractedView::texture_format, move compositing_space to ExtractedCamera#23734
Remove ExtractedView::hdr, add ExtractedView::texture_format, move compositing_space to ExtractedCamera#23734atlv24 wants to merge 5 commits intobevyengine:mainfrom
Conversation
|
It looks like your PR is a breaking change, but you didn't provide a migration guide. Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes. |
|
Can you add docs to ExtractedCamera/View clarifying the differences between them? (view can be any sort of rendering thing e.g. a shadow map, camera is stuff relevant to an actual camera) |
|
|
||
| /// Flags that specify features on the pipeline key. | ||
| flags: VolumetricFogPipelineKeyFlags, | ||
| texture_format: TextureFormat, |
There was a problem hiding this comment.
| texture_format: TextureFormat, | |
| /// Texture format of the view target | |
| texture_format: TextureFormat, |
There was a problem hiding this comment.
there's some semantics I want to figure out before writing this comment, specifically with how Hdr overrides the window texture format and uses a different internal. Still thinking
IceSentry
left a comment
There was a problem hiding this comment.
I really like the direction of this PR, bit +1 from me once CI is passing.
tychedelia
left a comment
There was a problem hiding this comment.
Ty, this is a helpful step towards some other goals I have here. I'm wondering for a few of these passes where you switched to camera.hdr, is there a strong reason not to have them use the view target texture?
| let mut view_key = MeshPipelineKey::from_msaa_samples(msaa.samples()) | ||
| | MeshPipelineKey::from_hdr(view.hdr); | ||
| // HACK: we should be specializing by texture format, not bool hdr, but mesh pipeline keys have limited space. | ||
| let is_hdr = view.texture_format == ViewTarget::TEXTURE_FORMAT_HDR; |
There was a problem hiding this comment.
hrm... yeah, don't love this. i think with how much we've improved specialization the cost of having a bigger mesh key would be worth investigating, it could totally be whatever
There was a problem hiding this comment.
yeah I think its fine, i think its the only iffy part. its not hard to have a followup yeet it and be independently benchmarked though
There was a problem hiding this comment.
but mesh pipeline keys have limited space.
Not really. In #22637 I made texture format a part of mesh key.
|
I like the direction you're proposing! very nice. Extracted views really shouldn't care about hdr or compositing space or not, cameras should. I did a bunch of research and testing for this PR and I tried fixing the 2d hdr bloom, 3d scene examples and also created a compositing space example. While trying not to reintroduce any of the antipatterns being cleaned up in this PR. atlv24#5 |
|
I hope we could put all webgpu renderable && blendable formats into the mesh pipeline key to reduce collisions with #22637. But I'm not against the current either. |
Objective
Solution
Testing
This needs a migration guide.