JsBindings: Asynchronous loading of XML files in JS #1251

frericp · 2021-06-25T16:22:26Z

This PR adds proper support for readFromXmlString and readFromXmlFile to the JavaScript bindings, for both browsers and NodeJs. As part of that, it consolidates the way we use custom JS code in the emscripten module and cleans up the CMake file a bit.

Update #1184

frericp · 2021-06-28T08:24:14Z

The builds are failing because I've added some files in the TestSuite folder, which seem to be picked up automatically by some of the other tests. Any suggestion where I could move these test files to instead?

I realized that NodeJS might not need any special treatment at all, since files will usually be loaded from the local filesystem, which the native (C++) functions should be capable of. I'll try to bypass all the JS code for Node and simply let the C++ function handle file loading. If that works, we can remove all code related to NodeJS and therefore simplify the implementation.

kohakukun · 2021-06-28T11:02:56Z

source/JsMaterialX/JsMaterialXCore/post.js

-    return value;
-  }
-};
+//


Hum, I didnt add this header to source/JsMaterialX/JsMaterialXFormat/StrContainerTypeRegistration.h, could you add it there as well? the LICENSE thing :)

kohakukun · 2021-06-28T11:06:14Z

source/JsMaterialX/test/XmlIo.spec.js

-    const xmlString = mx.writeToXmlString(doc, writeOptions);
-    expect(xmlString).to.include(includePath);
-  });
+    let mx;


Wow, very comprehensive test suite. I'm wondering if this one is more through than the original cpp and whether it makes sense to put some of these tests there? like the cycle and the deeply nested includes. Could be in another task or PR, just wondering if it makes sense

The first three tests are stripped-down versions of a test that is run in the C++ pipeline as well. I haven't checked all of them, but I think there is also one that tests nested includes by now. Some of these JS tests might still be interesting for C++, but it's important to note that we're not testing the C++ implementation here, but the JS one. So even if the tests are duplicated in C++, they should remain here as well.

kohakukun · 2021-06-28T11:10:55Z

source/JsMaterialX/test/XmlIo.spec.js

+        // are skipped.
+        const doc = mx.createDocument();
+        const filename = 'CustomNode.mtlx';
+        await mx.readFromXmlFile(doc, filename, examplesPath);


not sure what I'm expecting from this? So, if I read the same file several times, do you get the document as it was before? Or what is added? if nothing is added can we add an assert more explicit, like the exported string is the same of the number of nodedefs are preserved?

kohakukun · 2021-06-28T11:16:29Z

source/JsMaterialX/test/XmlIo.spec.js

+        // Read and validate the example documents as strings
+        const examplesWithIncludesStrings = getMtlxStrings(examplesWithIncludes, searchPath);
+        readAndValidateExamples(examplesWithIncludesStrings, libs,
+            async (document, file, sp) => {


Do these async functions work? The async part is inside the callback, you would need to add the await aswell in the readAndValidateExamples calls 🤔

readAndValidateExamples has an await in front of the invoked callback.

Ah, now I got what you mean. You're right. I guess it works because the code after the readAndValidateExamples calls doesn't depend on what happens inside of this function. I'll still add the awaits, though.

kohakukun · 2021-06-28T11:17:58Z

source/JsMaterialX/test/XmlIo.spec.js

+    it('Disabling XML includes', async () => {
+        const doc = mx.createDocument();
+        const readOptions = new mx.XmlReadOptions();
+        console.log(readOptions);


kohakukun · 2021-06-28T11:36:48Z

source/JsMaterialX/JsMaterialXFormat/post.js

+            }
+            pathList[i] = path;
+        }
+        if (typeof paths === 'string') {


not fan of the return value if string otherwise look into the object. In Threejs they return the modified object aswell. I would say you always return something

kohakukun · 2021-06-28T11:39:17Z

source/JsMaterialX/JsMaterialXFormat/post.js

+        // Create folders if necessary.
+        var folders;
+        if (isFile) {
+           folders = file.substring(1, file.lastIndexOf("/")).split("/");


While reading through the code, I'm a little confuse when to use pathSep, PATH_LIST_SEPARATOR and the string literal "/". Can we substitute the use of "/" by a variable so that it is more readable when to use it? Something like "browserSep" 🤷‍♀️

I've added some comments on the variables to clarify their purpose.

kohakukun · 2021-06-28T11:40:12Z

source/JsMaterialX/JsMaterialXFormat/post.js

+
+            // Upload the initial file (string) to the WASM FS
+            var wasmCwd = getWasmCwd(wasmRootFolder);
+            var initialFileName = wasmCwd + "/ChosenToHopefullyNotClashWithAnyOtherFile123";


frericp · 2021-06-29T10:57:19Z

@bernardkwok This PR is ready to be merged, but the Codacy check complains about lines like while(--i) or if (++i < someValue). I can rewrite this, which would make the code a little more convoluted, or we ignore these issues. Whatever you prefer.

Scratch that. Turns out changing the code isn't too involved.

- Add in a file menu with hotkeys - Add in a help menu - Add in Viewer menu

frericp added 4 commits June 25, 2021 14:41

Asynchronous loading of xml files in browsers and NodeJs

9523663

clean up and documentation

136250b

more cleanup

2db7a73

more documentation

d08a106

frericp requested review from ashwinbhat, kohakukun, sdunkel, bernardkwok and muenstc June 25, 2021 16:22

frericp added 2 commits June 25, 2021 18:34

clarify readme docs

562d584

fix parameter check after introducing searchPath parameter

3193eba

kohakukun reviewed Jun 28, 2021

View reviewed changes

frericp added 2 commits June 28, 2021 15:05

Address review comments

b044859

address comments

a681ee5

kohakukun approved these changes Jun 28, 2021

View reviewed changes

frericp added 2 commits June 29, 2021 12:07

Moving test data to a different folder

b692a69

Set longer timeouts for some tests

352da55

Fix codacy issues

6d53420

frericp merged commit f618a3d into adsk_contrib/dev Jun 29, 2021

frericp deleted the adsk_contrib/async_xml_loading branch June 29, 2021 12:42

ashwinbhat pushed a commit that referenced this pull request Feb 25, 2023

UI Enhancements for MaterialX Graph Editor (#1251)

944c99c

- Add in a file menu with hotkeys - Add in a help menu - Add in Viewer menu

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JsBindings: Asynchronous loading of XML files in JS #1251

JsBindings: Asynchronous loading of XML files in JS #1251

frericp commented Jun 25, 2021

frericp commented Jun 28, 2021 •

edited

Loading

kohakukun Jun 28, 2021

kohakukun Jun 28, 2021

frericp Jun 28, 2021

kohakukun Jun 28, 2021

kohakukun Jun 28, 2021 •

edited

Loading

frericp Jun 28, 2021

frericp Jun 28, 2021

kohakukun Jun 28, 2021

kohakukun Jun 28, 2021

kohakukun Jun 28, 2021

frericp Jun 28, 2021

kohakukun Jun 28, 2021

frericp commented Jun 29, 2021 •

edited

Loading

JsBindings: Asynchronous loading of XML files in JS #1251

JsBindings: Asynchronous loading of XML files in JS #1251

Conversation

frericp commented Jun 25, 2021

frericp commented Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kohakukun Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frericp commented Jun 29, 2021 • edited Loading

frericp commented Jun 28, 2021 •

edited

Loading

kohakukun Jun 28, 2021 •

edited

Loading

frericp commented Jun 29, 2021 •

edited

Loading