linebender / kurbo

A Rust library for manipulating curves
Apache License 2.0
716 stars 69 forks source link

Initial implementation of a StaticShape type #331

Open ratmice opened 9 months ago

ratmice commented 9 months ago

This brings in the StaticShape It was the best name I could come up with, but I'm not opposed to changing it if we come up with a better one.

One of the difficulties with the Shape trait as it stands is that:

  1. Shape isn't object safe.
  2. it is very much not obvious to me if the typical erased_serde style hacks that can make some non-obj safe traits be obj safe can work with this style of trait that uses GATs.

This puts libraries and applications in an awkward pickle, a) there isn't a good way to store them in hetereogenerous collections, and b) because of the open trait there isn't really a single crate that can actually compile list of types implementing Shape. Given the combined effect of these two things, it feels like a really awkward corner of the language.

This pr attempts to deal with both of these things. For the latter I've added the notion of an External variant, which can be any T: Shape, For ergonomics in the case where there is no external variant this defaults to an uninhabited type, so if you are just using kurbo provided shape types, you shouldn't have to worry about that generic.

One of the big downsides of this implementation is that it ends up having to use a Box<dyn Iterator<...>> for the PathElementIterator. I really appreciate everyone who took the time to discuss some of this stuff yesterday on zulip. So thank you & I look forward to your feedback.

ratmice commented 9 months ago

Already a patch for the lints in #330

ratmice commented 7 months ago

This didn't require any changes after #340 but i've rebased it to keep it up to date, and fixed up clippy.

juliapaci commented 5 months ago

Would it be benifital to make a macro for all the matching?

ratmice commented 5 months ago

It didn't seem likely to me that it would be beneficial, because of the macros cannot expand to match arms limitation of macros. Unless there is some trick which I am unaware of. If you do have any ideas in that regard I'd happily consider it though.

juliapaci commented 5 months ago

But in this case all the match arms are similar so can't we write a macro for the entire match statement?

notgull commented 5 months ago

This use case is already mostly covered by BezPath, especially since you can convert any Shape into a BezPath in current kurbo.

ratmice commented 5 months ago

But in this case all the match arms are similar so can't we write a macro for the entire match statement?

Yeah, looks like that might work, I had previously tried not the entire match statement but the block of match arms, which also ran into the match arm error, it seems like including match as well somehow fixes that though.

This use case is already mostly covered by BezPath, especially since you can convert any Shape into a BezPath in current kurbo.

Mostly, but it does lose the specific primitive Shape kind involved, which is not easily regained after conversion to a BezPath. Consider editors like inkscape which have both Objects and Paths, and an Object to Path conversion. While a Circle Object may provide the ability to edit the radius property, there is no equivalent operation for BezPath. This code was written such that it retains the primitive shape kind exactly for that purpose.

notgull commented 5 months ago

This code was written such that it retains the primitive shape kind exactly for that purpose.

Thanks for explaining. Although I have to wonder if PrimitiveShape would be a better name for this type.

ratmice commented 5 months ago

Indeed, PrimitiveShape does seem like a better name.

ratmice commented 1 month ago

FWIW, I think that despite the approval, after some thinking on it I would rather like to investigate the other potential options I outlined in my original PR Message, like the erased_serde style. Which I had not really fully investigated since the "KISS" approach using an enum hit all the qualifications I had outlined in that message... Since after review it no longer hits all those qualifications due to the ugly bits.

At least I feel like it is worth revisiting, to see if I can get anything that hits points a) and b) of my original message without the ugly bits that review is removing that cover point b.

PoignardAzur commented 1 week ago

So what's the status of this PR? Even if we don't have a consensus on the External parameter (though it kind sounds like we do), I'd still like for this to be merged.

ratmice commented 1 week ago

The current status is that after review this patch won't solve all of the points listed in my original PR description, so I've been hoping to find time/motivation to spend on the potential alternatives (e.g. using more of erased_serde style which I don't feel I fully explored because the current patch worked well enough).

So I would prefer to just hold off until we've fully explored the space, and see if we can't get a type that works with other libraries and shape impl's outside of kurbo, but understand if you just want to have one that works with the types implemented in kurbo itself.

because as it is, there isn't a reason libraries/users can't just implement this type itself it is just annoying to duplicate everywhere.