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

As{Std140, Std430} for arrays of As{Std140, Std430} #27

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

ElectronicRU
Copy link
Contributor

Working arrays in crevice, can you believe it?!

Quite a bit of black magic with newtypes and additional traits, but overall I quite like the final design.

@ElectronicRU
Copy link
Contributor Author

Force-pushed to fix some unintentional formatting changes.

@mockersf
Copy link

Any news on this? I'm guessing CI failed on 1.46 as this is using const generics which was stabilised in 1.51. MSRV is now 1.52 so it should work?

@LPGhatguy
Copy link
Owner

Hey, I ended up rewriting a huge chunk of the library. We might be able to rework a lot of this, but will likely have to start over.

I'm sorry about that! It fixed a lot of bugs and may make this easier.

Quite a bit of black magic with newtypes and additional traits,
but overall I quite like the final design.
@ElectronicRU
Copy link
Contributor Author

Rebased, gonna add GLSL support soon (tm).

@ElectronicRU
Copy link
Contributor Author

Oh - given the rewrite, I should be able to strip a LOT of stuff off, actually.

@ElectronicRU
Copy link
Contributor Author

Not really, as it turns out - still necessary for vec3s :P

@ElectronicRU
Copy link
Contributor Author

Arrays done-zo! unfortunately wgpu validation segfaults for me for some reason (faulty driver probably)

Complex arrays are not supported by standard GLSL, and vec3 strides
test is failing.
@ElectronicRU
Copy link
Contributor Author

@LPGhatguy I cannot debug locally, unfortunately - lend me a hand and dig into wgpu validation failing?

@ElectronicRU
Copy link
Contributor Author

CI blocked by gfx-rs/naga#1504. We can decide to proceed with this PR (sans maybe the debug commit :) anyway.

@ElectronicRU
Copy link
Contributor Author

@LPGhatguy the base issue is fixed, I think we can merge now.

Copy link
Owner

@LPGhatguy LPGhatguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thank you for the PR (and poke on Discord today to give it a proper look). I left a few comments with specific feedback.

It's cool that arrays are possible on stable Rust. I don't think I'm comfortable merging the feature with its current complexity, however. Crevice is already close to its ceiling of how complicated the code can be while still being possible to reason through. I have to weigh the impact of a feature versus the maintenance burden it imposes.

I put off reviewing this PR for a long time, I'm sorry about that. I've been thinking for a long time about how to phrase my feedback. If we can cut down on the number of traits, types, and interesting attributes that we need to implement arrays, we should!

If it isn't possible to implement arrays using stable Rust features without causing rippling complexity through the codebase, we should look into unstable features. What's the minimum const evaluation feature that makes arrays in Crevice very simple? What can we do to help push for stabilization? Are there any tricks we can apply from there, working backwards, to something that works on stable Rust?

If all else fails, I'd be comfortable shipping a "nightly" feature using those unstable const evaluation features for consumers who want to opt in and get arrays.

We can also see what other minimum implementations of arrays might be Good Enough for a lot of users as well. For example, it's trivial to implement WriteStd140 for all array types. Is that enough for most consumers?

Definitely looking for feedback and input from other contributors here, too.

src/glsl.rs Outdated Show resolved Hide resolved
src/std140/traits.rs Outdated Show resolved Hide resolved
src/std140/traits.rs Outdated Show resolved Hide resolved
src/std140/dynamic_uniform.rs Outdated Show resolved Hide resolved
src/std140/traits.rs Outdated Show resolved Hide resolved
src/std140/traits.rs Outdated Show resolved Hide resolved
src/std140/traits.rs Outdated Show resolved Hide resolved
src/glsl.rs Outdated Show resolved Hide resolved
src/std140/primitives.rs Outdated Show resolved Hide resolved
@ElectronicRU
Copy link
Contributor Author

Need to change CI to work on nightly somehow, but there ya go.
List of changes:

  • Segregated arrays into their own thing as much as possible. Unfortunately the tasty fruit of blanket-implementing the padding types is plagued by compiler crashes, which change shape at the slightest cough (I've counted about 4 distinct crash reasons!) and so I've been unable to minify them. Same reason dynamic uniforms don't currently implement arrays, but hey, there's Writer.
  • GLSL redone from a scary compile-time linked list to a (scarier?) compile-time string-concatenation. There are some tradeoffs, for sure.
  • Generally tried to hide implementation details like Std140Convertible, but the Padding type kinda leaks out because, see compiler bug.

At this point, we should probably squash that, I won't be rebasing :)

TODO: write some docs, fix CI.
@LPGhatguy

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 this pull request may close these issues.

None yet

3 participants