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

EXT_manifold for glTF #420

Merged
merged 14 commits into from
May 5, 2023
Merged

EXT_manifold for glTF #420

merged 14 commits into from
May 5, 2023

Conversation

elalish
Copy link
Owner

@elalish elalish commented Apr 25, 2023

Deals with most of #422

Replaces #414, avoiding breaking changes to Mesh.

This is the beginning of a JS glTF I/O library for Manifold, allowing lossless round-trip of multi-material models by defining a new glTF extension (which I plan to write up and submit to Khronos).

@elalish elalish self-assigned this Apr 25, 2023
const primitive = doc.createPrimitive().setIndices(indices);
const material = materials[run];
if (material) {
primitive.setMaterial(material);
Copy link
Owner Author

Choose a reason for hiding this comment

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

@donmccurdy I'm loving your library! However, I'm getting Error: Cannot connect disconnected graphs. at this line. I can't quite tell from your docs how I should go about moving a Material (along with the textures it references) from a file I read to a file I'm writing. Is there a reason it can't attach them to the requested doc automatically? I was looking for something like Root.attach(), but came up empty.

Copy link

@donmccurdy donmccurdy Apr 25, 2023

Choose a reason for hiding this comment

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

There isn't currently a feature that transfers a resource on its own or with its dependencies, though that's an interesting idea. Currently the options would be:

(a) Merge documents, then clean up any unwanted resources. The prune() transform can help with getting rid of unused resources.

documentA.merge(documentB);

// ... assign materials as needed ...

// clean up
documentA.getRoot().listScenes()[1].dispose();
await documentA.transform(prune());

(b) Create a new material in the target document, then deep copy the original.

document.createMaterial().copy(material, (ref) => ref.clone());

EDIT: On second thought (b) may not be that straight-forward, this might be good for a feature request.

Choose a reason for hiding this comment

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks, that'll unblock me. Looking forward to the new feature though 😄

Copy link
Owner Author

Choose a reason for hiding this comment

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

Hmm, a) does not seem to be working - prune() doesn't remove anything, even though I have nodes whose only parent is the root. I tried debugging it, but I can't seem to figure out how to pull a non-minified version of your library with ES6 module support from jsDelivr. The source map helps a bit, but it's still quite hard to debug. Any thoughts?

Copy link

@donmccurdy donmccurdy May 2, 2023

Choose a reason for hiding this comment

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

Curious if that will break again the next time a new version is released?

Unfortunately, yes. The CDN is dumber than a bundler, it doesn't know what version(s) are already on the page, so it just resolves the newest compatible version of /core. The /functions module intentionally doesn't pin an exact version of /core — I would consider pinning good practice for a library's dev dependencies but not necessarily production dependencies.

Expected a JavaScript module script but the server responded with a MIME type of "application/node" ...

It looks like JSDelivr will serve the (.cjs) "main" entry by default, and the /+esm tag is required to request something else. Maybe this new esm.run thing by JSDeliver would be the way to go? It isn't something I've used before, though: https://www.jsdelivr.com/esm Never mind, it's the same thing. 😅

Copy link
Owner Author

@elalish elalish May 2, 2023

Choose a reason for hiding this comment

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

Ah, interesting. Well this works, and I think should be reasonably future-proof since I'm guessing you at least restrict by major version:

    "imports": {
      "@gltf-transform/core": "https://cdn.jsdelivr.net/npm/@gltf-transform/core@3/+esm",
      "@gltf-transform/functions": "https://cdn.jsdelivr.net/npm/@gltf-transform/functions@3/+esm"
    }

Nevermind, this still doesn't work.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ugh, I guess I should just switch to TypeScript. Probably a good idea anyway, especially for manifoldCAD.org. I just need to bite the bullet.

Choose a reason for hiding this comment

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

I do restrict to same major version, not sure why @3/+esm doesn't work. ☹️

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ah, I get it now - the /+esm thing is adding exact versions to your import statements and allowing them to automatically resolve. The reason /dist/core.modern.js isn't working is that I need to add all of your imports to my import map. I'm beginning to realize why bundling and tree-shaking is so necessary.

node.setMesh(mesh);
doc.getRoot().listNodes().forEach((n) => console.log(n.listParents()));
doc.transform(prune());
doc.getRoot().listNodes().forEach((n) => console.log(n.listParents()));
Copy link
Owner Author

Choose a reason for hiding this comment

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

@donmccurdy Here's the spot where I can see on the command line that two of the three nodes contain only the root as a parent, yet prune() doesn't remove them. Any thoughts?

Choose a reason for hiding this comment

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

probably just need to await the transform call. :)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Argh, thanks - however interestingly I get exactly the same results. After prune, I have nodes whose only parent is the Root, yet they're not marked as disposed (and prune still says it found nothing to dispose of).

Choose a reason for hiding this comment

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

Hmm that's a surprise! By default it should remove unused nodes, and also empty leaf nodes. Would it be possible to share the .glb in which prune is failing?

@codecov
Copy link

codecov bot commented Apr 27, 2023

Codecov Report

Patch coverage has no change and project coverage change: +3.63 🎉

Comparison is base (1959a30) 86.09% compared to head (db90451) 89.73%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #420      +/-   ##
==========================================
+ Coverage   86.09%   89.73%   +3.63%     
==========================================
  Files          38       35       -3     
  Lines        4604     4200     -404     
==========================================
- Hits         3964     3769     -195     
+ Misses        640      431     -209     

see 17 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@elalish
Copy link
Owner Author

elalish commented May 4, 2023

@pca006132 I'm getting a strange build error on the nix JS that I don't understand. I'm disabling it for now, but would you mind taking a look at it when you get a chance?

@pca006132
Copy link
Collaborator

Sure I will have a look at it


You can also test the manifoldCAD.org editor as well as our other example pages by serving from `bindings/wasm/examples/` with e.g. `npx http-server`.

Note that the `emcmake` command automatically copies your WASM build into `examples/built/` - these are checked into our repo in order to make sharing repro cases much easier. Note that you can test manifoldCAD.org on anyone's branch by simply going to: `https://raw.githack.com/<user>/manifold/<branch>/bindings/wasm/examples/index.html` e.g. https://raw.githack.com/elalish/manifold/glTFextension/bindings/wasm/examples/index.html
Copy link
Owner Author

Choose a reason for hiding this comment

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

This is inspired by three.js and I think will make working on the WASM a lot easier.


const NAME = 'EXT_manifold';

export class ManifoldPrimitive extends ExtensionProperty {
Copy link
Owner Author

Choose a reason for hiding this comment

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

This file is designed to make it easy to read and write the EXT_manifold glTF extension regardless of whether someone is using Manifold.

};
}

export function writeMesh(doc, manifoldMesh, attributes, materials) {
Copy link
Owner Author

Choose a reason for hiding this comment

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

This file is designed for interop specifically between Manifold and glTF. It doesn't actually have Manifold as a dependency because it can just use generic objects that look like a Mesh, but it's still based on our API.

.setIndices(spaceIndices).setAttribute('POSITION', position).setAttribute('TEXCOORD_0', uv);
const mesh = doc.createMesh('result').addPrimitive(moonPrimitive).addPrimitive(spacePrimitive);
const node = doc.createNode().setMesh(mesh);
const scene = doc.createScene().addChild(node);
Copy link
Owner Author

Choose a reason for hiding this comment

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

Much simpler example now that we can read and write files!

@elalish elalish merged commit 58fb7e3 into master May 5, 2023
21 checks passed
@elalish elalish deleted the glTFextension branch May 5, 2023 03:49
@pca006132
Copy link
Collaborator

It seems that the bug is this: emscripten-core/emscripten#19052 which is fixed in a new release of emscripten. I am waiting for NixOS/nixpkgs#229718 to land, and then we can just update the flakes (did not update it for so long!).

@elalish
Copy link
Owner Author

elalish commented May 5, 2023

Excellent, thank you!

@elalish elalish mentioned this pull request Nov 3, 2023
cartesian-theatrics pushed a commit to SovereignShop/manifold that referenced this pull request Mar 11, 2024
* extension export works

* reading GLBs

* trying to merge documents

* add await

* removed extra materials

* added builds

* several fixes

* fix versions

* move to built

* fixed test.js

* updated README

* disable nix JS test for now

* fix codecov timeout

* fix test
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.

None yet

3 participants