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

Code using AccelerationStructureInstanceKHR does not compile #56

Closed
ashpil opened this issue Oct 5, 2022 · 4 comments
Closed

Code using AccelerationStructureInstanceKHR does not compile #56

ashpil opened this issue Oct 5, 2022 · 4 comments

Comments

@ashpil
Copy link
Contributor

ashpil commented Oct 5, 2022

vk.zig:4902:5: error: packed structs cannot contain fields of type 'vk.TransformMatrixKHR'
    transform: TransformMatrixKHR,
vk.zig:4902:5: note: only packed structs layout are allowed in packed types
vk.zig:4897:39: note: struct declared here
pub const TransformMatrixKHR = extern struct {
                               ~~~~~~~^~~~~~
referenced by:
    AccelerationStructureInstanceKHR: vk.zig:4901:53

The issue appears to be that AccelerationStructureInstanceKHR is defined to be a packed struct, but it contains TransformMatrixKHR -- an extern struct, and doing so is now illegal in Zig. Not sure what the solution is here -- presumably, the only thing that actually works is to ensure all structs in packed structs are packed as well, but not sure if we can actually rely on this behaving as expected, as that would require replacing extern with packed in some scenarios, which the C ABI may not like.

Though a cursory glance makes me think only TransformMatrixKHR and SRTDataNV would need to undergo the extern -> packed transition, so at least for now it should be fine.

Also, since packed structs cannot be in extern structs, there's a couple places where that would need to be amended as well -- just the simple replacement solution as above seems even sketchier here. There are more scenarios affected by this.

@ashpil
Copy link
Contributor Author

ashpil commented Oct 5, 2022

Also, unrelated, but this not being noticed earlier makes me think this project should have one of those things some Zig projects have where it just references every struct to ensure it typechecks (though this specific issue seems to need an initialization attempt to typecheck, not just a reference).

@Snektron
Copy link
Owner

Snektron commented Oct 5, 2022

Ah yes, this type has given some trouble before. It used to work before the packed struct overhaul, I think.

Also, unrelated, but this not being noticed earlier makes me think this project should have one of those things some Zig projects have where it just references every struct to ensure it typechecks (though this specific issue seems to need an initialization attempt to typecheck, not just a reference).

Yes, thats a good idea

@Snektron
Copy link
Owner

Snektron commented Oct 6, 2022

Added this refAllDecls-like test also (even more elaborate than the STD one): https://github.com/Snektron/vulkan-zig/blob/master/test/ref_all_decls.zig

Of course it uncovered a bunch of issues, those are also all fixed now.

@ashpil
Copy link
Contributor Author

ashpil commented Oct 7, 2022

Thanks! Looks like vulkan-zig received a lot of improvements today! 😄

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

No branches or pull requests

2 participants