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

Fixed a bug where skybox ddsfile would crash from wgpu #12894

Merged
merged 2 commits into from Apr 8, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/bevy_render/Cargo.toml
Expand Up @@ -84,7 +84,7 @@ thread_local = "1.1"
thiserror = "1.0"
futures-lite = "2.0.1"
hexasphere = "10.0"
ddsfile = { version = "0.5.0", optional = true }
ddsfile = { version = "0.5.2", optional = true }
ktx2 = { version = "0.3.0", optional = true }
# For ktx2 supercompression
flate2 = { version = "1.0.22", optional = true }
Expand Down
105 changes: 98 additions & 7 deletions crates/bevy_render/src/texture/dds.rs
Expand Up @@ -24,23 +24,22 @@ pub fn dds_buffer_to_image(
}
let mut image = Image::default();
let is_cubemap = dds.header.caps2.contains(Caps2::CUBEMAP);
let mut depth_or_array_layers = if dds.get_num_array_layers() > 1 {
let depth_or_array_layers = if dds.get_num_array_layers() > 1 {
dds.get_num_array_layers()
} else {
dds.get_depth()
};
if is_cubemap {
if !dds.header.caps2.contains(
if is_cubemap
&& !dds.header.caps2.contains(
Caps2::CUBEMAP_NEGATIVEX
| Caps2::CUBEMAP_NEGATIVEY
| Caps2::CUBEMAP_NEGATIVEZ
| Caps2::CUBEMAP_POSITIVEX
| Caps2::CUBEMAP_POSITIVEY
| Caps2::CUBEMAP_POSITIVEZ,
) {
return Err(TextureError::IncompleteCubemap);
}
depth_or_array_layers *= 6;
)
{
return Err(TextureError::IncompleteCubemap);
}
image.texture_descriptor.size = Extent3d {
width: dds.get_width(),
Expand Down Expand Up @@ -277,3 +276,95 @@ pub fn dds_format_to_texture_format(
));
})
}

#[cfg(test)]
mod test {
use wgpu::{util::TextureDataOrder, TextureDescriptor, TextureDimension};

use crate::texture::CompressedImageFormats;

use super::dds_buffer_to_image;

/// `wgpu::create_texture_with_data` that reads from data structure but doesn't actually talk to your GPU
fn fake_wgpu_create_texture_with_data(desc: &TextureDescriptor<'_>, data: &[u8]) {
// Will return None only if it's a combined depth-stencil format
// If so, default to 4, validation will fail later anyway since the depth or stencil
// aspect needs to be written to individually
let block_size = desc.format.block_copy_size(None).unwrap_or(4);
let (block_width, block_height) = desc.format.block_dimensions();
let layer_iterations = desc.array_layer_count();

let outer_iteration;
let inner_iteration;
match TextureDataOrder::default() {
TextureDataOrder::LayerMajor => {
outer_iteration = layer_iterations;
inner_iteration = desc.mip_level_count;
}
TextureDataOrder::MipMajor => {
outer_iteration = desc.mip_level_count;
inner_iteration = layer_iterations;
}
}

let mut binary_offset = 0;
for outer in 0..outer_iteration {
for inner in 0..inner_iteration {
let (_layer, mip) = match TextureDataOrder::default() {
TextureDataOrder::LayerMajor => (outer, inner),
TextureDataOrder::MipMajor => (inner, outer),
};

let mut mip_size = desc.mip_level_size(mip).unwrap();
// copying layers separately
if desc.dimension != TextureDimension::D3 {
mip_size.depth_or_array_layers = 1;
}

// When uploading mips of compressed textures and the mip is supposed to be
// a size that isn't a multiple of the block size, the mip needs to be uploaded
// as its "physical size" which is the size rounded up to the nearest block size.
let mip_physical = mip_size.physical_size(desc.format);

// All these calculations are performed on the physical size as that's the
// data that exists in the buffer.
let width_blocks = mip_physical.width / block_width;
let height_blocks = mip_physical.height / block_height;

let bytes_per_row = width_blocks * block_size;
let data_size = bytes_per_row * height_blocks * mip_size.depth_or_array_layers;

let end_offset = binary_offset + data_size as usize;

assert!(binary_offset < data.len());
assert!(end_offset <= data.len());
// data[binary_offset..end_offset])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly. The commented line is how wgpu reads the data. I wanted to leave it here as a comment to show why the asserts are there.

Hexorg marked this conversation as resolved.
Show resolved Hide resolved

binary_offset = end_offset;
}
}
}

#[test]
fn dds_skybox() {
let buffer: [u8; 224] = [
0x44, 0x44, 0x53, 0x20, 0x7c, 0, 0, 0, 7, 0x10, 0x08, 0, 4, 0, 0, 0, 4, 0, 0, 0, 0x10,
0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0x47, 0x49, 0x4d, 0x50, 0x2d, 0x44, 0x44, 0x53, 0x5c,
0x09, 3, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0x20, 0, 0, 0, 4, 0, 0, 0, 0x44, 0x58, 0x54, 0x35, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0x08, 0x10, 0, 0, 0, 0xfe, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0xff, 0xff, 0x49, 0x92, 0x24, 0x49, 0x92, 0x24, 0xda, 0xd6,
0x2f, 0x5b, 0x8a, 0, 0xff, 0x55, 0xff, 0xff, 0x49, 0x92, 0x24, 0x49, 0x92, 0x24, 0xd5,
0x84, 0x8e, 0x3a, 0xb7, 0, 0xaa, 0x55, 0xff, 0xff, 0x49, 0x92, 0x24, 0x49, 0x92, 0x24,
0xf5, 0x94, 0x6f, 0x32, 0x57, 0xb7, 0x8b, 0, 0xff, 0xff, 0x49, 0x92, 0x24, 0x49, 0x92,
0x24, 0x2c, 0x3a, 0x49, 0x19, 0x28, 0xf7, 0xd7, 0xbe, 0xff, 0xff, 0x49, 0x92, 0x24,
0x49, 0x92, 0x24, 0x16, 0x95, 0xae, 0x42, 0xfc, 0, 0xaa, 0x55, 0xff, 0xff, 0x49, 0x92,
0x24, 0x49, 0x92, 0x24, 0xd8, 0xad, 0xae, 0x42, 0xaf, 0x0a, 0xaa, 0x55,
];
let r = dds_buffer_to_image("".into(), &buffer, CompressedImageFormats::BC, true);
assert!(r.is_ok());
if let Ok(r) = r {
fake_wgpu_create_texture_with_data(&r.texture_descriptor, &r.data);
}
}
}