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

emit error if ordering of fields is not optimal #11

Open
benmkw opened this issue Oct 18, 2020 · 8 comments
Open

emit error if ordering of fields is not optimal #11

benmkw opened this issue Oct 18, 2020 · 8 comments

Comments

@benmkw
Copy link

benmkw commented Oct 18, 2020

I don’t have the answer at hand but one could check if the sizes of the members (recursively for structs) are in descending or ascending order or else emit an error.

If I remember correctly if padding is not reused then ascending and descending order are both optimal.

This would make sure one does not waste space.
Maybe there are use cases where one wants that so it probably needs to be optional.

@LPGhatguy
Copy link
Owner

I'm not sure that we want to error or even emit a warning in this case. Users might not always control the order of fields in uniforms, or they might be optimizing for something other than size!

Maybe as an optional attribute this could be useful, but I think that making that information usable could be difficult. It almost feels like something that we'd want as a custom linter rule.

@benmkw
Copy link
Author

benmkw commented Oct 18, 2020

I've not heard about custom lint rules, how would that work?

An optional attribute sounds good as this might also not introduce more complexity into the existing ones.
Providing good error messages might be an issue but at least the user would be aware that there might be an issue.

@LPGhatguy
Copy link
Owner

I don't actually know if something like cargo clippy has any sort of custom lint rules, actually! 😅 I mostly mean that this feels like something that would best be best done as a custom linter rule. That would provide the ability for users to opt in and out of the feature, configure whether it's a warning or an error, etc.

Maybe another approach would be to build something akin to my type-layout crate but for Crevice-generated structs, since type-layout as-is isn't very useful on them. I think this would be useful in two ways:

  • Asserting that generated structs are optimal, because there would be runtime information describing their layout
  • Asserting that generated structs match your shaders! Extra compile-time validation here might be the more compelling reason to build something like this.

@benmkw
Copy link
Author

benmkw commented Oct 18, 2020

because there would be runtime information describing their layout

I can't quite follow because what I thought of was just a compile time check which would not have runtime overhead (which some might not like)

Asserting that generated structs match your shaders!

That sounds very nice but we'd need parsers for GLSL and possibly others. Naga https://github.com/gfx-rs/naga would probably make that possible but thats still very much wip from what I gathered.

@LPGhatguy
Copy link
Owner

I can't quite follow because what I thought of was just a compile time check which would not have runtime overhead (which some might not like)

It would be used as part of writing a test, not necessarily during the primary runtime of your program. If it ends up being const, it could even be run at compile time!

That sounds very nice but we'd need parsers for GLSL and possibly others. Naga https://github.com/gfx-rs/naga would probably make that possible but thats still very much wip from what I gathered.

We can also use SPIR-V reflection for people using Vulkan and wgpu. To be clear, this is a stretch goal to me, but something that I've always wanted to do! 😄

@benmkw
Copy link
Author

benmkw commented Feb 18, 2021

another nice to have might be a way to say "assert_std140/ assert_std430" as an annotation and get an error if the struct is not structured correctly. This might be nice in cases where a user wants to catch errors and control layout more tightly/ avoid copies.

@LPGhatguy
Copy link
Owner

That's another great idea! It would be very cool if we could detect that a struct is packed correctly, but I think that at proc macro evaluation time we aren't able to know yet. I think we would need to know before runtime in order to avoid a copy.

Maybe we could introduce a different derive macro, like being able to derive Std140 directly.

@StephanvanSchaik
Copy link

I don’t have the answer at hand but one could check if the sizes of the members (recursively for structs) are in descending or ascending order or else emit an error.

If I remember correctly if padding is not reused then ascending and descending order are both optimal.

This would make sure one does not waste space.
Maybe there are use cases where one wants that so it probably needs to be optional.

Just making sure to mention that while sorting by descending sizes works in the general case, this is not optimal for something like std140, where if you are using vec3 and floats, for instance, you want them to be ordered like:

struct Foo {
    vec3 x;
    float u;
    vec3 y;
    float v;
};

instead of:

struct Foo {
    vec3 x;
    vec3 y;
    float u;
    float v;
};

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

3 participants