Fixes for MaterialX format checker#2647
Fixes for MaterialX format checker#2647jstone-lucasfilm merged 5 commits intoAcademySoftwareFoundation:mainfrom
Conversation
- Add in XML syntax checking and validation as options. - Apply to libraries to fix some implementation files.
| <input name="specular_weight" type="float" value="1.0" uimin="0.0" uisoftmax="1.0" uiname="Specular Weight" uifolder="Specular" | ||
| doc="Multiplies the specular reflectivity." /> | ||
| <input name="specular_color" type="color3" value="1, 1, 1" uimin="0,0,0" uimax="1,1,1" uiname="Specular Color" uifolder="Specular" | ||
| doc="Color of the specular reflection (controls the physical edge-tint for metals, and a non-physical overall tint for dielectrics)." /> |
There was a problem hiding this comment.
Would it be visually clearer to maintain our existing layout for shading model inputs, where the doc string is placed on its own line?
To my eye, the original formatting with the doc string on its own line seems clearer and more understandable, though it's true that our MaterialX format checker doesn't yet handle this automatically.
There was a problem hiding this comment.
Personally I don't find it any clearer with the new line break - as to me it hints that it is only the "doc" attribute on that line, but we can see in the example above that "uiName" comes after, which my eyes fail to parse because of the new line break.
Ultimately I don't have a super strong opinion here, other than I think the formatter tools rules should be the ultimate truth. If we want to enforce any formatting convention the it should be implemented in the formatting tool. I feel that any human post-editing of the files is potentially error prone, and burdensome.
If we are going to enforce "doc" on a new line - are we applying that everywhere? does it make sense everywhere? If we do can we also ensure it is the last attribute in the element?
I would cast a soft vote against making a new line, only because I worry there will be other corner cases where we don't want the additional lines.
For instance here - https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/libraries/stdlib/stdlib_defs.mtlx#L109
do we feel it would be beneficial to introduce new lines for each "doc" attribute, including the one in the "nodedef" element?
There was a problem hiding this comment.
Even though I believe one aim is make original XML as user readable for things like comments and doc strings this makes it so that non-standard "extra" XML rules keep on having to be introduced. Additionally this promotes hand-editing XML which from this PR can introduce unnoticed user errors.
My vote is thus to not introduce more special case handling which no other XML parser will ever handle. If we want formatted document strings then this should be introduced properly and no rely on newlines embedded into a text file.
ld-kerley
left a comment
ld-kerley
left a comment
There was a problem hiding this comment.
This looks like a great change to me - anything we can do to improve our ability to keep formatting consistent across the project gets my vote.
Left a couple of small things that probably need to be fixed up locally below.
| <input name="amount" type="float" nodename="tangent_rotate_degree_offset" /> | ||
| <input name="axis" type="vector3" nodename="tangent_normal" /> /> | ||
| <input name="axis" type="vector3" nodename="tangent_normal" /> | ||
| <:anonymous name="1" /> |
There was a problem hiding this comment.
This looks like a typo in the original source?
I'm a little surprised this passes validation - are we confident that the validator is processing all of the files too?
There was a problem hiding this comment.
Hmm. The std library generally get's loaded into docs and this get's validated. Strange it's never been caught. Can patch the original and log a new parsing issue.
| <input name="specular_weight" type="float" value="1.0" uimin="0.0" uisoftmax="1.0" uiname="Specular Weight" uifolder="Specular" | ||
| doc="Multiplies the specular reflectivity." /> | ||
| <input name="specular_color" type="color3" value="1, 1, 1" uimin="0,0,0" uimax="1,1,1" uiname="Specular Color" uifolder="Specular" | ||
| doc="Color of the specular reflection (controls the physical edge-tint for metals, and a non-physical overall tint for dielectrics)." /> |
There was a problem hiding this comment.
Personally I don't find it any clearer with the new line break - as to me it hints that it is only the "doc" attribute on that line, but we can see in the example above that "uiName" comes after, which my eyes fail to parse because of the new line break.
Ultimately I don't have a super strong opinion here, other than I think the formatter tools rules should be the ultimate truth. If we want to enforce any formatting convention the it should be implemented in the formatting tool. I feel that any human post-editing of the files is potentially error prone, and burdensome.
If we are going to enforce "doc" on a new line - are we applying that everywhere? does it make sense everywhere? If we do can we also ensure it is the last attribute in the element?
I would cast a soft vote against making a new line, only because I worry there will be other corner cases where we don't want the additional lines.
For instance here - https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/libraries/stdlib/stdlib_defs.mtlx#L109
do we feel it would be beneficial to introduce new lines for each "doc" attribute, including the one in the "nodedef" element?
| <implementation name="IM_divide_vector4FA_genmdl" nodedef="ND_divide_vector4FA" sourcecode="{{in1}} / {{in2}}" target="genmdl" /> | ||
| <implementation name="IM_divide_matrix33_genmdl" nodedef="ND_divide_matrix33" sourcecode="{{in1}}" target="genmdl" /> <!-- TODO: Implement properly --> | ||
| <implementation name="IM_divide_matrix44_genmdl" nodedef="ND_divide_matrix44" sourcecode="{{in1}}" target="genmdl" /> <!-- TODO: Implement properly --> | ||
| <implementation name="IM_divide_matrix33_genmdl" nodedef="ND_divide_matrix33" sourcecode="{{in1}}" target="genmdl" /> |
There was a problem hiding this comment.
Might be good to move these comments above the lines they apply to - by hand.
|
As a general comment, I will revert all formatting changes which are under discussion and just keep the obvious XML error fixes. This should make the PR self-contained to just enhancing the |
…ripped. Clean up a few TODO comments.
jstone-lucasfilm
left a comment
jstone-lucasfilm
left a comment
There was a problem hiding this comment.
Thanks for this investigation work, @kwokcb, and these improvements look good to me!
jstone-lucasfilm
merged commit 0c1ed1e
into
AcademySoftwareFoundation:main
|
Nice! I see we used the correct XML form and changed those There still remains a definite XML format break enforced here. If we use UDIM tags, the resulting file will break the Python ElementTree parser: <input name="file" type="filename" value="diffuse<UDIM>.jpg" />is illegal. Correct XML representation is: <input name="file" type="filename" value="diffuse<UDIM>.jpg" />Looks like we chose readability by humans over strict adherence to W3 specifications by overriding PUGI code directly. This will be surprising to any TD wishing to apply XML based Python scripts to a collection of MaterialX documents. This hit me once this year as I use ET to check unit test result files and they started failing after I added UDIMs into the test mixture. |
|
I would cast a vote to move to a completely conformant XML parser. |
4 participants
Changes
The utility
mx_format.pywas not traversing to sub-folders so when applied tolibrariesit would miss some files.This fixes some files which had invalid XML (unescaped characters) as
PugiXMLwill "fix" this on write.Two additional options have been added:
Both are off by default.
Results
librariesfolder. Changes are mostly cosmetic except for the mentioned fixes to missed files.resourcesfolder. Found a file which had duplicate elements. Fixed manually.