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

Add support for DescriptorType::COMBINED_IMAGE_SAMPLER #26

Closed
dffdff2423 opened this issue Jun 16, 2022 · 4 comments · Fixed by #27
Closed

Add support for DescriptorType::COMBINED_IMAGE_SAMPLER #26

dffdff2423 opened this issue Jun 16, 2022 · 4 comments · Fixed by #27

Comments

@dffdff2423
Copy link

Shader (fragment):

#version 450

layout (location = 0) in vec3 color;
layout (location = 1) in vec2 tex_cord;

layout (location = 0) out vec4 out_color;

layout (set = 0, binding = 0) uniform sampler2D tex;

void main()
{
	out_color = mix(texture(tex, tex_cord), vec4(color, 1.f), 0.2f);
}

Relevant part of the backtrace:

thread 'main' panicked at 'not yet implemented: Instruction { opname: "TypeSampledImage", opcode: TypeSampledImage, capabilities: [], extensions: [], operands: [LogicalOperand { kind: IdResult, quantifier: One }, LogicalOperand { kind: IdRef, quantifier: One }] } Not implemented; untested', /home/$username/.cargo/registry/src/gh.neting.cc-1ecc6299db9ec823/rspirv-reflect-0.7.0/src/lib.rs:333:17
stack backtrace:
   0: rust_begin_unwind
             at /rustc/420c970cb1edccbf60ff2aeb51ca01e2300b09ef/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/420c970cb1edccbf60ff2aeb51ca01e2300b09ef/library/core/src/panicking.rs:142:14
   2: rspirv_reflect::Reflection::get_descriptor_type
             at /home/$username/.cargo/registry/src/gh.neting.cc-1ecc6299db9ec823/rspirv-reflect-0.7.0/src/lib.rs:333:17
   3: rspirv_reflect::Reflection::get_descriptor_type_for_var
             at /home/$username/.cargo/registry/src/gh.neting.cc-1ecc6299db9ec823/rspirv-reflect-0.7.0/src/lib.rs:241:9
   4: rspirv_reflect::Reflection::get_descriptor_type
             at /home/$username/.cargo/registry/src/gh.neting.cc-1ecc6299db9ec823/rspirv-reflect-0.7.0/src/lib.rs:292:24
   5: rspirv_reflect::Reflection::get_descriptor_type_for_var
             at /home/$username/.cargo/registry/src/gh.neting.cc-1ecc6299db9ec823/rspirv-reflect-0.7.0/src/lib.rs:241:9
   6: rspirv_reflect::Reflection::get_descriptor_sets
             at /home/$username/.cargo/registry/src/gh.neting.cc-1ecc6299db9ec823/rspirv-reflect-0.7.0/src/lib.rs:485:21

It looks like the code is already written but is just commented out:
https://github.com/Traverse-Research/rspirv-reflect/blob/main/src/lib.rs#L335

@MarijnS95
Copy link
Member

Hi, thanks for reporting this! As you can see the message reads Not implemented; untested, I've simply never hit this bit of code to properly test it. I vaguely remember not trusting the dim match here, but if you have a use-case for it and can test+confirm that would be great to confirm 👍!

rspirv-reflect/src/lib.rs

Lines 333 to 343 in d968ff4

todo!("{:?} Not implemented; untested", type_instruction.class);
// Note that `dim`, `sampled` and `storage` are parsed from TypeImage
// if dim == SpvDimBuffer {
// if sampled {
// DescriptorType::UNIFORM_TEXEL_BUFFER
// } else if storage {
// DescriptorType::STORAGE_TEXEL_BUFFER
// }
// } else {
// DescriptorType::COMBINED_IMAGE_SAMPLER
// }

@MarijnS95
Copy link
Member

Looks like I actually handled this a bit different in #5: 3beb355.

@dffdff2423 dffdff2423 changed the title Panics with not yet implemented Add support for DescriptorType::COMBINED_IMAGE_SAMPLER Jun 17, 2022
@chyyran
Copy link

chyyran commented Oct 24, 2022

Hi, any updates on this issue?

@MarijnS95
Copy link
Member

@chyyran I haven't had much feedback on #27 but think I'll just merge it based on the ack from @dffdff2423.

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 a pull request may close this issue.

3 participants