Sphinx Python Documentation #1567
Conversation
|
|
| ### Example Images | ||
| ## Example Images | ||
|
|
||
|  |
There was a problem hiding this comment.
Not specific to this PR but I noticed that images don't show up properly on the PyPi package page for MaterialX.
If were adding more images, just curious is there a more robust way to specify image references -- such as an absolute reference to the web site location instead of using relative paths ?
There was a problem hiding this comment.
Oh, I see the issue: https://pypi.org/project/materialx/
It's great to see that the documentation content is reused in many places!
I've migrated the Markdown docs, including README.md, to use absolute paths now:
jstone-lucasfilm
changed the title
StefanHabel
force-pushed
the
#342-Sphinx-Python-Documentation
branch
from
8a66335
to
9e4c1ff
Compare
Work in progress. This PR adds a new build target named `MaterialXDocsPython`, which generates Python API documentation using Sphinx. The existing developer guide contents are incorporated into the new HTML documentation, which lives side-by-side to the existing Doxygen-generated C++ API documentation. The docstrings of the Python modules were tweaked to describe what the individual modules are responsible for. Signed-off-by: Stefan Habel <[email protected]>
StefanHabel
force-pushed
the
#342-Sphinx-Python-Documentation
branch
from
9e4c1ff
to
bef6e1f
Compare
…MATERIALX_BUILD_PYTHON_DOCS build option.
…d Markdown files to use absolute URLs. This is to make sure the URLs can be loaded within GitHub, Doxygen, Sphinx, and the PyPI project page. Requires internet access. Also updated XML text snippets to use XML comment delimiters.
…d `index.rst`. Changed index page title, added introduction text, and changed the order of Python modules, to reflect the order in which support for the different shading languages was added to MaterialX.
…ed import order dependency issue. This patch adds calls of `pybind11::module::import()` to the Python bindings of modules that depend on specific other modules. This approach is similar to importing required modules via `import` in Python packages/modules. At the same time, we can remove the new documentation for this issue, and can remove extra steps from the Sphinx `conf.py` configuration file. With this patch in place, it should be possible to import any of the MaterialX Python modules in any order.
…d Sphinx configuration. - Added a new description/about text in the sidebar, including blue link buttons to GitHub and MaterialX on Mastodon (with an SVG-based embedded icon) - Applied the "monokai" syntax highlighting theme to code snippets - Added custom Jinja templates for classes and modules, in order to add a section title before the alphabetical indices on pages - Added a custom HTML template for the navigation section in the sidebar in order to limit the depth of the table of contents - Added code to remove module names from the signatures of functions, in order to make the docs more readable
…a couple of typos in header files.
…define to produce detailed error messages in pybind11.
…docstrings to PyMaterialXCore.
…docstrings to PyMaterialXFormat.
…docstrings to PyMaterialXGenGlsl.
…docstrings to PyMaterialXGenMdl.
…docstrings to PyMaterialXGenMsl.
…docstrings to PyMaterialXGenOsl.
…docstrings to PyMaterialXGenShader.
…docstrings to PyMaterialXRender.
…docstrings to PyMaterialXRenderGlsl.
…docstrings to PyMaterialXRenderMsl.
…docstrings to PyMaterialXRenderOsl.
…d listings of modules in `README.md` files.
…d aliasing of `PyMaterialXFormat.readFromXmlFile()` function. The function is now exposed under its real name in the pybind11 bindings in C++.
Signed-off-by: Stefan Habel <[email protected]>
either within the `MaterialX` Python package or as a standalone module. Signed-off-by: Stefan Habel <[email protected]>
…emySoftwareFoundation#1567) Signed-off-by: Stefan Habel <[email protected]>
…Documentation Signed-off-by: Jonathan Stone <[email protected]>
| @@ -19,7 +19,7 @@ MATERIALX_NAMESPACE_BEGIN | |||
| /// filtered by the given shader type and target. By default, all surface shader nodes | |||
| /// are returned. | |||
| /// @param materialNode The node to examine. | |||
| /// @param nodeType THe shader node type to return. Defaults to the surface shader type. | |||
There was a problem hiding this comment.
@StefanHabel Starting with this edit, I'm seeing a great set of fixes to spelling, letter case, and other details in the C++ libraries for MaterialX. Would it make sense to separate these out into their own pull request, so that they can be reviewed independently of the new Python Documentation functionality in this proposal?
There was a problem hiding this comment.
yes, thanks @jstone-lucasfilm !
This PR is getting a little unwieldy.
We could generally split this into separate PRs:
- Base functionality of generating the Python API Documentation via Sphinx.
- Fixes in header files for typos encountered while copying docs in headers to docstrings in the Python bindings.
- Added argument names and docstrings in the Python bindings.
We could treat this PR here (1567) as the one for the Sphinx base functionality, splitting off parts 2 and 3 into separate PRs.
The third part (adding the actual docstrings) could be broken apart further into four separate PRs for the main (groups of) MaterialX Python modules:
- PyMaterialXCore
- PyMaterialXFormat
- PyMaterialXGenShader
- PyMaterialXGenGlsl
- PyMaterialXGenOsl
- PyMaterialXGenMdl
- PyMaterialXGenMsl
- PyMaterialXRender
- PyMaterialXRenderGlsl
- PyMaterialXRenderOsl
- PyMaterialXRenderMsl
While working on this, I also encountered a couple of issues in the bindings themselves:
PyMaterialXCore.UnitDef.setUnitType()is wrongly bound tohasUnitType(): https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/source/PyMaterialX/PyMaterialXCore/PyDefinition.cpp#L76-L77PyMaterialXCore.GeomPropDef: duplicate definitions can be removed: https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/source/PyMaterialX/PyMaterialXCore/PyGeom.cpp#L60-L71setGeomProp()hasGeomProp()getGeomProp()
PyMaterialXGenShader.HwShaderGeneratorstatic functions are wrongly bound as instance methods: https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/source/PyMaterialX/PyMaterialXGenShader/PyHwShaderGenerator.cpp#L31-L33bindLightShader()unbindLightShader()unbindLightShaders()
PyMaterialXRenderMsl.MetalTextureHandler.bindImage()is wrongly bound tounbindImage(): https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/source/PyMaterialX/PyMaterialXRenderMsl/PyMetalTextureHandler.mm#L17-L18
We could treat these issues in the Python bindings as a separate ticket and PR.
Here's a list of tickets and corresponding PRs in which we could organize the overall work here:
- New issue ticket for existing issues in Python bindings
- New PR to address the issues
- Sphinx Python Documentation #342
- Sphinx Python Documentation #1567 for the Sphinx part
- Separate new PR for docstrings for PyMaterialXCore
- Separate new PR for docstrings for PyMaterialXFormat
- Separate new PR for docstrings for PyMaterialXGen{Shader,Glsl,Osl,Mdl,Msl}
- Separate new PR for docstrings for PyMaterialXRender{,Glsl,Osl,Msl}
- Separate new PR for fixes in header files
Alternatively:
- New issue ticket for existing issues in Python bindings
- New PR to address the issues
- Sphinx Python Documentation #342
- Sphinx Python Documentation #1567 for the Sphinx part and docstrings in all Python modules
- Separate new PR for fixes in header files
I'd be happy with either approach.
I have more commits to add to this PR here, adding more argument names and docstrings. I'll keeping using this PR here for this for now.
There was a problem hiding this comment.
@StefanHabel One approach that could be useful here is to break out one separable change at a time from this larger pull request (e.g. fixes to C++ libraries), so that it can be reviewed and merged to MaterialX main on its own.
As each of these subsets is reviewed and merged to main, we can then click "Update Branch" on this larger pull request, effectively removing that subset from the main proposal.
This should help to focus reviewer attention on a single, independent change at a time, rather than considering the entire proposal at once.
There was a problem hiding this comment.
Understood! Sounds good!
I'll upload a few more commits to this PR here, to have everything in one place.
I'll then create separate PRs for subsets of this work, which can be reviewed and merged independently.
…ademySoftwareFoundation#1567) Mentioning that all functions and classes from those modules are available in the top-level `MaterialX` Python package. Also removed the repeated `import` statements from example code in utility functions of `PyMaterialXCore`. Also wrapped more module docstrings in `PYMATERIALX_DOCSTRING()`. Signed-off-by: Stefan Habel <[email protected]>
…areFoundation#1567) Signed-off-by: Stefan Habel <[email protected]>
…wareFoundation#1567) Signed-off-by: Stefan Habel <[email protected]>
…Foundation#1567) ...by renaming `Element._getChild()` to `Element.getChild()`. Signed-off-by: Stefan Habel <[email protected]>
…on#1567) Signed-off-by: Stefan Habel <[email protected]>
Signed-off-by: Stefan Habel <[email protected]>
AcademySoftwareFoundation#1567) Signed-off-by: Stefan Habel <[email protected]>
Signed-off-by: Stefan Habel <[email protected]>
…ation#1567) Signed-off-by: Stefan Habel <[email protected]>
…cademySoftwareFoundation#1567) Signed-off-by: Stefan Habel <[email protected]>
Signed-off-by: Stefan Habel <[email protected]>
…c-process-bases'. (AcademySoftwareFoundation#1567) Signed-off-by: Stefan Habel <[email protected]>
Signed-off-by: Stefan Habel <[email protected]>
Signed-off-by: Stefan Habel <[email protected]>
Signed-off-by: Stefan Habel <[email protected]>
Signed-off-by: Stefan Habel <[email protected]>
Signed-off-by: Stefan Habel <[email protected]>
Signed-off-by: Stefan Habel <[email protected]>
Signed-off-by: Stefan Habel <[email protected]>
Signed-off-by: Stefan Habel <[email protected]>
Signed-off-by: Stefan Habel <[email protected]>
Signed-off-by: Stefan Habel <[email protected]>
Signed-off-by: Stefan Habel <[email protected]>
|
@StefanHabel We'll be holding ASWF Dev Days 2024 on September 26th and 27th, so let us know if you might be interested in following up on this project then! |
Absolutely, count me in! Apologies for putting this on hold for so long. Looking forward to it! |
Work in progress.
This PR adds a new build target named
MaterialXDocsPython, which generates Python API documentation using Sphinx.The existing developer guide contents are incorporated into the new HTML documentation, which lives side-by-side to the existing Doxygen-generated C++ API documentation.
Docstrings were added to all modules, functions, classes, methods, and attributes.
All parameters were named. None should appear as
arg0,arg1,arg2etc anymore.Statistics about the Python API have been added to the Sphinx build process.
The Sphinx build output currently ends with the following messages, providing an overview of the state of the API docs:
Docstring example:
Preview of generated HTML pages:
Development environment:
Update #342