-
Notifications
You must be signed in to change notification settings - Fork 291
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
Added argument handling for ASCII export of assemblies to GLTF and STL #1418
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1418 +/- ##
==========================================
+ Coverage 94.24% 94.27% +0.02%
==========================================
Files 28 28
Lines 5790 5799 +9
Branches 987 990 +3
==========================================
+ Hits 5457 5467 +10
Misses 199 199
+ Partials 134 133 -1
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@lorenzncode @adam-urbanczyk Please take a look when you have time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jmwright. Yes, IIRC there was a timing issue where the STL assembly export PR was created before but merged after STL binary support was added.
I would suggest not to add both binary
and ascii
keywords.
# ?
assy.save("out.stl", binary=True, ascii=True)
Can glTF export instead be enhanced to handle the format automatically based on extension?
I expect this to export to JSON.:
assy.save("out.gltf")
Expect binary glTF instead of error:
assy.save("out.glb")
@lorenzncode I made that change, but do you think that level of implicit behavior will be confusing for users? I added a note to the import/export documentation, which apparently had not been updated yet to show that we support GLTF export. |
@jmwright No, not at all! I've opened issue #1419. Other tools handle the glTF format automatically by file extension as well. The format is encoded in the file extension explicitly already by convention. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, you might want to fix coverage though.
codecov/patch keeps failing, but it does not seem like CodeCov is showing me the correct information when I click through to its report. What am I missing? |
I finally was able to fix CodeCov. @lorenzncode I think I have all of your suggestions implemented. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jmwright ! I tested the branch and did not find any other issues.
@adam-urbanczyk, @jmwright Is there a policy against git sqaush? If not, perhaps squash might be applied more on merge. For example, I like black, but maybe squash commits such as black fixes, codecov tests, etc.:
|
@lorenzncode squash is good, I just always forget to do it. |
I always squash when merging. |
The
binary
option forocc_impl.exporters.assembly.exportGLTF()
was not getting passed through from theassembly.save()
call. This PR fixes that and also adds theascii
option for STL export as well.