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

Update naga_oil to make use of naga frontend translation units. #107

Closed
DexterHill0 opened this issue Sep 13, 2024 · 16 comments
Closed

Update naga_oil to make use of naga frontend translation units. #107

DexterHill0 opened this issue Sep 13, 2024 · 16 comments

Comments

@DexterHill0
Copy link

After using naga_oil myself and running into some issues, I think it would really benefit having access to naga internals. I did search for this feature request within WGPU, but I originally did not find any other issues, so I opened my own (gfx-rs/wgpu#6250). It turns out there is also a similar issue at gfx-rs/wgpu#5713, and a more fitting issue gfx-rs/wgpu#6132. In both my issue and 6132, the green light was given to make a PR to expose some of the naga frontend translation units for use in other projects. I realised in mine it would make more sense to first try and update naga_oil to use naga before making the PR so there is a clear list of features that need to be publicized.

I know this idea has been floating around some of this issues in this repo, is this definitely something that would still want to be implemented here? Although I couldn't start experimenting right now, I would be open to contributing to this change in the near future.

@DexterHill0 DexterHill0 changed the title Update naga_oil to make use of naga internals. Update naga_oil to make use of naga frontend translation units. Sep 13, 2024
@robtfm
Copy link
Collaborator

robtfm commented Sep 13, 2024

Yes definitely. @stefnotch has made some efforts in this direction already, though I’m not sure how far they’ve progressed.

@stefnotch
Copy link
Contributor

The latest status on this is that we've started work on multiple additions to WGSL (imports, conditional compilation, IDE support, ...)

https://github.com/wgsl-tooling-wg/wesl-spec

We have a Discord server at https://discord.com/invite/Ng5FWmHuSv and would love to have you there! Please let us know what things you need, it'd be awesome if we can help with solving those problems.

@DexterHill0
Copy link
Author

Very cool, I will take a look at the server and spec soon!

However, (unless the info is on the server in which case let me know) what's the future plans for this crate? Are you going to go back to implementing it with naga? Are you going to make/use a different wgsl parser based on the custom spec? Wait for the spec to be officially added?

I think naga_oil would really benefit from naga and I'm happy to work on that if you wanted to focus on the spec or something else.

@stefnotch
Copy link
Contributor

There are quite a few advanced features, like generics, that we're considering. Since parsing those with naga won't be practical, we will likely end up building a parser for a superset of WGSL.

As such, eventually naga_oil will be superseded by something newer. The big question is how long that'll take. So, if this sounds interesting to you, do join us and help us build a better WGSL for Bevy and co!

@DexterHill0
Copy link
Author

That makes sense. By the sounds of it though it doesn't seem like it will be very quick and in the project I'm working on I'd love to use naga_oil for its features but the current implementation wasn't working well for me. As a stepping stone between now and the future where this custom superset of wgsl is available, is it sensible to still update naga_oil?

@ncthbrt
Copy link

ncthbrt commented Sep 15, 2024

@DexterHill0 I think it'd make sense to update naga oil as a stepping stone. However first the naga translation units need to be exposed in the first place.

@ncthbrt
Copy link

ncthbrt commented Sep 15, 2024

Oh also, would be helpful to get a sense of your timelines, because this is assuming you want to make changes to naga oil yesterday...

@DexterHill0
Copy link
Author

@DexterHill0 I think it'd make sense to update naga oil as a stepping stone. However first the naga translation units need to be exposed in the first place.

That was what my original issue was for within naga itself but then I realised I wouldn't know what parts of naga to expose without first implementing the features into naga_oil.

Oh also, would be helpful to get a sense of your timelines, because this is assuming you want to make changes to naga oil yesterday...

Right now I'm 3/4s a way through another project so I wouldn't start work on it however by the end of the month I should have more time to focus on attempting something like this.

@DexterHill0
Copy link
Author

I've begun to make some changes to naga_oil to make use of the naga translation units, as well as naga to expose the required units. These changes are on my fork of naga_oil here and naga here.
I wanted to make a little bit of headway before I posted this because I wasn't sure if it was going to be feasible when I first took a look. When I first viewed the code it was pretty overwhelming and hard to get to grips with so I didn't want to immediately say I would go ahead and make the changes.
Everything has been fine otherwise and my main goal with the changes is to get it to a point where all the tests pass as they did before.

The only major issue so far is that I realised that the lexer I'm using is only for WGSL and so it would exclude GLSL functionality from the crate. GLSL has a very different lexing setup to WGSL which would make it hard to implement both. I don't think it's impossible but it would require some weirdness. How much of an issue would that be?

@robtfm
Copy link
Collaborator

robtfm commented Oct 12, 2024

could you summarize what you're doing? i'm not entirely clear on why using naga internals requires a custom lexer.

it would be nice to maintain glsl even if not fully-featured. but whether we are willing to drop it would depend on the benefits i suppose.

@DexterHill0
Copy link
Author

could you summarize what you're doing? i'm not entirely clear on why using naga internals requires a custom lexer.

it would be nice to maintain glsl even if not fully-featured. but whether we are willing to drop it would depend on the benefits i suppose.

Sorry, now I read it back it does sound like I'm using a custom lexer. What I meant is that I'm just importing the lexer from the WGSL module within naga, and not the one for GLSL. The difference between the way the two languages get parsed is significant so, for now, I just went with using the WGSL lexer.
I can look into implementing GLSL after WGSL but the logic for everything else is much easier to get working with WGSL for now as it's a much simpler lexing setup in naga.

@DexterHill0
Copy link
Author

Going to be honest this is unfortunately beginning to look more and more unfeasable as time goes on. When I originally had this idea to use the lexer I was certain tokens were going to be much better than lines and regexes however at this stage I think lines and regexes ironically are a far more robust solution.

A massive advantage of lines and a huge weakness of the lexer is whitespace. The most major issues left to solve, and the ones that have been "solved", are to do with whitespace and the little flexibility the lexer provides with that. One more simple example is something like:

#import foobar

struct X {}

in the new system this would be considered foobar::struct::X as it's impossible to tell its separated by a line. The WGSL lexer does provide Token::Trivia but it's use is very limited as it not only includes whitespace and newlines, but comments as well.
The current issue I am facing is within Preprocessor::preprocess and how it handles padding the final string with spaces so any errors will match the original source spans correctly. The lexer is not going to allow for this padding and therefore all errors will no longer align with the original source.

There is also the other problem that a lot of this system would have to be duplicated to add the functionality of the GLSL lexer. I will be starting a new job at the end of this month and I don't think I could have it completed by then so I'm not sure where to take it. As much as I want this to work it may be a case of leaving it to a whole new project somewhere within the WESL spec.

@stefnotch
Copy link
Contributor

@DexterHill0 I understand, it is surprisingly tough to get all the existing pieces to line up. I had previously written a whole parser for this, only to run into a few other architectural issues.

As for GLSL, I believe it is ok to not have perfect interoperability. For example, we could require WGSL bindings for GLSL modules.

  1. foo.glsl
  2. Create a foo.wgsl file, which contains stubs for everything
  3. Import from the foo.wgsl file
  4. Once it has all been parsed, we separately parse the foo.glsl file, and then replace the naga modules.

Alternatively, depending on naga's capabilities, one might be able to translate almost arbitrary GLSL code to WGSL code. Then we could simply tell people to automatically translate their existing GLSL code to WGSL.

Would you be interested in helping out with WESL? We're currently focusing on an MVP, which has a lot of tasks where we'd love to get some help wgsl-tooling-wg/wesl-spec#54

  • Spec: There are a few final bits that we need to get across the finish line. We absolutely appreciate feedback at this stage!
  • Documentation: Be it writing documentation, or giving us feedback from a user perspective. We also need to set up an mdbook webpage.
  • 3rd party tools: IDE support would be amazing! There are quite a few WGSL syntax highlighters and language servers out there. Polishing those would be amazing.
  • ...

@DexterHill0
Copy link
Author

@stefnotch I personally think for this project it's better to leave it as is since it would probably be easier to rewrite the whole thing and I think that's better off for WESL to do anyway. I'd definitely be happy to help there and there with WESL.

For helping out with the spec is it just a case of opening an issue in the spec repo or should it go through the Discord first?

@stefnotch
Copy link
Contributor

For helping out with WESL, feel free to directly open issues on GitHub. The Discord is there for times when you want to ask a quick question, or when you think that a quick discussion would be worthwhile.

@DexterHill0
Copy link
Author

Thanks, I've run into a few things that might be nice so I'll definitely make sure to open an issue or two.

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

4 participants