Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add shader-compiler option and versions 1.0.6, 1.0.7 and 1.0.8 #18996
Add shader-compiler option and versions 1.0.6, 1.0.7 and 1.0.8 #18996
Changes from 10 commits
6a681b9
20e953a
51e7d4a
964bdb1
c125aef
cb4b07f
59651fc
b70371c
e81621a
e0a7992
c7e4741
e788969
1cb80fb
a6666b6
33af023
8d3b4b9
baf37ab
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, remove the option shader_compiler. It will execute an extra step that will run git clone to another project:
In case needing an external dependency, it should be converted to a Conan package first, so we can manage it in the dependency, otherwise, we will have no control over any version, settings or options change.
For this very specific case,
glslang
is already available in Conan Center, so you could use it, but it would require a patch to use it as external dependency. Thinking about maintenance, it would be even better adding a feature directly to VSG project, giving the option to declaredglslang
as configurable, as a separated project, likeUSE_GLSLANG_SYSTEM
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the shader_compiler the VSG is not usable to its full extent. The original author integrates glslang here by adding directly into the source graph of VSG and uses headers that are not exposed by the glslang conan-package. I've tried really hard to integrate a custom glslang-package, but this required massive changes in the source-structure which are not maintainable on the long-run. As glslang-sources are inlined with VSG and the author likes to control which exact versions to be bundled with VSG I don't really get the argument about control over version.
I also tried to convince the author not to use git-submodules or direct checkouts but he arguments that he wants to have VSG as self-contained as possible. I can start another attempt to create a custom vsg-glslang package, but honestly I don't think I will have the time in the long run to maintain changes here by creating patches. Still, I'll try.
Sorry for inconveniences, but I thought checking it out into the own source-graph should be ok, as it is not visible from the outside, thus not interfering downstream with other packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I revisited the issue and found the culprit being a wrong assumption about the glslang version used. I was able to tackle the issue by adding a new version to glslang. So basically this PR has to wait for the glslang-PR completed. (link TBD)