Traverse-Research / rspirv-reflect

🦀 Minimal SPIR-V reflection library.
https://traverseresearch.nl
Apache License 2.0
41 stars 10 forks source link

Add OpTypeArray to get_descriptor_type #16

Closed khyperia closed 3 years ago

khyperia commented 3 years ago

The current code errors with ReflectError::UnhandledTypeInstruction when dealing with an OpTypeArray:

Unhandled OpType instruction Instruction { class: Instruction { opname: "TypeArray", opcode: TypeArray, capabilities: [], operands: [LogicalOperand { kind: IdResult, quantifier: One }, LogicalOperand { kind: IdRef, quantifier: One }, LogicalOperand { kind: IdRef, quantifier: One }] }, result_type: None, result_id: Some(1902), operands: [IdRef(601), IdRef(46)] }

I believe handling arrays the same as runtime arrays is probably the right thing to do, but I don't know for sure!

MarijnS95 commented 3 years ago

Fwiw:

StructuredBuffer<uint> test;

Is a runtime array (have to subscript test to access an individual integer), and:

StructuredBuffer<uint[3]> test;

Is an OpTypeArray of size 3, inside the runtime array.

khyperia commented 3 years ago

Actually, not sure if is_bindless is supposed to be set to true or not.

To be honest, I have no idea what I'm doing, I just ran into this crash when doing completely unrelated stuff, went "oh, this seems like this should be implemented", and doing this fixed the crash and made things work!

MarijnS95 commented 3 years ago

@khyperia I'm 99.99% confident this is correct :)

MarijnS95 commented 3 years ago

Actually, not sure if is_bindless is supposed to be set to true or not.

Ah, now that might cause trouble...

Unfortunately we don't need/use rspirv-reflect anymore because all our code is bindless, so I can't test this. But yeah, I think we should set bindless to false and only set it back to true if it ends up being wrapped.

khyperia commented 3 years ago

I'll make a quick follow up thing, then~

MarijnS95 commented 3 years ago

I'll make a quick follow up thing, then~

Just double-checking:

StructuredBuffer<uint> test[4];

This is also considered bindless afaik (four bindings where each binding can be accessed in a "bindless" manner).

khyperia commented 3 years ago

Ah, then I guess no follow-up? is_bindless is currently set to true for StructuredBuffer<uint> test[4]; - it's kind of wishy washy, yeah, not sure what is_bindless actually means/is used for

edit: or, wait, uh, I'm confused at the difference between StructuredBuffer<uint> test[4]; and StructuredBuffer<uint[4]> test;. I'll... just let someone who knows what they're doing do any potential follow-up, this fixes the case that was crashing us, so I'm happy with just this :)

MarijnS95 commented 3 years ago

Ah, then I guess no follow-up? is_bindless is currently set to true for StructuredBuffer<uint> test[4]; - it's kind of wishy washy, yeah, not sure what is_bindless actually means/is used for

Ah, ehh, it's good that I added some doc-comments to DescriptorInfo when making this... ehh... wait... :confused:

edit: or, wait, uh, I'm confused at the difference between StructuredBuffer<uint> test[4]; and StructuredBuffer<uint[4]> test;

The former should be an array of four bindings, each holding a variable number of uints. The latter is a variable number of uint[4]s, analogous to a uint4 vector.

MarijnS95 commented 3 years ago

I'll... just let someone who knows what they're doing do any potential follow-up, this fixes the case that was crashing us, so I'm happy with just this :)

I think I understand now after reading the tests, will follow-up before we do a release :)