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

3mf node export #517

Merged
merged 3 commits into from
Aug 8, 2023
Merged

3mf node export #517

merged 3 commits into from
Aug 8, 2023

Conversation

elalish
Copy link
Owner

@elalish elalish commented Aug 4, 2023

This makes our 3MF export consistent with our GLB export when GLTFNode is used, creating the same scene hierarchy with mesh reuse. No materials yet, but we do include object names and a proper header.

This is blocked on hrgdavor/jscadui#37, which will need an npm release for us.

@elalish elalish self-assigned this Aug 4, 2023
@elalish elalish requested a review from pca006132 August 4, 2023 04:47
@elalish
Copy link
Owner Author

elalish commented Aug 4, 2023

FYI @hrgdavor

@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (7facd0e) 90.36% compared to head (bb5fd3c) 90.37%.
Report is 7 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #517   +/-   ##
=======================================
  Coverage   90.36%   90.37%           
=======================================
  Files          35       35           
  Lines        4431     4456   +25     
=======================================
+ Hits         4004     4027   +23     
- Misses        427      429    +2     

see 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hrgdavor
Copy link

hrgdavor commented Aug 4, 2023

just released @jscadui/3mf-export/0.4.1 with changes incorporated.

I would like to point out that the export supports embedding a thumbnail, it would be cool if manifold would take advantage of that when exporting. There is example of it in the repo called testGen.js

there was a small bug introduced in to3dmodel ... parameter ...header was changed to header which is not correct ... header is supposed to pickup all other keys from the function options param, and fwd it.

Copy link

@hrgdavor hrgdavor left a comment

Choose a reason for hiding this comment

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

version change needed

@@ -16,7 +16,7 @@
"@gltf-transform/core": "^3.2.1",
"@gltf-transform/extensions": "^3.2.1",
"@gltf-transform/functions": "^3.2.1",
"@jscadui/3mf-export": "^0.3.0",
"@jscadui/3mf-export": "^0.4.0",
Copy link

@hrgdavor hrgdavor Aug 4, 2023

Choose a reason for hiding this comment

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

needs to be 0.4.2

@elalish
Copy link
Owner Author

elalish commented Aug 4, 2023

there was a small bug introduced in to3dmodel ... parameter ...header was changed to header which is not correct ... header is supposed to pickup all other keys from the function options param, and fwd it.

Hmm, I actually changed that intentionally in order to make the header parameters work (and tested it). Since header is passed as a complete object to pushHeader, if you use ... you don't actually end up with a variable called header at all (you only get it's sub-members individually).

@hrgdavor
Copy link

hrgdavor commented Aug 4, 2023

What does your calling the function look like ? This dotted variant works in the test script within the package

@hrgdavor
Copy link

hrgdavor commented Aug 5, 2023

@elalish I checked the header parameter and the types you proposed. I agree with separating the header object instead of mixing the keys in the options.

v0.4.2 is published that is aligned with your call to to3dmodel that I also found more practical. I also separated out to3dmodelSimple variant of the function for cases where I have just set of meshes without a hiararchy (that is usual for jscad).
I transfered the proposed interfaces to jsdoc format with minor tweaks.

@elalish
Copy link
Owner Author

elalish commented Aug 5, 2023

@hrgdavor Awesome, thank you! I'll get back to this shortly, but I need to go to a wedding first.

@elalish
Copy link
Owner Author

elalish commented Aug 8, 2023

@pca006132 Did you say something about a problem with the windows TBB build?

@pca006132
Copy link
Collaborator

yes, maybe we can disable it for now until I find a way to fix it.

@elalish
Copy link
Owner Author

elalish commented Aug 8, 2023

It's already marked "not required", so I guess it's fine for the moment. Ideally we should scope down this CI list a bit (if we go full TBB) and make all the remaining ones required.

@elalish elalish merged commit fca6638 into master Aug 8, 2023
22 of 23 checks passed
@elalish elalish deleted the 3mfNodes branch August 8, 2023 05:35
@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
* added 3mf hierarchy support, not tested yet

* 3mf nodes work with updated exporter

* updated 3mfexport version
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.

3 participants