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

Added argument handling for ASCII export of assemblies to GLTF and STL #1418

Merged
merged 16 commits into from
Oct 23, 2023

Conversation

jmwright
Copy link
Member

The binary option for occ_impl.exporters.assembly.exportGLTF() was not getting passed through from the assembly.save() call. This PR fixes that and also adds the ascii option for STL export as well.

@codecov
Copy link

codecov bot commented Oct 14, 2023

Codecov Report

Merging #1418 (d33fbef) into master (f467ac9) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head d33fbef differs from pull request most recent head 36bffa8. Consider uploading reports for the commit 36bffa8 to get more accurate results

@@            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     
Files Coverage Δ
cadquery/assembly.py 96.76% <100.00%> (+0.03%) ⬆️
cadquery/occ_impl/exporters/assembly.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jmwright
Copy link
Member Author

@lorenzncode @adam-urbanczyk Please take a look when you have time.

Copy link
Member

@lorenzncode lorenzncode left a 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")

@jmwright
Copy link
Member Author

@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.

@lorenzncode
Copy link
Member

I made that change, but do you think that level of implicit behavior will be confusing for users?

@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.

cadquery/assembly.py Outdated Show resolved Hide resolved
cadquery/assembly.py Outdated Show resolved Hide resolved
Copy link
Member

@adam-urbanczyk adam-urbanczyk left a 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.

cadquery/occ_impl/exporters/assembly.py Show resolved Hide resolved
doc/importexport.rst Outdated Show resolved Hide resolved
doc/importexport.rst Outdated Show resolved Hide resolved
cadquery/assembly.py Outdated Show resolved Hide resolved
@jmwright
Copy link
Member Author

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?

@jmwright
Copy link
Member Author

I finally was able to fix CodeCov.

@lorenzncode I think I have all of your suggestions implemented.

Copy link
Member

@lorenzncode lorenzncode left a 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.

@jmwright jmwright merged commit cce60ce into master Oct 23, 2023
6 checks passed
@lorenzncode
Copy link
Member

@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.:

$ git log | grep -i black | wc
     89     460    3151

@jmwright
Copy link
Member Author

@lorenzncode squash is good, I just always forget to do it.

@jmwright jmwright deleted the ascii_exports branch October 24, 2023 01:40
@adam-urbanczyk
Copy link
Member

I always squash when merging.

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