JsBindings: Document bindings usage and contribution hints #1218
Conversation
frericp
requested review from
ashwinbhat,
kohakukun,
sdunkel,
bernardkwok and
muenstc
| @@ -1,14 +1,18 @@ | |||
| # Javascript Support | |||
There was a problem hiding this comment.
Just some minor changes up here that shouldn't lead to conflicts with @aGuluzade's work. I would like to refactor the building section once the Docker changes are merged.
| @@ -100,6 +103,87 @@ npm install | |||
| npm run test | |||
| ``` | |||
|
|
|||
| #### CI | |||
| Note that a sample build, install and test configuration can be found in the `.travis.yml` file. | |||
| ## Using the Bindings | |||
There was a problem hiding this comment.
Actual changes / additions start here.
| @@ -74,21 +78,20 @@ cmake --build . | |||
|
|
|||
|
|
|||
| ### Output | |||
| After building the project the `JsMaterialX.wasm` and `JsMaterialX.js` files can be found in `./<build_folder>/source/JsMaterialX/`. | |||
| After building the project the `JsMaterialX.wasm` and `JsMaterialX.js` files can be found in the global install directory of this project. | |||
There was a problem hiding this comment.
Can you be a littke more specific? I dont even use that path, I usually copy the bindings from ./<build_folder>/source/JsMaterialX/.
There was a problem hiding this comment.
I've explicitly made it less specific, since the configuration will no longer be controlled by the JS bindings sub-project as soon as this PR is merged: https://github.com/autodesk-forks/MaterialX/pull/1197/files#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20a
The intention is to explicitly install all assets to a globally managed path. Mentioning the current path here is condemned to get outdated.
|
|
||
| ### Install | ||
| To install the results into the install directory run | ||
| To install the results into the test directory run |
There was a problem hiding this comment.
can you also be specific here, like source/JsMaterialX/test . Otherwise sounds like a provided path
There was a problem hiding this comment.
Frankly, this whole section is bogus. Don't know why I even touched it. There will be another clean-up PR for this file later, where I want to remove this paragraph. I just postponed to not create conflicts with Aynur's PR.
source/JsMaterialX/README.md
Outdated
|
|
||
| #### Setup | ||
| These tests require `node.js`. This is a part of the emscripten environment. So make sure to call `emsdk_env` before running the steps described below. | ||
| These tests require `node.js`, which is shipped with the emscripten environment. Make sure to execute the `emsdk/emsdk_env` script before running the steps described below. |
There was a problem hiding this comment.
hum, not sure if it changes on windows but on mac I dont execute it but run source emsdk/emsdk_env
source/JsMaterialX/README.md
Outdated
| #### Data Type Conversions | ||
| Data types are automatically converted from C++ types to the corresponding JavaScript types, to provide a more natural interface on the JavaScript side. For example, a string that is returned from a C++ function as an `std::string` won't have the C++ string interface, but will be a JavaScript string instead. While this is usually straight-forward, it has some implications when it comes to containers. | ||
|
|
||
| C++ vectors will be converted to JS arrays (and vice versa!). Other C++ containers/collections are converted to either JS arrays or objects as well. While this provides a more natural interface on the JS side, it comes with the side-effect that modifications to such containers on the JS side will not be reflected in C++. For example, pushing an element to a JS array that was returned in place of a C++ vector will not update the vector in C++. Elements within the array can be modified and their updates will be reflected in C++, though (this does only apply to class types, not primitives or strings, of course). |
There was a problem hiding this comment.
I would drop the ! in vice versa~!~
| This section provides some background on binding creation for contributors. In general, we recommed to look at existing bindings for examples. | ||
|
|
||
| ### What to Bind? | ||
| In general, we aim for 100% coverage of the MaterialX API, at least for the Core and Format packages. However, there are functions and even classes where creating bindings wouldn't make much sense. The `splitString` utility function is such an example, because the JavaScript string class does already have a `split` method. The `FilePath` and `FileSearchPath` classes of the Format package are simply represented as strings on the JavaScript side, even though they provide complex APIs in C++. This is because most of their APIs do not apply to browsers, since they are specific to file system operations. In NodeJs, they would present a competing implementation of the core `fs` module, and therefore be redundant (even though they might be convenient in some cases). |
| As explained in the user documentation, types are automatically converted between C++ and JavaScript. While there are multiple examples for custom marshalling of types (e.g. `std::pair<int, int>` to array, or `FileSearchPath` to string), the most common use case is the conversion of C++ vectors to JS arrays, and vice versa. This conversion can automatically be achieved by including the `VectorHelper.h` header in each binding file that covers functions which either accept or return vectors in C++. | ||
|
|
||
| <!-- | ||
| TODO: Tests and other best practices |
There was a problem hiding this comment.
I guess you could add a small parragraph mentioneing that we dont aim to test the C++ functionality but but focus particularly on the custom functions and complex marshalling
Add some basic filtering and exception handling for file load.
First pass of updating the Readme. For now, I'm mostly adding text to it. I would like to do another cleanup and reorganization pass once the Docker documentation PR is merged.
Update #1184