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

[thread] Unable to parse Bevy's ".vert" files #210

Closed
enfipy opened this issue Sep 20, 2020 · 31 comments · Fixed by #898
Closed

[thread] Unable to parse Bevy's ".vert" files #210

enfipy opened this issue Sep 20, 2020 · 31 comments · Fixed by #898
Labels
area: front-end Input formats for conversion kind: bug Something isn't working lang: GLSL OpenGL Shading Language

Comments

@enfipy
Copy link

enfipy commented Sep 20, 2020

I faced with the following issue: ParseError { kind: UnknownVariable(TokenMetadata { line: 19, chars: 16..21 }, "Model") } when tried to parse .vert file.

I noticed this commented test and now curious - what happened? It seems like it worked before but now - it's not.

How to fix it?

@enfipy enfipy changed the title Vertex failed to parse Unable to parse Vertex files Sep 20, 2020
@kvark
Copy link
Member

kvark commented Sep 20, 2020

GLSL front-end is a work in progress. We actually had 2 different GLSL front-ends, and we decided to remove the older one and proceed with newer one, which was less feature complete. Missing features in it are expected. Please provide the concrete shader so that @pjoe or other folks can make necessary fixes and additions.

@kvark kvark added area: front-end Input formats for conversion kind: bug Something isn't working lang: GLSL OpenGL Shading Language labels Sep 20, 2020
@enfipy
Copy link
Author

enfipy commented Sep 20, 2020

@kvark Thank you for the quick reply!

Here is the GLSL Vertex code:

#version 450

layout(location = 0) in vec3 Vertex_Position;
layout(location = 1) in vec3 Vertex_Normal;
layout(location = 2) in vec2 Vertex_Uv;

layout(location = 0) out vec3 v_Position;
layout(location = 1) out vec3 v_Normal;
layout(location = 2) out vec2 v_Uv;

layout(set = 0, binding = 0) uniform Camera {
    mat4 ViewProj;
};

layout(set = 2, binding = 0) uniform Transform {
    mat4 Model;
};

void main() {
    v_Normal = (Model * vec4(Vertex_Normal, 1.0)).xyz;
    v_Normal = mat3(Model) * Vertex_Normal;
    v_Position = (Model * vec4(Vertex_Position, 1.0)).xyz;
    v_Uv = Vertex_Uv;
    gl_Position = ViewProj * vec4(v_Position, 1.0);
}

This is from bevy repository. I think there are some more examples of simple vertex shaders that also don't work.

@pjoe
Copy link
Collaborator

pjoe commented Sep 21, 2020

I think the specific issue here is that the uniforms are without a named instance - which I haven't implemented yet. Maybe can change the title to include this info.

@pjoe
Copy link
Collaborator

pjoe commented Sep 21, 2020

Looking some more at the shader, swizzles like .xyz are also missing - need to figure out how to handle those

@kvark kvark changed the title Unable to parse Vertex files Unable to parse Bevy's ".vert" files Sep 21, 2020
pjoe added a commit to pjoe/naga that referenced this issue Sep 21, 2020
@kvark kvark closed this as completed in e5d17e8 Sep 22, 2020
@pjoe
Copy link
Collaborator

pjoe commented Sep 23, 2020

I think this got closed mistakenly, by me writing partly fixes #210 in a commit msg. I don't think GitHub understood the partly. Feel free to re-open, or file a more specific issue, e.g. about swizzles.

@kvark kvark reopened this Sep 23, 2020
@enfipy
Copy link
Author

enfipy commented Sep 24, 2020

@pjoe It looks like a new error after your changes: ParseError { kind: SemanticError("Can\'t lookup field on this type") }.

Maybe this is a dumb question but why there are no options in naga for spir-v? It looks like bevy sending some "STANDARDMATERIAL_SHADED" option that was used as CompileOptions for shaderc.

@pjoe
Copy link
Collaborator

pjoe commented Sep 24, 2020

@enfipy that error is because of missing swizzle support, working on it - stay tuned :)

about spirv-out, better check with @Napokue

pjoe added a commit to pjoe/naga that referenced this issue Sep 24, 2020
@kvark
Copy link
Member

kvark commented Sep 24, 2020

@enfipy what is STANDARDMATERIAL_SHADED, and how does it relate to SPIR-V?

@enfipy
Copy link
Author

enfipy commented Sep 25, 2020

@kvark I'm not sure about it but I think it's more for .frag - like here (it looks like vars passed to files). And here is the struct with 2 more props. As I understood naga::front::glsl::parse_str can parse 3 shader stages at once - so it's maybe needless for ".vert" or ".comp".

@pjoe
Copy link
Collaborator

pjoe commented Sep 25, 2020

Maybe I don't understand, but looks like normal pre-processor defines to me?

FYI: pre-processor is not yet working in new glsl-in

@enfipy
Copy link
Author

enfipy commented Sep 25, 2020

@pjoe I think yes. Is it hard/time consuming to add preprocessor support with pomelo?

@pjoe
Copy link
Collaborator

pjoe commented Sep 25, 2020

I'd prefer to have it as a separate step in front of the parser (maybe combined with #include support).

There was 1 or 2 pre-processor implementations suggested already (one is from old glsl-in).

Anyway needs some investigation to figure out how best to do.

kvark pushed a commit that referenced this issue Sep 25, 2020
* [glsl-in] Implement swizzle for r-values

Related to #210

* [glsl-in] Just return Result from field_selection

Removed unneccessary Otion in return type

* [glsl-in] Always match on type in field_selection

* [glsl.in] Borrow by value in field_selection
@pjoe
Copy link
Collaborator

pjoe commented Sep 28, 2020

FYI: looked up how shader_defs are handled in bevy through glsl_to_spirv.

They are just passed to glslang as cli params with -D <shader_def>. I think this would be equivalent to prepending the source with #define <shader_def> - though I guess the #version still need to be first

@enfipy
Copy link
Author

enfipy commented Oct 1, 2020

@pjoe Any updates?

@pjoe
Copy link
Collaborator

pjoe commented Oct 2, 2020

It will take a bit to implement pre-processor. Meanwhile you can take a look at #132.

@enfipy
Copy link
Author

enfipy commented Oct 4, 2020

@pjoe There is some weird error on SPV backend. I'm using the latest naga commit.

10-04 14:20:36.639 31339 31409 I RustStdoutStderr: RustStdoutStderr: ####### "#version 450\n\nlayout(location = 0) in vec3 Vertex_Position;\nlayout(location = 1) in vec3 Vertex_Normal;\nlayout(location = 2) in vec2 Vertex_Uv;\n\nlayout(location = 0) out vec3 v_Position;\nlayout(location = 1) out vec3 v_Normal;\nlayout(location = 2) out vec2 v_Uv;\n\nlayout(set = 0, binding = 0) uniform Camera {\n    mat4 ViewProj;\n};\n\nlayout(set = 2, binding = 0) uniform Transform {\n    mat4 Model;\n};\n\nvoid main() {\n    v_Normal = (Model * vec4(Vertex_Normal, 1.0)).xyz;\n    v_Normal = mat3(Model) * Vertex_Normal;\n    v_Position = (Model * vec4(Vertex_Position, 1.0)).xyz;\n    v_Uv = Vertex_Uv;\n    gl_Position = ViewProj * vec4(v_Position, 1.0);\n}\n"
10-04 14:20:36.645 31339 31409 I RustStdoutStderr: thread 'Compute Task Pool (0)' panicked at 'called `Option::unwrap()` on a `None` value', /Users/enfipy/.cargo/git/checkouts/naga-dbb2b19faed49210/90c56ac/src/back/spv/writer.rs:675:47

I'm not sure that this error linked to the pre-processor.

@pjoe
Copy link
Collaborator

pjoe commented Oct 4, 2020

Maybe @Napokue would know more about this error.

@pjoe
Copy link
Collaborator

pjoe commented Oct 4, 2020

Dunno, but could be related to #219

@enfipy
Copy link
Author

enfipy commented Oct 4, 2020

I tried this PR from #219 but nothing changed

@Timo-DK
Copy link
Collaborator

Timo-DK commented Oct 4, 2020

I will take a look, you use the shader that you have posted at the top of this issue, right?

@enfipy
Copy link
Author

enfipy commented Oct 4, 2020

@Napokue Yes, and got the error above

@Timo-DK
Copy link
Collaborator

Timo-DK commented Oct 4, 2020

Thank you for this! I have found the issue, and we always try to set OpName when the debug flag is enabled. As uniform variables can be unnamed, this caused the crash.

However, your shader will still not work yet, as AccessIndex is not yet implemented. I have created a PR yesterday to support AccessIndex and Access expresions, see #224. Should be finished today hopefully, and I expect it to be merged into the main branch soon.

I will use your shader as test data for the SPIR-V back-end.

@Timo-DK
Copy link
Collaborator

Timo-DK commented Oct 4, 2020

Question: is Bevy already using Naga for all their shader compilations or for their examples? If so, it will be a great source of test cases for us to test.

@enfipy
Copy link
Author

enfipy commented Oct 5, 2020

@Napokue Currently, Bevy doesn't use Naga because of luck some main features. But as cart said in this PR Bevy wants to migrate to Naga. Also, there're some forks that added Naga to Bevy.

@mrk-its
Copy link

mrk-its commented Oct 6, 2020

Today I was playing with bevy+naga, trying to run simple 2d sprite example. Good news is this vertex shader was parsed:

#version 450

layout(location = 0) in vec3 Vertex_Position;
layout(location = 1) in vec3 Vertex_Normal;
layout(location = 2) in vec2 Vertex_Uv;

layout(location = 0) out vec2 v_Uv;

layout(set = 0, binding = 0) uniform Camera {
    mat4 ViewProj;
};

layout(set = 2, binding = 0) uniform Transform {
    mat4 Model;
};
layout(set = 2, binding = 1) uniform Sprite_size {
    vec2 size;
};

void main() {
    v_Uv = Vertex_Uv;
    vec3 position = Vertex_Position * vec3(size, 1.0);
    gl_Position = ViewProj * Model * vec4(position, 1.0);
}

but if failed on spirv conversion.

Fragment shader failed on parsing - it seems that texture2D and sampler types were not recognized, after commenting it out it failed inside main on texture function call
Fragment shader source:

#version 450

layout(location = 0) in vec2 v_Uv;

layout(location = 0) out vec4 o_Target;

layout(set = 1, binding = 0) uniform ColorMaterial_color {
    vec4 Color;
};

# ifdef COLORMATERIAL_TEXTURE 
layout(set = 1, binding = 1) uniform texture2D ColorMaterial_texture;
layout(set = 1, binding = 2) uniform sampler ColorMaterial_texture_sampler;
# endif

void main() {
    vec4 color = Color;
# ifdef COLORMATERIAL_TEXTURE
    color *= texture(
        sampler2D(ColorMaterial_texture, ColorMaterial_texture_sampler),
        v_Uv);
# endif
    o_Target = color;
}

@kvark kvark changed the title Unable to parse Bevy's ".vert" files [thread] Unable to parse Bevy's ".vert" files Oct 6, 2020
@mrk-its
Copy link

mrk-its commented Oct 7, 2020

Question: is Bevy already using Naga for all their shader compilations or for their examples? If so, it will be a great source of test cases for us to test.

As @enfipy said bevy plans to migrate to naga as soon as possible. In the meantime I'm considering naga for webgl2 rendering backend (and for that I don't need spirv conversion, only parsing / reflection of glsl shaders, especially '#version 300 es')

@pjoe
Copy link
Collaborator

pjoe commented Oct 28, 2020

FWIW: the frag shader now parser, when COLORMATERIAL_TEXTURE is not defined. I'll try taking a look at the texture stuff next.

@pjoe
Copy link
Collaborator

pjoe commented Nov 2, 2020

After #257, the frag shader now also parses with COLORMATERIAL_TEXTURE.

There is however still some issues with outputting spv from the resulting naga IR.

@Timo-DK
Copy link
Collaborator

Timo-DK commented Nov 2, 2020

Yes sorry for my slacking the last couple of weeks, there have been some private matters the last couple of weeks, which led me to having to take some time off from contributing. I think I will be able to pickup the work this week again.

@pjoe
Copy link
Collaborator

pjoe commented Nov 2, 2020

@Napokue: no worries, I also have times where 'life' (day job, family, etc.) gets 'in the way' 😄

@JCapucho
Copy link
Collaborator

I've tested the provided shaders in this issue and with #898 they are all parsed, validated and emitted successfully, so when it lands I'll close this issue.

@JCapucho JCapucho linked a pull request May 23, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: front-end Input formats for conversion kind: bug Something isn't working lang: GLSL OpenGL Shading Language
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants