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

Python run script #38

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Python run script #38

wants to merge 10 commits into from

Conversation

itised
Copy link

@itised itised commented Aug 23, 2021

RE: #35 (comment)

I started implementing what I had mentioned in the referenced comment. This is definitely a WIP, but I thought it might help explain what I was talking about.

The dactyl_manuform.py file is definitely not working, but I think it's enough to demonstrate what I was thinking. Essentially I made a global shape_config that can be referenced from the functions. I haven't really gotten into this yet, but I imagine I could also use the debug flag from the command line to replace debug_exports and debug_trace

itised added 3 commits August 23, 2021 12:42
modularized generate_configuration, and updated Dockerfile and run.sh to work with the new run.py script
@joshreve
Copy link
Owner

The concept of run.py was what I was thinking as well as I worked through minimizing win / nix differences since python is shared code. It leaves only docker to have to be managed separately.

Unfortunately the whole program is a mess of globals which is definitely a carryover of its original form that I never managed to detangle. I think that is why you may have issues with the run.py. I'm sure it can work, but it will likely take a bit of hackery. Unfortunately this looks like a week I won't have much time to tackle this, so I am going to ensure do a few bug fixes to make it stable and start a dev branch to continue this. Hopefully I can devote a bit more time this weekend or later this week.

@itised
Copy link
Author

itised commented Aug 24, 2021

I just took the time to search/replace all the global variable. I was able to get a case to generate, and the output looks ok. But a test suite would definitely be helpful.

Still todo:

  • update run.bat
  • update README
  • I'm sure much more that I'm just blanking out on now...

@itised
Copy link
Author

itised commented Aug 24, 2021

I've taken care of all the warnings about unknown symbols, except for on line #3995

I'm not sure what those variables are supposed to be...

@joshreve
Copy link
Owner

I've taken care of all the warnings about unknown symbols, except for on line #3995

I'm not sure what those variables are supposed to be...

Wow, those snuck in there. I was going to look at a wedged base and must have immediately got sidetracked. I'll strip that when I get a chance. It shouldn't be there until it actually does something.

@joshreve
Copy link
Owner

Looking through this, I am going to bring most of it in. I should have converted to the dictionary references a long time ago, but just never go around to it. The global referencing was out of control.

The are two areas of concern:

Simplifying the workflow. I think I like the run.py approach since it keeps it os agnostic, but I am going to load your repo and try it out to make sure it make sense and can help reduce the execution paths. Ideally, I think I want 1 path that has 3 layers: Directly running the files, running through CLI, and docker execution through CLI. I will try that out with your setup. I think it gets close if I can just sort out the best docker approach to align.

Importing both cadquery and solid at the top. I was specifically shielding the imports such that you could run without both libraries installed. People tend to have issues with installing one or the other and I was trying to prevent people from having to get both operational. This one I will probably adopt some of the general aspect, but hide the imports again so you can run with only one library working.

@itised
Copy link
Author

itised commented Aug 31, 2021

I was actually thinking about the engine imports, and wanted to abstract away the engine-specific code, in the same vein as the engine specific helpers.

I haven't had time to dig into the code enough to really understand the actual model generation, so I'm not sure if it's feasible, but I wanted to also look into completely abstracting away each specific section of a model. Something like a PlateGenerator class, that could have sub-class implementations for each engine. If it's possible, a similar approach could be used for the thumb cluster, where instead of lots of conditionals in the main generator file, the correct ThumbGenerator (default, mini, trackball, etc.) could be used.

Again, I don't have much of a grasp on the actual model generation, so I'm not sure how feasible it is.

On another note, I was also thinking about asymmetric builds. I was thinking about removing the "symmetry" setting in the config and instead adding an "asymmetries" dictionary. If the "asymmetries" setting is missing or empty, the build is symmetric. But if it contains any values, they would be used as overrides when generating the other side of the keyboard. It would allow a lot more portable customization of a complete build.

All of this can be done in different updates, but I wanted to mention it here. If you think they might be useful ideas, it could help to keep them in mind as you integrate what you like from the PR.

@joshreve joshreve marked this pull request as ready for review September 4, 2021 19:48
import getopt, sys
import src.argument_parser as parser

args = parser.parse()
Copy link

Choose a reason for hiding this comment

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

Sorry to ask, but have you considered to use internal python lib argparse instead of re implementing this?

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