Allowed creating uninitialized images (for use as storage textures)#17760
Allowed creating uninitialized images (for use as storage textures)#17760alice-i-cecile merged 13 commits intobevyengine:mainfrom
Conversation
This allows `Image` to be used to create an uninitialized texture for use as a storage texture
|
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
|
When I ran cargo check I didn't enable all features. :( |
| let texture_end = texture_begin + rect_width * format_size; | ||
| atlas_texture.data[begin..end] | ||
| .copy_from_slice(&texture.data[texture_begin..texture_end]); | ||
| let Some(ref mut atlas_data) = atlas_texture.data else { |
There was a problem hiding this comment.
This could be moved out of the loop and it should maybe be turned into an error instead of a log.
There was a problem hiding this comment.
I'll move it outside the loop, although I'm not sure what you mean by an error instead of a log. It's an error already, just not a panic, but we shouldn't panic unless we have to
There was a problem hiding this comment.
Error as in, return a Result::Err not just a log.
There was a problem hiding this comment.
should I also keep the error! ?
There was a problem hiding this comment.
this requires me to add another variant to TextureAtlasBuilderError. Which isn't currently non_exhaustive, should I make it non_exhaustive?
There was a problem hiding this comment.
more generally, is it Bevy policy to always make error enums non_exhaustive
There was a problem hiding this comment.
Given the prevalence of breaking changes at this stage, we don't particularly care about non_exhaustive.
And in some cases, like the ECS, we can pretty reliably predict that error enums are exhaustive.
Co-authored-by: IceSentry <IceSentry@users.noreply.github.com>
IceSentry
left a comment
There was a problem hiding this comment.
Overall this looks like a useful change, but I would prefer if you used expect() instead of unwrap in code that isn't inside a test. I didn't add a comment on every unwrap but a few of them should definitely be updated.
IceSentry
left a comment
There was a problem hiding this comment.
I think we should just return Err and let users decide if they want to log. Other than that LGTM
|
Nice error handling, and a well-motivated little fix. Thanks. |
|
Please revert this change. Uninitialized images can be expressed as a zero sized Vec. This change hurts the usability of let Some(data) = &mut data else {return;}in a lot of situations since |
|
There is certainly a valid argument with the unwrap thing. But using an empty Vec to represent an uninitialised Image is just a really bad idea. It's functionally equivalent to an Option but utterly fails to express the intent and is very arguably not 'correct'. I don't consider it an option. There definitely needs to be a way to have uninitialised images, and there is significant gain from unifying all image related things with this one type. Most users don't need to read image data, and writing image data isn't a problem. This has already passed code review and been merged so I'm not sure what the protocol is for reverting something. But any case I don't have the power to do that. |
|
Also doesn't let Some(ref mut data) = &mut data else {return;}work just fine? |
|
Yeah, reverting isn't the solution here. Most users don't need to access the data directly and it makes sense to use Option for uninitialized images. Using |
I agree.
After some thought, I think the real solution is we should add some methods to edit the image, then we can sort of side step this issue. image.edit_region::<Srgba>(rect, |x, y, rgba: &mut Srgba| {
}) |
Objective
#17746
Solution
Image.datafrom being aVec<u8>to aOption<Vec<u8>>Testing
All current tests pass
Tested a variety of existing examples to make sure they don't crash (they don't)
Linux x86 64-bit NixOS
Migration Guide
Code that directly access
Imagedata will now need to use unwrap or handle the case where no data is provided.Behaviour of new_fill slightly changed, but not in a way that is likely to affect anything. It no longer panics and will fill the whole texture instead of leaving black pixels if the data provided is not a nice factor of the size of the image.