Create MaterialXGenHw module #2565
Conversation
…ecific components out from the common shader generation system
ld-kerley
force-pushed
the
shadergen/create-materialxgenhw
branch
from
bcdd225 to
c76e0a4
Compare
There was a problem hiding this comment.
This looks promising, thanks @ld-kerley, and I've written up some recommendations for making the PR more focused and streamlined.
| @@ -287,7 +287,7 @@ if(MSVC) | |||
| add_compile_options(/WX) | |||
| endif() | |||
| else() | |||
| add_compile_options(-Wall -Wno-missing-braces) | |||
| add_compile_options(-Wall -Wno-missing-braces -Wunused-parameter) | |||
There was a problem hiding this comment.
This seems like a worthwhile proposal, but let's keep this PR focused on the new MaterialXGenHw module, and we can take up a discussion of additional GCC/Clang warnings in a separate PR.
In particular, I'm not sure the unused-parameter warning leads to a better MaterialX codebase, and we might consider taking the reverse approach, where this warning is disabled across all platforms.
There was a problem hiding this comment.
The disparity of the current compiler arguments leads to significant friction when developing on non Windows platforms as the rules are currently stricter there. If we're going to remove this work from this PR - I would either want to immediately submit it as a separate PR - or have someone on the Windows side take a look at loosening the compiler warnings pretty soon. I don't develop on a Windows platform, so am unable to tackle things from that side.
| @@ -6,26 +6,27 @@ | |||
| #include <MaterialXGenGlsl/GlslShaderGenerator.h> | |||
|
|
|||
| #include <MaterialXGenGlsl/GlslSyntax.h> | |||
| #include <MaterialXGenHw/HwLightShaders.h> | |||
There was a problem hiding this comment.
For consistency with existing code, let's organize these includes within each source file as a block for MaterialXGenGlsl, followed by a block for MaterialXGenHw, and then a block for MaterialXGenShader.
There was a problem hiding this comment.
In this specific instance the block of headers included below this are all the different C++ nodes being included - it seemed to make things more readable to me to keep those separate - and then the couple of headers above are then (I think) in the suggest order.
if you'd prefer to break apart the list of C++ node includes to keep the module headers grouped - then let me know - it just felt like a smaller change to align things this way, but I don't have strong opinions either way here.
| @@ -3,15 +3,15 @@ | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
| // | |||
|
|
|||
| #ifndef MATERIALX_HWBITANGENTNODE_H | |||
| #define MATERIALX_HWBITANGENTNODE_H | |||
| #ifndef MATERIALXGENHW_NODES_HWBITANGENTNODE_H | |||
There was a problem hiding this comment.
This is an interesting idea for a future convention for our include guards, but for now I'd recommend keeping the existing convention, and we can consider global improvements to these naming conventions in a separate PR.
There was a problem hiding this comment.
I was concretely bitten, while working on this PR by the include guards not being unique, I also think that the possible upcoming work on the Shader Generation system is likely to require us to have an include guard convention that allows duplicated filenames in different modules.
It's worth noting that there is no current consistent include guard system applied through the project. With examples such as
MATERIALXVIEW_VIEWER_H- which already follows this new convention I'm suggestingMATERIALX_FORMAT_UTIL_H- which is similar in idea to what I'm suggesting - but not quite the same.MATERIALX_DOCUMENT- that doesn't end_HMATERIALX_ELEMENT_H- that does end_HMATERIALX_UNIT_H_- that even ends_H_(I think this is the only one)
As this is a new module, it doesn't seem like its harmful to introduce new style include guards that are going to be easier to keep unique.
If we can agree on a new include guard syntax - I'm happy to put up a separate PR conforming the rest of the library to the new convention - but I would like to land whatever we agree for this new module in this PR.
Is there a concrete concern you have with the include guards as they stand in this PR?
Following on from the great work in #2555, this PR consolidates the shader generation code for HW shading languages in to a new MaterialX module - MaterialXGenHw.
This PR does not change any lines of actual code, only moves them around to different files, and updates include statements to reflect the new headers.
HwShaderGenerator.hhas been split in to separate files, which reduces the coupling to some downstream modules to just the constants in some places.One thing to note, the include guards for this new module are of the form
MATERIALXGENHW_<FILENAME>. This is a slight change from the existing convention where things startMATERIALX_. I think this would be a useful convention to rollout across the rest of the codebase, as it reduces the change for clashing include guards.The MaterialXRender directly links to this new module, and that may not be optimal. We may want to refactor things such that only MaterialXRenderHw links to this new module, leaving the MaterialXRender module agnostic of shading languages, but this is outside the scope of this PR. I welcome discussion/PRs for this work if people think it is important.
I did not attempt to refactor the python or javascript bindings to reflect this new module because I'm not super familiar with that code, and am not a regular user of the bindings. If it is important to have the bindings reflect the C++ modules then I would welcome a PR to reflect this change in the bindings.
It should be noted that this does break the API. As noted by @niklasharrysson here we've already broken API compatibility. I think it might be possible to provide some shim code to provide compatibility, if that is important for downstream consumers, but I propose we "wait and see" if that is required.
On a similar note, a number of unnecessary header includes were removed, they appeared to be copy/paste inclusions, so it should be harmless, but technically it could impact downstream consumers if they were dependent on them.
The only exception to the "no code was changed" is to fix newly introduced warnings. I added
-Wunused-parameterto GCC/Clang compilation because I was bitten by the Windows compiler catching more warnings. This was raised by @bernardkwok in #2385, so hopefully attempting to bring these things closer inline with each other will help.The warning should be disabled for NanoGUI and ImGui, as we don't want to modify that source, and the code changes here are in spots that the Windows compiler never reaches.