Fix AtmosphereEnvironmentMap handling - #25699
Conversation
JMS55
added
C-Bug
| mut commands: Commands, | ||
| ) { | ||
| // Get the first view entity's textures to borrow | ||
| let Some(view_textures) = view_textures.iter().next() else { |
There was a problem hiding this comment.
I know this is what the previous code was doing, but why not use .single()? And if we can't use .single() then what happens if there's more than one view_texture?
There was a problem hiding this comment.
I uhh, couldn't say lol. Single would throw an error if there's more than 1, which, maybe there could be? Idk.
There was a problem hiding this comment.
.single() would fail with two cameras that have AtmosphereSettings. Also I think this can be on a LightProbe, which doesn't have its own AtmosphereTextures -- I think this is the correct/only option
There was a problem hiding this comment.
To be clear, I get that it would throw an error. I was more wondering about what it should be doing if there's more than one because the current code seems wrong? Like, should it at least print a warning or something?
| ..Default::default() | ||
| }); | ||
| if already_added_env_map { | ||
| entity.remove::<( |
There was a problem hiding this comment.
I'm a bit confused by this. You're removing these components but then adding them back with the new values? Is that remove necessary? Isn't the end result going to be the same?
There was a problem hiding this comment.
This handles reinserting a different amosphere env map (e.g. a different size) on the same entity.
IceSentry
left a comment
IceSentry
left a comment
There was a problem hiding this comment.
+1 on the try_remove suggestions. I still feel like the .iter().next() thing is a bit weird but it's out of scope for this PR to solve.
LGTM
Co-authored-by: Stuart Parmenter <pavlov@pavlov.net>
Co-authored-by: Stuart Parmenter <pavlov@pavlov.net>
JMS55
added
S-Ready-For-Final-Review
3 participants
Objective
Solution
Testing