Refactor for clarity and performance improvements, add CLI #1

ES-Alexander · 2022-02-20T12:55:54Z

Hi @CNCKitchen, cool project!

I had a look at the code and thought it could be fun to help out by doing some refactoring.
Here's a brief summary of my changes/improvements:

I've tried to neaten things up a bit, grouping relevant functionality into classes
a simple test on my machine had this running ~8x faster than the initial script
while it's running there's now progress tracking that shows a line count and the time it's been processing for
I've also added in an argument parser so it can be run and configured from the command line instead of needing to change the code file (e.g. python3 bend_gcode.py my_file.gcode, or python3 bend_gcode.py --help to see all the options)

RichardBronosky

I found this PR to be too large to adequately review. I cloned the source repo locally and considered each commit individually. I read b6dd503 but it was a bit much for me to confidently approve.

I think we need to add [at a minimum] some gcode files to serve as before and after examples to be used to test code refactors and optimizations. But eventually you are going to need full blown unit tests.

commit 5591b6da3368859f0f97bd0c1c1212544dff47f6
    README: update to reflect CLI usage

commit f78bac007c3376161c5d66eaac93404620e70db7
    refactor for clarity+performance, and add CLI

commit 7c2d91dc5aa4b33c4b8c7b469a14f1cfbb70b6f4
    incorporate extra functionality, add caching

commit b6dd50373ebe2621b4628621da9fd483147d8df8
    refactor spline functionality, improve performance

commit a505e51617a62ed26bd62096d9faa62625cef1c0
    ignore common unwanted files

RichardBronosky · 2022-02-22T04:04:53Z

.gitignore

@@ -0,0 +1,2 @@
+*.gcode
+**__pycache__**


I think you should also include

*.pyc

I think we need to add [at a minimum] some gcode files to serve as before and after examples to be used to test code refactors and optimizations. But eventually you are going to need full blown unit tests.

Agreed that would be useful. I made my own test gcode file to compare against while I was refactoring so I could know the results weren't just garbage. From my verification, the only change that actually affects the generated output is the rounding in the writeLine function, because:

it's now f-string style, which includes trailing zeros (e.g. str(round(1.23999, 3)) -> '1.24', whereas f'{1.23999:.3f}' -> '1.240') - this may slightly increase file sizes, but is processed faster, and is closer to normal slicer outputs

it now supports user-specified precision, which can be used to reduce file sizes for less accurate printers/prints

I think you should also include *.pyc

I believe they're always generated inside a __pycache__ directory?

RichardBronosky · 2022-02-22T04:07:51Z

README.md

 - The model can't be too large in the X dimension, otherwise you'll get self intersections
 # Usage
 - Place your part preferably in the middle of your print plate with known center X coordinates
 - Place the sliced GCode in the same directory as the Python script


I haven't gotten far enough in the review to know if it is still required to Place the sliced GCode in the same directory as the Python script, but a mature CLI tool should not require this. I'm just noting it here before I forget.

Yeah, the argument works fine with a path. I decided not to change that usage line from Stefan's description because I expect most users won't be programmers, in which case it might be easier to leave like that, and someone who's comfortable with CLIs would likely try to just specify a path if it's more convenient to do so.

That said, I don't feel strongly about it, so would be happy for it to be changed to better reflect the actual usage. Perhaps this could end up being pip-installable as a script or something, in which case that would be more important.

README.md

bend_gcode.py

RichardBronosky · 2022-02-22T04:56:18Z

bend_gcode.py

+        for index, currentLine in enumerate(gcodeFile):
+            if index % 1000 == 0: # track progress
+                print(f"[{timedelta(seconds=perf_counter()-start)}]: line {index}\r", end='')
            if currentLine[0] == ";":   #if NOT a comment


Shouldn't this say if IS a comment?

See, I think it should, but it's Stefan's comment, and I'm not certain if he intended it to be commenting about the block being entered, or what happens if execution gets past that block... It's his project, so he gets to choose the commenting style, even if it means I'm stuck between severely disliking this style or it just actually being a mistake.

RichardBronosky · 2022-02-22T05:05:03Z

bend_gcode.py

                outputFile.write(currentLine)
                continue
-            if currentLine.find("G90 ") != -1:   #set absolute mode
+            if currentLine.startswith("G90 "):   #set absolute mode


You removed the trailing space on L72 but left it in L76. Is there a reason for this or was it an oversight?

I feel that both str().startswith and str().find are brittle and should be replaced with a dedicated function.

You removed the trailing space on L72 but left it in L76. Is there a reason for this or was it an oversight?

I think I actually added an extra space on 72 so the comments would line up between 72 and 76, since they already lined up (which can aid readability). Once again, not sure if an intentional commenting style from Stefan, or just the number of spaces that happened to end up on those lines.

I feel that both str().startswith and str().find are brittle and should be replaced with a dedicated function.

find could conceivably be incorrect if G90 appears in a comment. startswith should be correct, although could be marginally more robust with an lstrip() first. I chose to avoid that because it's then less clear, and I figured the input gcode is expected to come from a slicer, and I don't believe they ever put spaces before lines, but perhaps that's not the case (maybe some use indenting or something?).

Do you have any particular preference as to what should be there?

More generally, I expect a bunch of the logic throughout this function could be cleaned up somewhat, I just left it alone beyond where I thought something was error-prone, or needed to change due to the spline refactoring. In my profiling the logic itself wasn't an obvious performance throttle, so I figured my time would be more efficiently spent elsewhere :-)

bend_gcode.py

RichardBronosky · 2022-02-22T05:19:36Z

bend_gcode.py

+    from argparse import ArgumentParser, ArgumentDefaultsHelpFormatter
+
+    parser = ArgumentParser(formatter_class=ArgumentDefaultsHelpFormatter)
+    parser.add_argument("in_file", help="input file name (*.gcode)")
+    parser.add_argument("-o", "--out_file", help="output filename, default 'BENT_{in_file}'")
+    parser.add_argument("-x", "--x_values", default=(125, 95), nargs="*", type=float,
+                        help=("x values that define the spline, space-separated (e.g. '125 50 33')."
+                              " First should be in the center of your part."))
+    parser.add_argument("-z", "--z_values", default=(0, 140), nargs="*", type=float,
+                        help=("corresponding z values that define the spline, space-separated."
+                              " First should be 0 (e.g. '0 80 140')."))
+    parser.add_argument("-l", "--layer_height", default=0.3, type=float,
+                        help="layer height of the sliced gcode [mm].")
+    parser.add_argument("-a", "--warning_angle", default=30, type=float,
+                        help="Maximum angle [degrees] printable with your setup")
+    parser.add_argument("-b", "--bend_angle", default=-30, type=float,
+                        help="Angle [degrees] of the spline at the top point")
+    parser.add_argument("-d", "--discretization_length", default=0.01, type=float,
+                        help="Discretization length for the spline length lookup table")
+    parser.add_argument("-s", "--skip_plot", action="store_true",
+                        help="flag to skip plotting of the spline")
+    parser.add_argument("--xy_precision", type=int, default=4,
+                        help="Decimals of precision to round x/y values to.")
+    parser.add_argument("--z_precision", type=int, default=3,
+                        help="Decimals of precision to round z (height) values to.")
+    parser.add_argument("--e_precision", type=int, default=5,
+                        help="Decimals of precision to round extrusion amounts to.")
+    parser.add_argument("--printer_dims", nargs=2, default=(200, 200), type=float,
+                        help="printer width and height [mm], space-separated.")
+
+    args = parser.parse_args()
+
+    spline = Spline(args.x_values, args.z_values, args.discretization_length,
+                    args.bend_angle)
+    if not args.skip_plot:
+        print("Press Q to close the plot and continue")
+        spline.plot(printer_dims=args.printer_dims)
+
+    out_file = args.out_file or f"BENT_{args.in_file}"
+
+    main(args.in_file, out_file, spline, args.layer_height, args.warning_angle,
+         args.xy_precision, args.z_precision, args.e_precision)


I would have put all of this in a cli function, but that's just a style preference.

I don't see a particular benefit to that, given it's already in the script portion (so can't be imported), and is only 'called' once. I've never seen a cli function before, so am curious how it would be implemented. Are you thinking something like

def cli(): parser = ArgumentParser() parser.add_argument(...) ... return parser.parse_args()

or would the parser be returned instead of the parsed arguments?

Given it's your preferred approach, are there benefits I'm missing?

thereeroyz

I dont now frame

ES-Alexander added 4 commits February 20, 2022 18:45

ignore common unwanted files

a505e51

refactor spline functionality, improve performance

b6dd503

incorporate extra functionality, add caching

7c2d91d

refactor for clarity+performance, and add CLI

f78bac0

ES-Alexander force-pushed the refactor branch from f5c20f0 to f78bac0 Compare February 20, 2022 13:06

README: update to reflect CLI usage

5591b6d

RichardBronosky approved these changes Feb 22, 2022

View reviewed changes

thereeroyz approved these changes Feb 22, 2023

View reviewed changes

		@@ -0,0 +1,2 @@
		*.gcode
		__pycache__

Refactor for clarity and performance improvements, add CLI #1

Are you sure you want to change the base?

Refactor for clarity and performance improvements, add CLI #1

Uh oh!

Conversation

ES-Alexander commented Feb 20, 2022

Uh oh!

RichardBronosky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ES-Alexander Feb 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thereeroyz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ES-Alexander Feb 22, 2022 •

edited

Loading