Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial implementation of a StaticShape type #331

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

ratmice
Copy link
Contributor

@ratmice ratmice commented Jan 27, 2024

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
Copy link
Contributor Author

ratmice commented Jan 27, 2024

Already a patch for the lints in #330

@ratmice
Copy link
Contributor Author

ratmice commented Mar 15, 2024

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

@juliapaci
Copy link

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

@ratmice
Copy link
Contributor Author

ratmice commented May 28, 2024

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
Copy link

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

@notgull
Copy link
Contributor

notgull commented May 28, 2024

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

@ratmice
Copy link
Contributor Author

ratmice commented May 28, 2024

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.

@@ -1,6 +1,8 @@
// Copyright 2024 the Kurbo Authors
// SPDX-License-Identifier: Apache-2.0 OR MIT

#![allow(unused_qualifications)]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see a way to get this lint working, while leaving it enabled
it basically complains that we use crate::$it where $it evaluates to Rect because that is already in scope.
but the macro is called for other types which aren't in scope and for which the qualification isn't unnecessary.

@notgull
Copy link
Contributor

notgull commented May 30, 2024

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
Copy link
Contributor Author

ratmice commented May 30, 2024

Indeed, PrimitiveShape does seem like a better name.

src/primitive_shape.rs Outdated Show resolved Hide resolved
Comment on lines +9 to +35
mod _never_shape {
use super::*;
use crate::PathEl;
/// An uninhabited type that implements shape.
#[derive(Debug, Clone, Copy, PartialEq)]
#[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))]
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
pub enum NeverShape {}
impl Shape for NeverShape {
type PathElementsIter<'a> = core::iter::Empty<PathEl>;
fn path_elements(&self, _: f64) -> Self::PathElementsIter<'_> {
unreachable!()
}
fn area(&self) -> f64 {
unreachable!()
}
fn perimeter(&self, _: f64) -> f64 {
unreachable!()
}
fn winding(&self, _: Point) -> i32 {
unreachable!()
}
fn bounding_box(&self) -> Rect {
unreachable!()
}
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend removing this block and the whole External shape argument.

The External argument only lets you add one other shape to ConcreteShape anyway. If you want to add multiple "extra" shapes, you need to pass an enum as the External argument, at which point you might as well just implement you own enum instead of ConcreteShape.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My general feeling is that by using the defaulted type parameter it really doesn't add too much in the way of extra complexity the caller needs to be aware of (unfortunately it's still there to be looked at).

But does preserve the openness of type, meaning it'll becomes possible to use in library code even though someone is using an external shape argument, or across projects which both include extra shapes by making an enum which bridges both.

The need for an extra enum is not desirable, but it seems the only way to make an open enum type.
Without this I fear that someone will want to use the code in library code, and rather than might as well they must implement their own enum instead of using ConcreteShape. Then once people start targeting their own enum as the base of their API it becomes hard to share code across projects that depend upon kurbo.

So I would kind of push back, but I do think it is useful without the extra type parameter, and that custom shape impls seem pretty rare, so I would probably relent if there are strong feelings against this type parameter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants