Update JavaScript bindings to 1.38 #1181
Conversation
| @@ -113,6 +108,9 @@ extern "C" | |||
| ems::function("DEFAULT_TYPE_STRING", ems::optional_override([]() { | |||
| return mx::DEFAULT_TYPE_STRING; | |||
| })); | |||
| ems::function("EMPTY_STRING", ems::optional_override([]() { | |||
There was a problem hiding this comment.
Any idea why these constants are exposed as functions and not via
https://emscripten.org/docs/api_reference/bind.h.html?highlight=bind%20h#constants
?
There was a problem hiding this comment.
Nope. I've put it on the todo list.
| api.isValidName = Module.isValidName; | ||
| api.incrementName = Module.incrementName; | ||
|
|
||
| api.getVersionIntegers = function() { | ||
| var vec = Module.getVersionIntegers(); | ||
| return vecToArray(vec); | ||
| }; | ||
|
|
||
| // TODO: Do we really need to map such helper functions? JS already has a String.split method. |
There was a problem hiding this comment.
We probably should and add this as a point when we review/adjust the general implementation approach
There was a problem hiding this comment.
One thing to keep in mind is that the separator string indicates any character can be used as the separator, whereas I think the JS versoin is an exact match of the entire string.
There was a problem hiding this comment.
The JS version can take a string (in which case it looks for an exact match) or a regex. The latter can be used to achieve the same result, e.g. with str.split(/[characters_to_search_for]/).
Independent of this particular function, the idea is to provide an interface that feels as native as possible to JS developers, e.g. by using the JS Array interface instead of C++ vectors. We will probably need to evaluate and discuss a couple examples like this on the way.
There was a problem hiding this comment.
Agreed, it is better to use native interfaces and as you say visit specific functions like these to see if they are useful to add. Some native utilities we're added to convert from MaterialX types to native types for the Python implementation if this is of interest.
There was a problem hiding this comment.
@bernardkwok The PR is currently not passing the checks due to some linting issues. Since we will most likely change/remove this code soon anyway, is there a way to ignore JS linting for now? If not, I can disable the failing check manually in the affected files.
I tried ignoring this in the codacy check but I will try to disable more. It's been a bit finicky in that I have tried to restart the check manually but it may only kick in on the next push to this PR.
| api.isValidName = Module.isValidName; | ||
| api.incrementName = Module.incrementName; | ||
|
|
||
| api.getVersionIntegers = function() { | ||
| var vec = Module.getVersionIntegers(); | ||
| return vecToArray(vec); | ||
| }; | ||
|
|
||
| // TODO: Do we really need to map such helper functions? JS already has a String.split method. |
There was a problem hiding this comment.
One thing to keep in mind is that the separator string indicates any character can be used as the separator, whereas I think the JS versoin is an exact match of the entire string.
Definitely, the bindings are currently not complete. As we discussed before, the way they are done might not be ideal, which is why we would like to look into how to improve that as a first step. Once this has been figured out, we can aim for full coverage. |
|
@bernardkwok The PR is currently not passing the checks due to some linting issues. Since we will most likely change/remove this code soon anyway, is there a way to ignore JS linting for now? If not, I can disable the failing check manually in the affected files. |
frericp
force-pushed
the
adsk_contrib/updateJsBindings
branch
from
69638de
to
dcf5860
Compare
Update #1184
This PR updates the existing JS bindings to work with v1.38 of MaterialX:
In addition to that, the build process has been improved a bit: