Skip to content
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

Type description improvements #2176

Open
wants to merge 35 commits into
base: main
Choose a base branch
from

Conversation

niklasharrysson
Copy link
Contributor

This change list is an overhaul of the type system in shader generation.

Improvements include:

  • Removing the singleton pattern for TypeDesc storage and access, making the type system thread safe.
  • Improved handling of TypeDesc internal data, supporting any amount of data for the future, while keeping the class size minimal.
  • Handling of re-registration of type descriptions.
  • Support for keeping type descriptions alive after the lifetime of ShaderGenerator and GenContext. For applications that holds/uses the ShadePort reflection data that is generated by shader generation, even after the ShaderGenerator itself is destroyed. (To do this an application can create and pass in an external TypeSystem).

Co-work with @ld-kerley

@niklasharrysson
Copy link
Contributor Author

This PR includes and replaces #2107, which can be closed if this PR is approved.

.smart_ptr_constructor("GlslShaderGenerator", &std::make_shared<mx::GlslShaderGenerator>)
ems::class_<mx::GlslShaderGenerator, ems::base<mx::HwShaderGenerator>>("GlslShaderGenerator")
.constructor<mx::TypeSystemPtr>()
.class_function("create", &mx::GlslShaderGenerator::create)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see that it's tricky to provide JavaScript bindings for these create functions, in part because their default arguments are function calls rather than constant values.

As an alternative, what if the default arguments to these create functions were simply nullptr, and the create functions would internally call TypeSystem::create when they saw a null typeSystem input?

This would make it more straightforward to provide JavaScript (and Python) bindings for these create methods, and would allow us to simplify the new logic in our JavaScript test suite above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea! I was struggling to make the JavaScript bindings working again. But this should simplify things.

;
ems::class_<mx::EsslShaderGenerator, ems::base<mx::GlslShaderGenerator>>("EsslShaderGenerator")
.constructor<mx::TypeSystemPtr>()
BIND_CLASS_FUNC("create", mx::EsslShaderGenerator, create, 0, 1, mx::TypeSystemPtr);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any idea why I keep getting these errors with the binding here? As far as I can see I'm following the same pattern used elsewhere for binding a class function with optional parameters.

/home/runner/work/MaterialX/MaterialX/source/JsMaterialX/JsMaterialXGenGlsl/JsGlslShaderGenerator.cpp:17:42: error: expected ';' after expression
        .constructor<mx::TypeSystemPtr>()
                                         ^
                                         ;
/home/runner/work/MaterialX/MaterialX/source/JsMaterialX/JsMaterialXGenGlsl/JsGlslShaderGenerator.cpp:18:58: error: expected '(' for function-style cast or type construction
        BIND_CLASS_FUNC("create", mx::GlslShaderGenerator, create, 0, 1, mx::TypeSystemPtr);
                                  ~~~~~~~~~~~~~~~~~~~~~~~^
/home/runner/work/MaterialX/MaterialX/source/JsMaterialX/JsMaterialXGenGlsl/JsGlslShaderGenerator.cpp:18:91: error: expected '(' for function-style cast or type construction
        BIND_CLASS_FUNC("create", mx::GlslShaderGenerator, create, 0, 1, mx::TypeSystemPtr);
                                                                         ~~~~~~~~~~~~~~~~~^

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a guess, but are we providing a JavaScript binding for the TypeSystemPtr smart pointer, as we do here for the ElementPtr smart pointer?

https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/source/JsMaterialX/JsMaterialXCore/JsElement.cpp#L49

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have insight on this immediate problem - but it does raise the question in my mind - Do we need to provide a 1:1 API mapping for Javascript? - or do the use cases in the web domain suggest we should target something different, perhaps simpler or higher level? I'm certainly no expert in that area - but I thought I'd raise the question here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great question, @ld-kerley, and it's my sense that we currently provide too many MaterialX bindings in JavaScript, slowing down new development without substantial benefit for web developers.

@niklasharrysson If your takeaway is that our JavaScript bindings can be simplified in this area, I would definitely support that approach, and we can add JavaScript bindings for fine control of shader generation when they're truly needed in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants