search

linebender / kurbo

A Rust library for manipulating curves
Apache License 2.0
727 stars 71 forks source link

Add ConcreteShape type #401

Closed PoignardAzur closed 2 days ago

PoignardAzur commented 1 week ago

Supersedes #331. Based on @ratmice's code in that PR.

ratmice commented 1 week ago

I personally have no use for this code with the type modified this way since it is now a closed enum, rather than my original patch which was an open enum due to the generic parameter. This makes the type not useful for a library, when the library is intended to be used by a crate that implements the Shape trait.

PoignardAzur commented 1 week ago

We'll see in practice whether people have use for it. I already know it's going to be useful in Masonry.

ratmice commented 1 week ago

There is nothing here trait wise which requires the implementation to be in kurbo itself. IMO without supporting downstream implementations of the Shape trait if this is useful in Masonry, it is better to just put the implementation in Masonry. Would it's usefulness there even require it being pub?

waywardmonkeys commented 1 week ago

I also don't really understand what problem in masonry this is going to be solving to know how to evaluate that.

Why can't some experimentation happen with a version of this within masonry for now since we would otherwise have to do a release of Kurbo and hope that the API was both right and useful?

PoignardAzur commented 1 week ago

Well we could always stick to the version introduced in https://github.com/linebender/xilem/pull/591

ratmice commented 1 week ago

I definitely feel that I would prefer downstream crates adopting ad-hoc solutions over inclusion of a solution in kurbo that only works with Kurbo included implementations of the trait.

That is to say, that this is working around an issue with the Shape trait not being object-safe, The proposition with object safety is something like forall t: Trait, ObjectSafe(t) -> exists dyn Trait. This patch by being limited to kurbo specific variants for the Shape impls, doesn't apply in all the cases that forall would.

Thus my feeling is that this patch doesn't fix the problem, it only fixes the common occurrences of the problem. In doing so, it would seem to kill any motivation towards actually fixing the problem...

I would rather not see this included lightly based on it's convenience, Fixing it in downstream libraries like masonry isn't convenient or ideal, it in fact impedes code sharing across libraries that depend upon kurbo, but ensures that there is motivation to fix it properly, and avoids hamstringing kurbo to something silly should we decide to re-adopt object safety for the trait or some other option...

Apologies if i'm being overly difficult in this regard...

PoignardAzur commented 2 days ago

Overall, there's been little enough enthusiasm for this feature that I'd rather close the PR for now.