Implement support for more shapes

[kvdveer]

I added support for rect, circle, ellipse, polyline and line. I believe that completes support for shapes (not counting text here). I also fixed a bug when converting the px unit to mm.

Unfortunately, this implementation is not yet correct, as the coordinate system needs a bit of rework first. Right now, the viewport is treated as a fixed transform, in combination with the width and height of the document. That is not correct - those transforms should only apply to dimensions without units. Dimensions with units should remain untransformed. I will address that in a separate MR.

[kvdveer]

I'm still struggling with the coordinates. I've added a (failing) testcase which tests all possible CSS units.

[sameer]

Played around with these changes on Sunday and I now see the coordinate system problems you were talking about, the output looks...very wrong 😞

If you have time to continue this that would be great, otherwise I can take over from here. What changes were you planning to make to the coordinate system? Ellipse arc flags aside, that seems to be the main problem here.

Input:

Output:

[kvdveer]

I am still working on this, and I'm making some progress - the coordinates I have now are at least the right order of magnitude. My progress is quite slow, both because I'm doing this in my (rare) off-hours, and I'm trying to learn Rust at the same time.

I'm okay with someone taking over if there's time pressure on this feature, I'll try to find something else that has (some) use to me, and doesn't have broad demand otherwise.

[kvdveer]

What changes were you planning to make to the coordinate system?

Svg2gcode uses mm as the internal unit. However, SVG typically uses unspecified internal units, which are often pixels. There isn't really a problem when converting from unitless coordinates, but the elements in this PR allow specifying units, which then go wrong. I've been layering changes to account for this, which yields quite a significant diff. In the end I plan to unwind these changes, and produce a much simpler diff.

[sameer]

I'm okay with someone taking over if there's time pressure on this feature, I'll try to find something else that has (some) use to me, and doesn't have broad demand otherwise.

I started building on top of your PR out of curiosity, and realized the exact problem you just mentioned with the coordinate system. I got pretty far on the overhaul, what do you think about me opening that separately for you to look at? Then, you can build on top of that & focus on the shapes

Also: if you are interested in #13, I wrote something for another project that I think can pretty much be copied over to here and fit into place: https://github.com/sameer/raster2svg/blob/main/src/graph/tsp.rs. The triangle inequality should still hold for this problem since it's all geometry, but maybe that's not the case.

[kvdveer]

Go ahead, I think you're in a better position to tackle the coordinates than I am right now. I had restarted anyway after I learned that the internal unit wasn't mm as I assumed, but 3.75mm instead... Op do 7 mrt. 2024 02:26 schreef Sameer Puri ***@***.***>:

…

I'm okay with someone taking over if there's time pressure on this feature, I'll try to find something else that has (some) use to me, and doesn't have broad demand otherwise. I started building on top of your PR out of curiosity, and realized the exact problem you just mentioned with the coordinate system. I got pretty far on the overhaul, what do you think about me opening that separately for you to look at? Then, you can build on top of that & focus on the shapes If you are interested in #13 <#13>, I wrote something for another project that can pretty much be copied over to here and fit into place: https://github.com/sameer/raster2svg/blob/main/src/graph/tsp.rs — Reply to this email directly, view it on GitHub <#48 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADGM2NJY3JLH4S5MV4FH6LYW665RAVCNFSM6AAAAABBKLNIKCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBSGE3TCMRQGM> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[sameer]

The coordinate system should be good to go after 8f9db32

I haven't upgraded the CLI or web packages to use this new version yet but you can override it in cli/Cargo.toml by setting svg2gcode = { path = "../lib", features = ["serde"]}:

[package]
name = "svg2gcode-cli"
version = "0.0.7"
authors = ["Sameer Puri <[email protected]>"]
edition = "2021"
description = "Command line interface for svg2gcode"
repository = "https://github.com/sameer/svg2gcode"
license = "MIT"

[dependencies]
svg2gcode = { path = "../lib", features = ["serde"]}
env_logger = { version = "0", default-features = false, features = ["atty", "termcolor", "humantime"] }
log = "0"
g-code = "0.3"
codespan-reporting = "0.11"
structopt = "0.3"
roxmltree = "0.19"
svgtypes = "0.13"
serde_json = "1"

[[bin]]
name = "svg2gcode"
path = "src/main.rs"

[sameer]

Also, if it's helpful: here's a dump of the shape support code I had so far

[kvdveer]

I'll wait for the coordinate stuff to land in main before I'll continue working on this (if that's still necessary).
I'll pick up #13 in the mean time.

[sameer]

I'll wait for the coordinate stuff to land in main before I'll continue working on this (if that's still necessary)

Excited to see your work on #13, let me know if there's anything I can help with.

If you are focused on that I can take over on this PR and finish off the shapes. But I will want you to review my code to make sure it's right 😅

[sameer]

Done in 225574d, the test works perfectly now after all the coordinate system changes