Add versioning rules to Developer Guide #1664
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This changelist adds two new sections to the Developer Guide, describing the categories of changes to the MaterialX API and data libraries that are allowed in version upgrades.
|
This proposal follows up on a discussion at our last MaterialX TSC meeting, and additionally incorporates ideas from the discussion in #1022. |
kwokcb
reviewed
Jan 20, 2024
|
|
||
| The following rules describe the categories of changes to the [MaterialX API](https://materialx.org/docs/api/classes.html) that are allowed in version upgrades: | ||
|
|
||
| - In *build* version upgrades, only non-breaking changes to the MaterialX API are allowed. For any API call that is modified in a build version upgrade, backwards compatibility should be maintained using deprecated C++ and Python wrappers for the original API call. |
There was a problem hiding this comment.
- I think it useful to have something that says what is a major, minor and build number in a release. At least for build. Something like "the syntax of a version is
<major>.<minor>.<build>? - Probably want to add in Javascript wrappers as well.
There was a problem hiding this comment.
For defining the major, minor, and build versions, my sense is that we cover this in the section above, and I'd like to keep this new section as simple as possible.
That's a good note about JavaScript, though we don't yet hold this API to the same level of backwards-compatibility as C++ and Python, since it's not yet integrated into major industry projects. In the future, I think you're definitely right that we'll want to hold the JavaScript API to the same level of rigorous compatibility, but we're not quite there yet.
documents/DeveloperGuide/MainPage.md
Outdated
|
|
||
| The following rules describe the categories of changes to the [MaterialX Data Libraries](https://github.com/AcademySoftwareFoundation/MaterialX/tree/main/libraries) that are allowed in version upgrades: | ||
|
|
||
| - In *build* version upgrades, only additive changes to the MaterialX data libraries are allowed, e.g. adding new nodes or node inputs with backwards-compatible default values. Because additive changes are backwards-compatible by nature, they do not require corresponding version upgrade logic in the codebase. |
There was a problem hiding this comment.
Do we note that since these are "non-breaking" that the build version is not required as part of the document version which has only <major>.<minor>.
There was a problem hiding this comment.
Q from before. Do we want to say that new versions of nodes are allowed as these can be forward-compatible.
There was a problem hiding this comment.
Do we want to mentioned that a nodegraph implementation for a nodedef can be modified as long as the interface does not?
There was a problem hiding this comment.
Although these are accurate statements, I think they might be in the category of "too much information" for this simple summary of our versioning rules. Hopefully these edge cases, along with many others that we don't have room to state, will become clear through usage in version updates over time.
| The following rules describe the categories of changes to the [MaterialX Data Libraries](https://github.com/AcademySoftwareFoundation/MaterialX/tree/main/libraries) that are allowed in version upgrades: | ||
|
|
||
| - In *build* version upgrades, only additive changes to the MaterialX data libraries are allowed, e.g. adding new nodes or node inputs with backwards-compatible default values. Because additive changes are backwards-compatible by nature, they do not require corresponding version upgrade logic in the codebase. | ||
| - In *minor* version upgrades, changes to the names and interfaces of MaterialX nodes are allowed, with the requirement that version upgrade logic be used to maintain the validity and visual interpretation of documents from earlier versions. |
There was a problem hiding this comment.
We might want to be careful as changes to the nodedef name I don't think should ever be allowed. Changes to the nodedef node can work.
There was a problem hiding this comment.
As long as both content documents and data libraries are marked with the MaterialX versions in which they were authored, I think we can safely update nodedef names in addition to node names, using the same Document::upgradeVersion logic. There are subtleties to consider for the future, e.g. USD containers without document versioning, but the rules in this section are focused on the case of MaterialX documents that are fully versioned.
|
|
||
| - In *build* version upgrades, only additive changes to the MaterialX data libraries are allowed, e.g. adding new nodes or node inputs with backwards-compatible default values. Because additive changes are backwards-compatible by nature, they do not require corresponding version upgrade logic in the codebase. | ||
| - In *minor* version upgrades, changes to the names and interfaces of MaterialX nodes are allowed, with the requirement that version upgrade logic be used to maintain the validity and visual interpretation of documents from earlier versions. | ||
| - In *major* version upgrades, changes to the syntax rules of MaterialX documents are allowed, with the requirement that version upgrade logic be used to maintain the validity and visual interpretation of documents from earlier versions. These changes usually require synchronized updates to both the MaterialX API and data libraries. |
There was a problem hiding this comment.
Does this mean we can't change the nodegraph / implementations for nodes to make them "visuallly" better in minor / branch upgrades? e.g. if we change std surf or openpbr ? It just kind of sounds like that IMO.
There was a problem hiding this comment.
This is a good point, and I would probably refine this language to say that "fixes that align node implementations with the specification" are allowed in build updates. In these cases, we're correcting defects in a node implementation, not redefining the node in the standard, and these fixes should be allowed.
There was a problem hiding this comment.
Thanks for this note, and I've adjusted the language to clarify this.
jstone-lucasfilm
merged commit 98eaae0
into
AcademySoftwareFoundation:main
1 check passed
kwokcb
added a commit
to kwokcb/MaterialX
that referenced
this pull request
Jan 30, 2024
* Web viewer formatting improvements and fixes (AcademySoftwareFoundation#1635) - Add colouring to items and folders which can be set vis CSS. This makes it easier to tell what areas are under what folders. - Fix string disable setting (was using old API). - Fix parenting of enum widgets to be under current folder instead of top level. * Add HwImplementation class This changelist adds an intermediate HwImplementation class, allowing the sharing of common features between node implementations in hardware shading languages. * Merge geometry node implementations This changelist merges the implementations of geometry nodes across hardware shading languages, allowing a greater degree of code sharing. * Merge application node implementations This changelist merges the implementations of application and NPR nodes across hardware shading languages, allowing a greater degree of code sharing. * Add vector2 variant of normalmap (AcademySoftwareFoundation#1355) The materialx specification defines the scale attribute of the normalmap to be either a float or a vector2 but the vector2 variant was missing from the implementation. This PR adds it. * Renderable logic improvements to web viewer (AcademySoftwareFoundation#1644) - Adds in proper parsing of renderable elements vs always just picking the first surface shader found. - The test suite files which have (multiple) nodegraph outputs and top level outputs will now load properly. - Adds in UI to mark folders as renderable (using a shaderball icon) - Adds in "soloing" capability to allow picking a renderable and have it show up on all geometry. When nothing is solo'ed the default material assignment is used. - Includes fixes for: - Dag path assignment matching . - Missing exposure of NodeGraph::getDownStreamPorts() in JS. - Addressing the big performance hit when binding materials to geometry in ThreeJS. The code by default is some quite slow code for reflection / debugging purposes which is now turned off. Chess set load is seconds vs minutes. This affects the 'solo'ing workflow significantly as each switch is a geometry re-bind. If the material is not already cached then slow code will be hit which can cause seconds to pass when selecting a new material -- which appears like a "hang" from a user perspective. * Fix irradiance generation in MaterialXView (AcademySoftwareFoundation#1647) This changelist fixes a regression to environment irradiance generation in MaterialXView, caused by a change to image caching logic for GLSL rendering in MaterialX 1.38.8. This fix restores the ability to render environment maps that are missing their pre-generated irradiance data, with irradiance being generated on the fly via spherical harmonics. * Add UI limits for useSpecularWorkflow and normal in UsdPreviewSurface (AcademySoftwareFoundation#1646) UsdPreviewSurface allows useSpecularWorkflow to be 0 or 1. Normals should have values between -1 to 1, inclusive. This PR puts these limits into place. * Unify noise unit tests This changelist merges two sets of noise unit tests into a single document, and aligns their implementations for clarity. * Static analysis optimizations This changelist addresses a handful of static analysis optimizations flagged by PVS-Studio and cppcheck, including the following: - Pass immutable std::string, FilePath, and FileSearchPath arguments by const reference. - Mark immutable ShaderGenerator references as const. - Prefer std::string::empty over comparison against an empty string. - Remove unused private methods Graph::findLinkId, Graph::findInput, and Graph::selectMaterial. - Remove variable assignments with no impact on code behavior. * Improvements to noise implementations (AcademySoftwareFoundation#1653) - Leverage node graphs to share the conversion aspects of noise implementations across languages. - Simplify noise unit tests to cover only unique implementations. * Fix shader generation typos This changelist fixes a handful of minor typos in shader generation, introduced in AcademySoftwareFoundation#1355 and AcademySoftwareFoundation#1553. * Add frame capture to web viewer (AcademySoftwareFoundation#1636) Add frame capture code to trigger on 'f' key. This is the same key as used for the desktop viewer. * Document format updates This changelist applies the mxformat.py script to the libraries and resources folders in the repository, updating formatting for a handful of documents. * Add versioning rules to Developer Guide (AcademySoftwareFoundation#1664) This changelist adds two new sections to the Developer Guide, describing the categories of changes to the MaterialX API and data libraries that are allowed in version upgrades. * Improve robustness of TypeDesc pointer comparisons (AcademySoftwareFoundation#1665) - The globals presets defined for TypeDesc are pointers which may not be shared between shared modules. This occurs for instance in Python where the pointers are declared locally for each module. - Any pointer comparison between the same TypeDesc preset can thus result in a failure status. * Improvements to smoothstep implementations - Leverage node graphs to share the conversion aspects of smoothstep implementations across languages. - Simplify smoothstep unit tests to cover only unique implementations. * Update changelog for recent work * Update comments in stdlib_ng.mtlx This changelist updates the comments in stdlib_ng, aligning them with the conventions for nodegraph definitions in the data libraries. * Fix orphaned links when deleting node in graph editor (AcademySoftwareFoundation#1667) This PR introduces fixes related to the removal of orphaned links when deleting a node in the Graph Editor: - remove the attribute `INTERFACE_NAME_ATTRIBUTE` of input pins that were connected to the deleted node (Fixes AcademySoftwareFoundation#1577) - iterate over all of the output pins instead of only handling the first one. (Fixes AcademySoftwareFoundation#1666) * Add facingratio node to nprlib (AcademySoftwareFoundation#1671) This changelist adds a `facingratio` node to the NPR data library, providing an additional intermediate node for building NPR graphs. * Add geometry drag & drop to web viewer (AcademySoftwareFoundation#1663) - Add support to recognize dropping of individual geometry (glb) files. - Minor cleanup to stop if no MTLX or GLB files loaded. * Apply JavaScript formatting This changelist applies automated formatting to the MaterialX JavaScript codebase, aligning it with the 4-space indentation and Allman braces used in MaterialX C++. * Add missing classification of VolumeShader nodes (AcademySoftwareFoundation#1675) ShaderNodes.cpp had missing classification information for Volume Shaders. This PR is a simple addition of that classification. * Add invert node to specification (AcademySoftwareFoundation#1676) The node exists in the standard library code, but is missing from the specification. * Improvements to facingratio - Fix syntax of input default values. - Use the invert node in facingratio for compactness. - Clarify the edge brighten example material. * Always build GLFW as a static library (AcademySoftwareFoundation#1680) Currently the embedded glfw build for MaterialXGraphEditor inherits the value of BUILD_SHARED_LIBS from MATERIALX_BUILD_SHARED_LIBS, but we're not installing libglfw, per AcademySoftwareFoundation#1245 the intention was to statically link. --------- Co-authored-by: Jonathan Stone <jstone@lucasfilm.com> Co-authored-by: mnikelsky <michael.nikelsky@autodesk.com> Co-authored-by: Eric Haines <erich@acm.org> Co-authored-by: Leo Belda <leo.belda@wanadoo.fr> Co-authored-by: Dhruv Govil <dgovil2@apple.com> Co-authored-by: ld-kerley <154285602+ld-kerley@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This changelist adds two new sections to the Developer Guide, describing the categories of changes to the MaterialX API and data libraries that are allowed in version upgrades.