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

Alternative way to do the bash #3

Closed

Conversation

colorscad-tester
Copy link

This is a large change to the bash script. Feel free to do with it what you will, use it as an alternative version, replace the current version, use parts of it, whatever you want. This works way better for my needs but YMMV. Tested on a Debian based system, unknown if it works on Mac/Win/etc.

Main changes

  • parallel used for doing renders in parallel

This replaces the backgrounding with parallel. (The script was failing for me due to the current back-grounding method). The code is a lot less complex. However it adds in a new requirement that you install parallel if you wish to do parallel rendering. It does fall back to single threaded if you don't have parallel installed. So if rendering in parallel is not needed you can use the script normally.

  • Options replace the argument positions

I also added in getopts to take in options as I needed to pass through var's into openscad while generating objects. It was going to be too complex to deal with positional args and options so I just made everything an option with input and output options required.

  • Fix for openscad file not in current working directory

The original bash also fails if the openscad file for input is outside your CWD. This is because openscad puts its output in the same place as its input when given a relative path. Openscad output was changed to put the file into the temp directory instead of your CWD which avoids the issue. Cleanup was reduced to deleting the tmp directory.

  • Option to overwrite output

I was annoyed that I had to always delete the existing output to re-generate, so I added a -f option to do that for me.

  • More output

This is a bit more verbose as I added messages while debugging. I decided I liked the output so I just left it. It could potentually be hidden behind a verbose flag I suppose.

  • Whitespace changes...

Sorry, I tend to like spaces so I wrote my bits with spaces and updates some of the tabs while I was around. You might want to sed out " " for "\t" to bring back your tabs if desired.

@colorscad-tester colorscad-tester mentioned this pull request Jun 6, 2021
@jschobben
Copy link
Owner

jschobben commented Jun 12, 2021

Thanks for this PR! Let me comment on each "topic".

parallel used for doing renders in parallel

Indeed the old backgrounding code is wrong, it made the false assumption that if x jobs finish, then wait -n won't block for x calls. That alone causes less jobs running in parallel than expected, and maybe also the issue you saw. It's fixable with pure bash, but will add even more complication to the script.
Using something like GNU parallel, with fallback if it's missing, does seem the better option. However its "citation" demand is a bit overbearing IMO (we're not doing academic research on parallel algorithms in here, are we 😛), and the output would probably confuse a colorscad user who doesn't usually use parallel and didn't run parallel --citation to silence it. Maybe we should just add --will-cite with a nice comment above it? Hopefully that option is portable enough.

Options replace the argument positions

Great! Maybe the "$@" syntax can also be used to forward args to openscad with less quoting trouble, have to try it myself.

Fix for openscad file not in current working directory

This nasty trick was done to make it work with Windows openscad, I suspect the fix might break Windows. It wasn't a perfect hack either. There are also still some issues such as trying to include a file (stl, font) in your .scad file. If the fix indeed breaks Windows, maybe we should just make a separate issue for this.

Option to overwrite output

Fair enough!

More output

Always good to have extra output, might indeed be best to hide it behind a flag though.

Whitespace changes...

This will always remain a religious thing, and to make it worse for some reason I tend to use tabs in shellscript and spaces in c++ code 🤣. Anyway I don't mind if it's tabs or spaces, as long as it's consistent. Maybe best to not change spacing together with "real" changes, though.

@jschobben jschobben mentioned this pull request Jul 1, 2021
@jschobben
Copy link
Owner

Just merged the bulk of the changes in here with PR #4. Let me open new PRs for the remainder (most notably, the parallel fixes).

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.

4 participants