Conversation
ghost
added
A-Rendering
superdump
left a comment
superdump
left a comment
There was a problem hiding this comment.
I wonder if we should just make use of wgpu's information directly and not bother wrapping it.
| } | ||
| #[inline] | ||
| fn pixel_size(&self) -> usize { | ||
| self.describe().block_size as usize |
There was a problem hiding this comment.
This is correct for non-block formats, but for block formats, it will be wrong. You kind of have to use both the block dimensions and the block byte size. It degrades to pixels as dimensions become 1x1.
There was a problem hiding this comment.
Hmm I am not familiar with compressed textures / block formats, so I am not sure how to proceed. But my "soon to be" terrain plugin depends on being able to use some of the unlocked formats.
Do you want to take over this PR?
There was a problem hiding this comment.
Block formats encode MxN pixels in block_size bytes. So the pixel_size would be the mean if that, ie block_size / (block_dimensions.0 * block_dimensions.1)
There was a problem hiding this comment.
Is this information even useful to have? I could just remove the pixel size thing entirely.
It is used in some places in the engine though and I am not sure who else is using it.
There was a problem hiding this comment.
@superdump I have updated the PR now. Is the block size guaranteed to be a multiple of its area? If not than this change is not correct.
There was a problem hiding this comment.
The block size is the size of a compressed block in bytes. So it accounts for all the data used to encode the block. I think I understand the intent of your question and the answer to that intent is yes. :)
There was a problem hiding this comment.
Mhh, I think the current implementation of pixel size is wrong.
For example the TextureFormat::Astc5x4RgbaUnorm has got a blocksize of 16 bytes and dimensions of 5 by 4 pixels. Thus the average pixel size is 6.4 bit/px. If that number is stored as a usize it is casted to 0, which is wrong.
Prior to my PR only some of the uncompressed textures, where supported by the pixel size method, so this was not an issue.
This method is referenced in 14 places in the bevy code base and always used to calculate the size of the texture in bytes.
By replacing pixel_size with describe().block_size we would enable bevys Image to be compatible with all uncompressed texture formats (This is the issue I am personally blocked on).
However the same is not true for all compressed formats.
I am not familiar with how to calculate their size properly. Do you have to divide the texture size in pixels by the block dimensions and subsequently multiply that by the block size? How would you deal with rounding?
Does the texture size refer to the dimensions in pixels or in blocks? If the former: How to deal with sizes that are not a multiple of the block dimensions?
There was a problem hiding this comment.
Ah, I see. Good catch. Yes, I suppose maybe the real API then should match wgpu's in the sense that it should expose block dimensions and block size in bytes.
However the same is not true for all compressed formats.
I am not familiar with how to calculate their size properly. Do you have to divide the texture size in pixels by the block dimensions and subsequently multiply that by the block size? How would you deal with rounding?
Does the texture size refer to the dimensions in pixels or in blocks? If the former: How to deal with sizes that are not a multiple of the block dimensions?
Yes, pretty much. For calculating the size of a texture, you have to round up to the next block. So if you have a block size of 4x4 and a texture that is 6x6 then that will be 2 blocks wide and 2 blocks high. I think elsewhere I have calculated this as image width in blocks = (image width in pixels + block width in pixels - 1) / block width in pixels and similar for height. And then use the image dimensions in blocks to calculate the image size. Note then that for non-compressed formats, where the block width and block height are both 1, image width in blocks will be equivalent to image width in pixels and the block size in bytes would just be the pixel size in bytes.
There was a problem hiding this comment.
I removed the flawed pixel size method in favor of the new texture size method. This should be calculating the texture size in bytes properly now.
superdump
left a comment
superdump
left a comment
There was a problem hiding this comment.
I think this looks good now, but I think it should be well tested before I approve it. :)
|
Since this PR is still quite controversial and large I have made a small PR to unblock heightmaps for me and others over in #5249. |
# Objective Currently some TextureFormats are not supported by the Image type. The `TextureFormat::R16Unorm` format is useful for storing heightmaps. This small change would unblock releasing my terrain plugin on bevy 0.8. ## Solution Added `TextureFormat::R16Unorm` support to Image. This is an alternative (short term solution) to the large texture format issue #4124.
# Objective Currently some TextureFormats are not supported by the Image type. The `TextureFormat::R16Unorm` format is useful for storing heightmaps. This small change would unblock releasing my terrain plugin on bevy 0.8. ## Solution Added `TextureFormat::R16Unorm` support to Image. This is an alternative (short term solution) to the large texture format issue bevyengine#4124.
# Objective Currently some TextureFormats are not supported by the Image type. The `TextureFormat::R16Unorm` format is useful for storing heightmaps. This small change would unblock releasing my terrain plugin on bevy 0.8. ## Solution Added `TextureFormat::R16Unorm` support to Image. This is an alternative (short term solution) to the large texture format issue bevyengine#4124.
|
Fixed by #6820. |
# Objective Currently some TextureFormats are not supported by the Image type. The `TextureFormat::R16Unorm` format is useful for storing heightmaps. This small change would unblock releasing my terrain plugin on bevy 0.8. ## Solution Added `TextureFormat::R16Unorm` support to Image. This is an alternative (short term solution) to the large texture format issue bevyengine#4124.
3 participants
Objective
Currently some
TextureFormats are not supported by the Image type.Solution
To remove that limitation I have removed the custom mapping from texture format to pixel size and replaced it with the wgpu equivalent.