-
Notifications
You must be signed in to change notification settings - Fork 364
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
base: main
Are you sure you want to change the base?
Type description improvements #2176
Conversation
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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);
~~~~~~~~~~~~~~~~~^
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This change list is an overhaul of the type system in shader generation.
Improvements include:
Co-work with @ld-kerley