Suggestion: Drop ShaderSet assembly attribute about shadergen HOT 4 OPEN

By way of an addendum, but keeping separate as I'm probably really missing something.

If you limited a class to having one Fragment and VertexShader, then you could infer the shader set name as the class name. This would allow you to use nameof in the code to refer to a shader set, which would make the whole system refactor-proof.

Note, you should be able to use nameof in the existing ShaderSet attribute, to gain a similar robustness without changing anything, e.g.

[assembly: ShaderSet(nameof(Simple), nameof(Simple.VS), nameof(Simple.FS))]

However, I get the following error:
ShaderGen.ShaderGenerationException: No type was found with the name nameof(Simple
(note missing close bracket)

So it appears nameof is not currently implemented in the code.

If the above did work, I'd recommend changing your examples to use nameof.

from shadergen.

Please forgive me if I am missing something?

No, I don't think you have! This is one of the "clunkier" parts of the library, and while it works okay for now, it could certainly be simpler and more user friendly.

Rather than have the assembly attribute, would it be better to make the VertexShaderAttribute and FragmentShaderAttribute attributes accept a params string[] shaderSets parameter?

That sounds like a good option to me. I don't particularly like the assembly-level attributes.

If you limited a class to having one Fragment and VertexShader, then you could infer the shader set name as the class name. This would allow you to use nameof in the code to refer to a shader set, which would make the whole system refactor-proof.

We could probably allow both -- e.g. if you don't provide a set name, then we'll accept a class that has a single [VertexShader] and a single [FragmentShader]. Using nameof for full type names usually gets a little bit hairy, though, because you need to chain together lots of nameofs together. But it would be easier to refactor than a full string.

So it appears nameof is not currently implemented in the code.

Indeed. I also think that const string values cannot be used for a shader set name. We should allow anything that is legal C# to be used as the name, if it's specified.

from shadergen.

We could probably allow both -- e.g. if you don't provide a set name, then we'll accept a class that has a single [VertexShader] and a single [FragmentShader]. Using nameof for full type names usually gets a little bit hairy, though, because you need to chain together lots of nameofs together. But it would be easier to refactor than a full string.

Yes, that would be a much cleaner solution.

To specify fully qualified member names in an attribute it's normally easier to take a type and a member name e.g. ShaderSet(..., typeof(MyShader), nameof(MyShader.VS), ...) which avoids string mess.

I looked into the nameof support in attributes, and added a test case. I noticed the method that currently fails to parse them correctly is ShaderSetDiscoverer.GetStringParam. If you pass the Compilation to the SyntaxWalker you can use the SematicModel.GetConstantValue() method from the model (which you can get using the Compilation) to correctly parse 'nameof' expressions, as well as strings expressions. I started to do the work but sadly ran out of time and have to leave for an event. 'nameof' is a bit of a special case in Roslyn apparently and you need to deal with it explicitly, unfortunately that would mean refactoring quite a few bits where you currently expect a string expression. I looked at adding an extension to Utility, but I'm relatively new to Roslyn and wouldn't be confident of catching all the edge cases.

from shadergen.

Just in case anyone stumbling across this is still interested:
I've just implemented something similar for a personal project: Saalvage/ShaderGen@b7e4059...9653c25

nameof and typeof are supported everywhere, but instead of specifying shader sets via attributes on the Vertex/Fragment-Shader attributes I've decided to introduce ShaderClass as an attribute on the classes containing a shader set. You can either specify the shader methods manually or if there's just a single set it can be automatically inferred.
This reflects the current common praxis of encapsulating a single shader set within a single class as well as continuing to provide a "central" location where every shader set is defined, which I consider to be desirable.
If you want to spread your shaders across multiple classes you'll still have to rely on the ol' ShaderSet attribute, but I personally think that is preferable to having the definition of what any one shader set is potentially spread across multiple files.

I've also decided to keep the "legacy" ShaderSet attribute constructors (the ones not taking a Type) for backwards compatibility for now (marking them with Obsolete).

If this (awesome) project ever becomes active again I'd happily look into writing up a PR for these changes, as well as other improvements I've made. :)

from shadergen.