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 multiple file export at once #34

Merged
merged 6 commits into from
Aug 7, 2024
Merged

Added multiple file export at once #34

merged 6 commits into from
Aug 7, 2024

Conversation

jmwright
Copy link
Member

This PR adds the ability to specify multiple output files at once in the outfile parameter by separating them with a ; character. The number of codecs specified via the codec parameter should match the number of output files. This prevents users from having to invoke cq-cli from scratch for each different export format they want of a single model.

@justbuchanan If you have the time and interest, please review. If you do not, I'll merge in a few days.

@jmwright
Copy link
Member Author

Looks like the tests are failing with the Numpy 2.x issue that is fixed in CadQuery master. I will need to figure out how to fix that here, but this code should still work fine for testing.

@jmwright
Copy link
Member Author

The Numpy version issue is now fixed.

@julianstirling
Copy link
Contributor

@jmwright The speed improvement is significant. I can build 3 files in 8s instead of 20s.

The one issue I am having is that it throws an error if I don't specify the codec (for one file or for multiple files)

@jmwright
Copy link
Member Author

jmwright commented Aug 1, 2024

@julianstirling

The one issue I am having is that it throws an error if I don't specify the codec (for one file or for multiple files)

Correct, you have to specify a matching codec for each file. The reason for this is that the codec cannot always be inferred correctly from the file extension. Is this a blocker for the use of cq-cli with ExSource?

@julianstirling
Copy link
Contributor

Not a blocker, just was useful that it used to guess it correctly.

I suppose it isn't clear to me from the -h script which tags are optional and which are mandatory (if not using -h)

@jmwright
Copy link
Member Author

jmwright commented Aug 1, 2024

@julianstirling I think that codec has always been required unless you are just validating the script. Are you able to get the main branch to run a single outfile without specifying a codec?

@julianstirling
Copy link
Contributor

@julianstirling I think that codec has always been required unless you are just validating the script. Are you able to get the main branch to run a single outfile without specifying a codec?

Yes, I never specified a codec before. It wasn't immediately clear what the codecs were called (though in hindsight they were pretty obviously named)

@jmwright
Copy link
Member Author

jmwright commented Aug 5, 2024

@julianstirling Can you try with the latest commit to this PR branch? You should not be required to specify the codecs anymore. If you use a filename extension that does not match the codec name like stp for STEP files instead of step, an error will be thrown.

@julianstirling
Copy link
Contributor

I just did a test and it worked. Thanks!

@julianstirling
Copy link
Contributor

I had a quick skim through the code as we discussed. I don't have anything helpful to add/say, except that the logic makes sense to me.

@jmwright
Copy link
Member Author

jmwright commented Aug 7, 2024

@julianstirling Great, thanks for the review.

@jmwright jmwright removed the request for review from justbuchanan August 7, 2024 15:08
@jmwright jmwright merged commit b009833 into main Aug 7, 2024
3 checks passed
@jmwright jmwright deleted the multi-file branch August 7, 2024 15:08
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.

2 participants