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

Allow u32 coordinates for textureStore/textureLoad #2172

Merged
merged 11 commits into from
Dec 22, 2022

Conversation

PENGUINLIONG
Copy link
Contributor

As illustrated in https://www.w3.org/TR/WGSL/#texturestore:

F is a texel format
C is i32, or u32
...

So I guess vec<u32> should be a valid textureStore coordinate.

I saw that in back::glsl::write_texture_coord the coord is explicitly cast to ivec so I think that's good. Please remind me if anything should be changed accordingly in the codegens.

Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

I looked over the backends and this shouldn't cause any issues.
Could you add some lines to the tests/in/image.wgsl file to test for those overloads?

Thanks!

@teoxoy
Copy link
Member

teoxoy commented Dec 19, 2022

This PR would in theory allow textureLoad to also accept u32 coordinates. Could you also add a test for that?
Also, make sure you push the changed test snapshots (that's why CI is failing).

@teoxoy
Copy link
Member

teoxoy commented Dec 22, 2022

Looks like we need to check for component_kind == crate::ScalarKind::Uint and convert array_index_i32_id to a uint (similar to how we do for the else branch below).

naga/src/back/spv/image.rs

Lines 317 to 336 in 8e1b052

let array_index_i32_id = self.cached[array_index];
let reconciled_array_index_id = if component_kind == crate::ScalarKind::Sint {
array_index_i32_id
} else {
let component_type_id = self.get_type_id(LookupType::Local(LocalType::Value {
vector_size: None,
kind: component_kind,
width: 4,
pointer_space: None,
}));
let reconciled_id = self.gen_id();
block.body.push(Instruction::unary(
spirv::Op::ConvertUToF,
component_type_id,
reconciled_id,
array_index_i32_id,
));
reconciled_id
};

@PENGUINLIONG
Copy link
Contributor Author

ah thanks for the hint ;)

@PENGUINLIONG
Copy link
Contributor Author

wow thats funny. i thought the glsl back is not affected.

@PENGUINLIONG
Copy link
Contributor Author

Oof. Can I rerun the DXC test?

src/back/glsl/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks!

@teoxoy teoxoy changed the title Allow u32 textureStore coordinates Allow u32 coordinates for textureStore/textureLoad Dec 22, 2022
@teoxoy teoxoy merged commit 24316fd into gfx-rs:master Dec 22, 2022
@PENGUINLIONG PENGUINLIONG deleted the patch-1 branch December 23, 2022 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants