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

Reserved Keywords collision prevention #58

Open
thargy opened this issue May 11, 2018 · 1 comment
Open

Reserved Keywords collision prevention #58

thargy opened this issue May 11, 2018 · 1 comment

Comments

@thargy
Copy link
Contributor

thargy commented May 11, 2018

Finally got round to trying ShaderGen and Veldrid out as I like the idea of the project. I am something of a novice when it comes to writing shaders, but I thought I'd give it a go. I got everything set up (not entirely plug and play and I might raise a few suggestions for improving new user experience, but I guess the projects not at that stage yet).

Being a novice, I called my Vertex Shader method, VertexShader. Everything compiled, but a closer look at the build output revealed the following (note it didn't fail the build):

------ Rebuild All started: Project: Apocraphy, Configuration: Debug Any CPU ------
Failed to compile HLSL: StdOut: Microsoft (R) Direct3D Shader Compiler 10.1
Copyright (C) 2013 Microsoft. All rights reserved.

, StdErr: C:\Users\Craig.Dean\OneDrive - Web Applications UK Ltd\Sandboxes\Apocraphy\Apocraphy\obj\Shaders\Simple-vertex.hlsl(13,40-51): error X3000: syntax error: unexpected token 'VertexShader'

compilation failed; no code produced
.
Apocraphy -> C:\Users\Craig.Dean\OneDrive - Web Applications UK Ltd\Sandboxes\Apocraphy\Apocraphy\bin\Debug\netcoreapp2.0\Apocraphy.dll
========== Rebuild All: 1 succeeded, 0 failed, 0 skipped ==========

And, on inspection, the Shaders folder did not contain a hlsl.bytes file for the vertex shader.

After some Google-Fu I finally figured out that this was because VertexShader is a reserved keyword in hlsl (see HLSL Keywords). It's not obvious from the error, but that's not your fault. Again - I'm a bit of a novice when it comes to shaders.

The first question is, should the final HLSL compilation failure not ideally fail the build (I note an option in the ShaderGen targets that for the build to fail when ShaderGen itself fails, should we have a similar option or respect that option for HLSL compilation failures)?

Moreover, one of the benefits of ShaderGen is the ability it will give to perform more useful static analysis. It seems like it would be relatively easy to add reserved keyword collision checking for the various languages and to auto-rename colliding tokens in the generated output, or error. At the moment the code seems to assume that valid C# identifiers are also valid in the other languages, and that is not the case according to the various specifications I've quickly reviewed (opengl has reservations too).

Thanks for a great project.

@mellinoe
Copy link
Owner

This is one of the many rough edges of the library that gets ignored by me because it only pops up occasionally, and I just work around it every time. There's at least one place where reserved keywords are already handled, but there's no central place where it's done. For example, this part corrects a couple of GLSL keywords. Ideally, each of the backends would just have a single list of all keywords, and there's a central place where identifiers are corrected based on those lists.

The first question is, should the final HLSL compilation failure not ideally fail the build (I note an option in the ShaderGen targets that for the build to fail when ShaderGen itself fails, should we have a similar option or respect that option for HLSL compilation failures)?

Ideally, I would like the current behavior to stay. The tool will already quit out if it thinks it can't process the shader correctly. If it actually manages to spit out HLSL/GLSL, etc. then we've gotten far enough that the shader you've written (in C#) should be valid, and any failure to compile the HLSL/GLSL is a bug in ShaderGen.

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