plottertools / vpype-gcode

gcode extension for vpype
MIT License
35 stars 7 forks source link

default configuration unit contradiction #14

Closed mo-nonym closed 2 years ago

mo-nonym commented 3 years ago

Hello. Great work ! Just noticed that in vpype-gcode/vpype_gcode/bundled_configs.toml for each config document_start includes a G20 command but the unit variable at the end is set to mm. The G20 command is a switch to inches. G21 should be for mm following this. Maybe there's something I don't get because the code is actually running very well ! (:

abey79 commented 3 years ago

@tatarize can you comment on this? My actual g-code experience is next to zero.

tatarize commented 3 years ago

The value for G20 and the units are independent. A disjoint there will give the units in inches but tell the gcode that the units are in mm. This would cause a size error.

tatarize commented 3 years ago
G20 - to use inches for length units.

G21 - to use millimeters for length units.

G20 specifies Imperial (inch) and G21 specifies Metric (mm)

[gwrite.gcode]
document_start = "G20\nG17\nG90\n"
segment_first = "G00 X{x:.4f} Y{y:.4f}\n"
segment = "G01 X{x:.4f} Y{y:.4f}\n"
document_end = "M2\n"
unit = "mm"

This appears to simply be accurate. It declares imperial units then scaled to metric according to the toml.

tatarize commented 3 years ago

It got switched over from the scale to units when was converted.

https://github.com/plottertools/vpype-gcode/commit/95bf5756cef67dcf3ead79641f809475a8d49e00

scale = 1 / vp.convert_length(unit) is now the defined scale from original line that defined the scale directly

'scale': 0.2645833333333333  # G20 scale.
print(1.0 / vp.convert_length("mm"))
0.2645833333333333
print(1.0 / vp.convert_length("in"))
0.010416666666666666

Apparently my original unit conversion was wrong and this was accurately converted over from my broken units to new broken units.

G20 converting from vpype default units of 1/96 inch really is 1/96 scale.

tatarize commented 3 years ago

The default units are 1/96 of an inch. So converting to inches should actually require multiplying them by 96. Converting to MM would be conversion 96 / 25.4 so multiplying the units by 3.77952755906. I swear I asked about the default units. I'll check the actual output maybe I'm messing up the conversion still and the underlying math is somehow getting the right answer when it shouldn't.

tatarize commented 3 years ago
unit_scale = vp.convert_length(unit)
document.scale(scale_x / unit_scale, scale_y / unit_scale)

I think the math should be clearly the G20 get in rather than mm because that's what G20 means.

However since our units are initially 1/96 of an inch. Converting them to inches is dividing by 96. So that actually is right. Trying that code:

vpype circle 0 0 1in gwrite -p gcode 1in.gcode

gcode2

These are the correct units.

[gwrite.gcode]
document_start = "G20\nG17\nG90\n"
segment_first = "G00 X{x:.4f} Y{y:.4f}\n"
segment = "G01 X{x:.4f} Y{y:.4f}\n"
document_end = "M2\n"
unit = "in"

[gwrite.gcodemm]
document_start = "G21\nG17\nG90\n"
segment_first = "G00 X{x:.4f} Y{y:.4f}\n"
segment = "G01 X{x:.4f} Y{y:.4f}\n"
document_end = "M2\n"
unit = "mm"

So using bCNC to confirm the units the math in the current version is right but the units are as said in the OP wrong. These should be switched to G20/in.

We also might want to query folks for their profiles. I'm guessing they would have corrected the default units in their own custom toml since they would basically always need it.

tatarize commented 3 years ago

Adding to this I requested other people's configs and all of them that were in actual use had the G20 and units correctly matched.

tatarize commented 3 years ago

Okay, that should correct it. Added a couple things because requested profiles had a need for indexes starting from 1 (since 0 was lift pen in BBC-GL). Upped the version to 0.9.0 since there's a chance somebody was using the default profile and if they updated it and had scaling trying to fix the units bug it would change their units size a lot and mess up. Testing with actual gcode and bCNC shows this is correct. Adding in other people's profiles also shows that they all correctly caught that error and either switched to G21 or to in to correct the desync.

I think this covers it. It should be updated on pypi.

tatarize commented 2 years ago

This was corrected on 0.9.0.