vulkano-shaders: support glam as a linear algebra backend
#2479
Conversation
|
I'm afraid that this is unsound. There's a reason vulkano-shaders still can't generate glam types even though I would like it to. Namely, unlike cgmath and nalgebra, glam has overaligned types ( ExamplesOveraligned array elements#version 460
#extension GL_EXT_scalar_block_layout : enable
struct Example {
vec4 v;
float x;
};
layout(binding = 0, scalar) buffer Examples {
Example examples[];
};
void main() {}The above GLSL compiles to the following SPIR-V: The important line being: On the other hand, if you naively generate this Rust code: #[repr(C)]
struct Example {
v: glam::Vec4,
x: f32,
}
#[repr(C)]
struct Examples {
examples: [Example],
}Then Overaligned struct offsets#version 460
#extension GL_EXT_scalar_block_layout : enable
struct Example1 {
vec4 v;
};
struct Example2 {
Example1 example1;
};
layout(binding = 0, scalar) buffer Examples {
float x;
Example2 example2;
};
void main() {}The above GLSL compiles to the following SPIR-V: The important line being: And if you naively generate this Rust code: #[repr(C)]
struct Example1 {
v: glam::Vec4,
}
#[repr(C)]
struct Example2 {
example1: Example1,
}
#[repr(C)]
struct Examples {
x: f32,
example2: Example2,
}Then SolutionsAs you can probably tell from the above examples, a SPIR-V struct itself doesn't tell us anything about alignment of it or its fields, only at which offsets the fields are. It could therefore happen, as can be seen in the first example, that the All in all, if you want to implement this safely, it will essentially require a rewrite of the structs reflection. You will need to keep track of every struct that is encountered so that you can recursively check all other structs it is contained within to determine its alignment. It can also happen that the same struct is used in two other structs, which cause it to have a different alignement for each. |
|
Thanks for your in-depth explanation @marc0246. Indeed, the problem is much deeper than I naively expected and I managed to miss the Being completely honest, I don't have the amount of free, uninterrupted time to tackle this myself, yet I want to keep your message as a cautionary tale to whoever finds themselves in the same situation. I'll let you decide what to do with the PR and the issue (either keeping them as-is or closing them) |
|
The issue should stay open and this PR closed if you don't feel like implementing it, which is completely understandable. |
Fixes #2478.
Things to be considered:
glamprovidesVec3AandMat3Atypes which currently are not handled.Make
matcharms more readable.Rethink cases marked as
unreachable!().Update documentation to reflect any user-facing changes - in this repository.
Make sure that the changes are covered by unit-tests.
Run
cargo clippyon the changes.Run
cargo +nightly fmton the changes.Please put changelog entries in the description of this Pull Request
if knowledge of this change could be valuable to users. No need to put the
entries to the changelog directly, they will be transferred to the changelog
file by maintainers right after the Pull Request merge.
Please remove any items from the template below that are not applicable.
Describe in common words what is the purpose of this change, related
Github Issues, and highlight important implementation aspects.
Changelog: