Allow shader defines to be created with an optional value

[rms7326]

Is your feature request related to a problem? Please describe.
Using shader specialization constants to size global arrays doesn't work on the Mac.

We have a need in our vertex shader to size both a vertex attribute array and an array of gl_clipDistance by a defined value at shader compile time. We were using shader specialization constants to achieve this and on Linux and Windows with NVidia and AMD we seemed to have success even though the "Arrays inside a block" discussion in section "4.4.x Specialization-Constant Qualifier" of ARB_gl_spirv.txt made it sound like using specialization constants to size these array members in the shader is not supported. When we tried out the same code on the Mac we had very unreliable results and were not able to get clipping to work correctly if the vertex array or the gl_ClipDistance array was sized by a specialization constant.

Describe the solution you'd like
We would like to expand the definition of a shader compiler define in VSG to allow for an optional value to be associated with the define name. Here are several proposed solutions:

1. Continue with the existing std::string and std::set<std::string> interfaces for managing defines which allow client code to add valueless defines with a simple upper case string such as, "VERTEX_NORMALS". In addition to valueless defines that are currently supported, allow a define to have a "NAME=VALUE" syntax. This would allows a defined to be constructed as, "NUM_PRIMITIVE_CLIPS=3". There are several places in ShaderCompiler.cpp where the name portion of the define would need to be extracted when comparing with the list of #pragma import_defines, and when the define is expanded to the header stream. If an equal sign is found in a define it would be replaced with a space and then added to the headerstream just like it is today.
2. Add a new type, vsg::CompileDefine, that is used in place of the std::string interface and maintain a set of defines with std::set<vsg::CompilerDefine>. The constructor for vsg::CompileDefine would allow for valueless or name/value construction. The changes required to VSG would be much more intrusive and we might need special handling for VSG serialization. This makes option 2 less attractive than option 1.

Describe alternatives you've considered
We attempted to use shader specialization constants but it wasn't super clear if they are even allowed to be used to size global arrays and we had inconsistent behavior on the Mac. Additionally, even on Linux and Windows with NVidia and AMD we had to make a default value for the shader constant that was the maximum array size. In the end we determined that using specialization constants for this purpose was not correct.

Additional context
We are happy to provide a pull request for review.

[robertosfield]

I implemened the #pragma(tic) shader composition with a std::map<sttring, string> with the OSG, but chose std::set for simplicity and quickness of implementation with the expectation of generalizing to support the map<string, string> that the OSG has at some point in the future.

Retaining backwards compatibility with the most awakward part.

[rms7326]

Robert, would you like me to create a pull request with the changes required to change defines from std::set<string> to std::map<std::string,std::string>? Keep in mind that doing so would change the client API because the primary interface for adding a define is the std::set::insert(std::string) method. If we were to use a map that would require construction of a std::pair<std::string,std::string> at the client's call site. The solution I suggested works without breaking compatibility.

[robertosfield]

Requiring client code to be changed is breaking compatibility. If we can I'd much prefer being able to allow end user code that assumes a std::setstd::string to still compile.

Changing the serialization also breaks compatibility so that will need a bump in the VSG version so we can put a guard around the old and new defines data.

I think the right away to do this is wait till after I have tagged VSG-1.1.11 and then make the changes. To retain backwards compile compatibility we'd replace the std::setstd::string with a simple Defines struct that "has a" std::map<std::string, std::string> that has an insert(std::string& key) and a insert(string& key, string& value) method and iterator methods. This would enable basic compile compatibility.

[rms7326]

I don't believe my first proposal breaks compatibility since it uses the std::set<std::string> interface currently used. I checked serialization and it looks like it would be compatible. Here is an example:

hints id=56 vsg::ShaderCompileSettings
{
  vulkanVersion 4206592
  clientInputVersion 100
  language 0
  defaultVersion 450
  target 65536
  forwardCompatible 0
  generateDebugInfo 0
  optimize 0
  defines 2
  element "PHONG_LIGHTING"
  element "VERTEX_COLOR_SCALARS"
  element "VERTEX_SCALAR_COUNT=3"
}

The last name/value element, "VERTEX_SCALAR_COUNT=3" is just entered as a single element of the std::set. The only issue I see with it is the risk of someone adding the same define name with a different value or accidentally adding spaces around the equals or something like that which would cause double entries for a named item. This however could be checked at the time the defines are extracted for use in ShaderCompiler::combineSourceAndDefines and an error could be produced indicating the issue. Adding such a guard would allow full compatibility for when you move to a std::map in the future.

[robertosfield]

I see what you mean now.

The ShaderCompiler::combineSourceAndDefines() would need to strip off the actual define keyword before the = and the value after the = and then match the keywords to #pragma entries then insert the #define.

If you are happy to have a bash at making these change then go for it and I can review the final PR.

[rms7326]

Robert, I submitted a PR for VulkanSceneGraph and an updated example in vsgExamples.