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_mesh_manifold #2286

Merged
merged 8 commits into from
Dec 6, 2023
Merged

EXT_mesh_manifold #2286

merged 8 commits into from
Dec 6, 2023

Conversation

elalish
Copy link
Contributor

@elalish elalish commented May 5, 2023

As the long ago spec editor of 3MF, I realized that a small glTF extension would get us full interoperability with solid-modeling workflows. I'm thinking EXT makes sense here since the point is certainly to transfer these files between different software, but this extension is a bit niche as pure rendering workflows have little reason to care. However, implementation is also nearly trivial on the consuming side, as this extension puts the onus on the producer to create a more constrained file.

I don't have my Google hat on for this one - this is related to my side project, Manifold, an open-source robust mesh geometry kernel. I have implemented reading and writing of this extension using @donmccurdy's excellent gltf-transform library. You can test it live here.

Though I've been working with Khronos here for several years, I've never proposed an extension myself before, so I'd love any feedback on how best to proceed with this.

@fire
Copy link

fire commented May 5, 2023

I am quite interested in this. Will give it a look.

@emackey
Copy link
Member

emackey commented May 11, 2023

Interesting. Does there need to be any harmony between this extension and the manifold requirements placed by KHR_materials_volume on the mesh?

https://github.com/KhronosGroup/glTF/tree/main/extensions/2.0/Khronos/KHR_materials_volume#extending-materials

We already have a ratified extension with a parameter (thicknessFactor) that indicates when the mesh is expected to be manifold as opposed to thin-walled. That one doesn't go into nearly the detail shown here on what exactly that means for the asset. But I wouldn't want this extension and volume to disagree on whether a given mesh was supposed to be manifold or not. Just something to think about.

@elalish
Copy link
Contributor Author

elalish commented May 11, 2023

@emackey Yeah, I think they'll work just fine together. In the volume extension I believe manifoldness is not required (or even defined), but simply recommended. This extensions would be a nice pairing to ensure that manifoldness in a strict way if desired. Having recently tested a bunch of glTF sample models, I can tell you most of the volume ones don't actually pass this manifold check, which isn't really a surprise or a problem per se.

@emackey
Copy link
Member

emackey commented May 11, 2023

@elalish You're right, the volume extension doesn't define what "manifold" means. But it is a normative requirement that when thicknessFactor is nonzero, the polygons of any mesh using that material are no longer considered a loose collection of thin-walled triangles, and instead are considered to be a watertight boundary around a 3D volume of material. Behavior of gaps in the boundary is undefined. I've thought about asking the glTF validator to check such meshes for water-tightness, but that may add too much complexity and cost to validation, I would guess. At the very least, doubleSided has no meaning for such meshes, and that's specified normatively too.

@elalish
Copy link
Contributor Author

elalish commented May 11, 2023

Yeah, that seems fine. I'd put the distinction this way: the volume extension wants the mesh to be watertight geometrically, since that's all renderers care about. This extension requires manifoldness topologically since that's what's required for computational algorithms. The classic case is a sharp crease in the normals - the triangles butt up against each other, so it's perfectly fine for volume, but since the verts themselves are duplicated and not associated, it can't be topologically manifold without this extension.

@fire
Copy link

fire commented May 11, 2023

If gltf transform can apply the manifold extension to valid gltfs in both optional and required mode, that would be amazing for the use case of ensuring a manifold supply-chain from input to output.

@elalish
Copy link
Contributor Author

elalish commented Jun 21, 2023

@lexaknyazev Would you be so kind as to check my extension spec? I've verified that it's working well for round-tripping the data, but I'm not so familiar with JSON schema.

extensions/2.0/Vendor/EXT_manifold/README.md Outdated Show resolved Hide resolved
extensions/2.0/Vendor/EXT_manifold/README.md Outdated Show resolved Hide resolved
extensions/2.0/Vendor/EXT_manifold/README.md Outdated Show resolved Hide resolved
extensions/2.0/Vendor/EXT_manifold/README.md Outdated Show resolved Hide resolved
@elalish elalish requested a review from lexaknyazev June 29, 2023 17:30
@elalish
Copy link
Contributor Author

elalish commented Jul 12, 2023

@lexaknyazev Do you know why the test failed? I have no idea what it's checking. Also, this is ready for a second review.

@emackey
Copy link
Member

emackey commented Jul 12, 2023

@elalish Looks like the tests passed on the most recent commit.

@elalish
Copy link
Contributor Author

elalish commented Nov 15, 2023

Okay, I've updated my gltf-io.ts with documentation and it's now able to handle different numbers of properties per primitive and such. I should probably publish it to npm.

Meanwhile, that is now allowing manifoldCAD.org to export GLBs with this extension that all pass the Khronos validator. For a round-trip, you can try https://manifoldcad.org/make-manifold where you can upload a GLB with this extension to verify it is in fact manifold, or a GLB without this extension and it will find any meshes that are close to manifold and Merge and re-export them with this extension. e.g. DragonAttenuation:
image
Becomes:
image
The dragon itself is close enough to manifold to successfully Merge, but the cloth backdrop is not.

@elalish
Copy link
Contributor Author

elalish commented Nov 15, 2023

My understanding is that as soon as there are two independent implementations of an EXT extension it can be merged (there is no ratification process like for KHR). @fire @hrgdavor @tpotancok @alteous are any of you interested in making a second implementation? I've kicked the tires on this enough that I'm confident the extension works quite well.

@elalish
Copy link
Contributor Author

elalish commented Nov 15, 2023

I also just saw https://github.com/KhronosGroup/glTF/tree/main/extensions#naming - should I rename this to EXT_mesh_manifold?

@hrgdavor
Copy link

are any of you interested in making a second

I am interested in doing gtlf import for jscadui and jscad, but the problem is that it will take a long time until I get to it. I would also likely use gltf-transform, hope that does not disqualify it as second impl. The main issue for me is time.

@javagl
Copy link
Contributor

javagl commented Nov 16, 2023

My understanding is that as soon as there are two independent implementations of an EXT extension it can be merged (there is no ratification process like for KHR).

There are some elements of the process that are not perfectly pinned down yet, and I'm omitting some details - the following is an attempt of a very short tl;dr:

The intention of the KHR prefix originally was that it is used for ratified extensions. But if there is a VENDOR_... or EXT_... extension, then it should still be possible to ratify it, without having to rename it. So the prefix cannot indicate the 'ratification state' here. (Now, KHR_ basically means that it was proposed by the Khronos group - omitting some details and vaguenesses here)

Whether an extension specification is merged depends more on the "maturity" of the extension, and whether it has been thoroughly reviewed (some of this is hard to quantify...)

The requirement for "two independent implementations" is indeed a condition for an extension being ratified (but this could be done for EXT_- and even VENDOR_-extensions, eventually)

I would also likely use gltf-transform, hope that does not disqualify it as second impl.

I'm not aware of a clear definition of what 'a second implementation' means. But I could imagine that an implementation of the extension in glTF-Transform could count as an 'implementation' - iff it is not solved with manual twiddling in raw JSON objects, but indeed an extension implementation in the spirit of the library. (This concept of 'two implementations' should be defined more clearly...)

@elalish
Copy link
Contributor Author

elalish commented Nov 16, 2023

I wouldn't push this extension for ratification - glTF is primarily about rendering, while this extension is for CAD/3D printing workflows that are not normally the core users of glTF.

Whether an extension specification is merged depends more on the "maturity" of the extension, and whether it has been thoroughly reviewed (some of this is hard to quantify...)

It would be great to have some "next steps" on this. There aren't yet two independent implementations, but there is an open source one that reads and writes with a bunch of tests. Pretty much the only concern I have left about it is the name. It would be nice to point people to a published spec to encourage them to implement.

@alteous
Copy link

alteous commented Nov 24, 2023

@elalish, I'd be interested in implementing this.

@elalish
Copy link
Contributor Author

elalish commented Nov 27, 2023

@alteous Great, let me know if you have any questions for me. How do you plan to make use of it?

@emackey
Copy link
Member

emackey commented Nov 27, 2023

Pretty much the only concern I have left about it is the name.

+1 for EXT_mesh_manifold unless the implementers here have already shipped stable implementations of this. If it doesn't break too much, it would be great to rename it before more implementations show up.

@alteous
Copy link

alteous commented Nov 27, 2023

@alteous Great, let me know if you have any questions for me. How do you plan to make use of it?

I should have clarified, we would export the extension data from our modelling app: https://kittycad.io/modeling-app

@elalish
Copy link
Contributor Author

elalish commented Nov 27, 2023

Okay, I updated the name here, along with my implementation: elalish/manifold#630 since I'm pretty sure we're the only one so far.

@emackey, merging is only blocked until there is an approving review - would you feel comfortable?

@elalish
Copy link
Contributor Author

elalish commented Nov 28, 2023

@hrgdavor we should just publish my manifold-gltf.ts and gltf-io.ts on npm and you can grab them as dependencies, especially if you're already using manifold-3d. Let me know if you're interested.

@elalish elalish changed the title EXT_manifold EXT_mesh_manifold Nov 28, 2023
@hrgdavor
Copy link

@hrgdavor we should just publish my manifold-gltf.ts and gltf-io.ts on npm and you can grab them as dependencies, especially if you're already using manifold-3d. Let me know if you're interested.

Yes I am interested, I think at least few more people that are experimenting with manifoldjs outside of jscad.

extensions/2.0/Vendor/EXT_manifold/README.md Outdated Show resolved Hide resolved
extensions/2.0/Vendor/EXT_manifold/README.md Outdated Show resolved Hide resolved
extensions/2.0/Vendor/EXT_manifold/README.md Outdated Show resolved Hide resolved
extensions/2.0/Vendor/EXT_manifold/README.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

@elalish elalish left a comment

Choose a reason for hiding this comment

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

Thanks for the helpful reviews - I believe I've addressed all the comments.

extensions/2.0/Vendor/EXT_manifold/README.md Outdated Show resolved Hide resolved
extensions/2.0/Vendor/EXT_manifold/README.md Outdated Show resolved Hide resolved
extensions/2.0/Vendor/EXT_manifold/README.md Outdated Show resolved Hide resolved
extensions/2.0/Vendor/EXT_manifold/README.md Outdated Show resolved Hide resolved
extensions/2.0/Vendor/EXT_manifold/README.md Outdated Show resolved Hide resolved
Copy link
Member

@emackey emackey left a comment

Choose a reason for hiding this comment

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

Looks like all of my feedback was addressed here.

@elalish elalish merged commit 50c03ba into KhronosGroup:main Dec 6, 2023
1 check passed
@elalish elalish deleted the EXT_manifold branch December 6, 2023 09:36
@elalish
Copy link
Contributor Author

elalish commented Dec 6, 2023

Looks like we're official now - thanks all! Let's exchange some solid geometry!

abbaswasim pushed a commit to abbaswasim/glTF that referenced this pull request Sep 7, 2024
* first draft

* addressing feedback

* update name

* rename folder

* addressing feedback

* optional sparsity

* further simplifications

* fix typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants