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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ mod mindist;
pub mod offset;
mod param_curve;
mod point;
mod primitive_shape;
mod quadbez;
mod quadspline;
mod rect;
Expand All @@ -125,6 +126,7 @@ pub use crate::insets::*;
pub use crate::line::*;
pub use crate::param_curve::*;
pub use crate::point::*;
pub use crate::primitive_shape::*;
pub use crate::quadbez::*;
pub use crate::quadspline::*;
pub use crate::rect::*;
Expand Down
177 changes: 177 additions & 0 deletions src/primitive_shape.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
// Copyright 2024 the Kurbo Authors
// SPDX-License-Identifier: Apache-2.0 OR MIT

#![allow(unused_qualifications)]

use crate::{Point, Rect, Shape};
use alloc::boxed::Box;

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!()
}
}
}
Comment on lines +9 to +35

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.

Copy link
Member

Choose a reason for hiding this comment

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

Fwiw, I also strongly agree with Olivier here. Then enums that these people will make would likely be enum {StaticShape, MyShape}; the scenario where that's an issue is exactly the same scenario where your propsed StaticShape<MyShape> becomes an issue.

(this is apart from the merit of this API at all, on which I have no opinion)

Copy link
Contributor Author

@ratmice ratmice Sep 25, 2024

Choose a reason for hiding this comment

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

Sounds like I'm in the minority here then, and I value consensus more than my own opinion, So I'll probably give it some time to see if anyone else chimes in, and otherwise remove it.

By removing it I guess what we lose is the ability to StaticShape<T> where T: Shape from library code, which is kind of what I was aiming for, along with avoiding a proliferation of shape wrappers.


/// Because the `Shape` trait is not dyn safe, it can be difficult to store
/// Collections of `Shape` items in hetereogenuous collections.
///
/// It allows an external `Shape` impl to be provided as an extension point
/// for shape impls provided by external crates. This defaults to an
/// uninhabited type.
#[derive(Clone, Debug, PartialEq)]
#[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))]
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
#[non_exhaustive]
pub enum ConcreteShape<External = _never_shape::NeverShape>
where
External: Shape,
{
/// Corresponds to a `PathSeg`
PathSeg(crate::PathSeg),
/// Corresponds to an `Arc`
Arc(crate::Arc),
/// Corresponds to a `BezPath`
BezPath(crate::BezPath),
/// Corresponds to a `Circle`
Circle(crate::Circle),
/// Corresponds to a `CircleSegment`
CircleSegment(crate::CircleSegment),
/// Corresponds to a `CubicBez`
CubicBez(crate::CubicBez),
/// Corresponds to an `Ellipse`
Ellipse(crate::Ellipse),
/// Corresponds to a `Line`
Line(crate::Line),
/// Corresponds to a `QuadBez`
QuadBez(crate::QuadBez),
/// Corresponds to a `Rect`
Rect(Rect),
/// Corresponds to a `RoundedRect`
RoundedRect(crate::RoundedRect),
/// A type implementing shape that may be defined by an external crate.
External(External),
}

macro_rules! from_shape {
($it: ident) => {
impl From<crate::$it> for ConcreteShape {
fn from(it: crate::$it) -> Self {
Self::$it(it)
}
}
};
}

from_shape!(PathSeg);
from_shape!(Arc);
from_shape!(BezPath);
from_shape!(Circle);
from_shape!(CircleSegment);
from_shape!(CubicBez);
from_shape!(Ellipse);
from_shape!(Line);
from_shape!(QuadBez);
from_shape!(Rect);
from_shape!(RoundedRect);

impl ConcreteShape {
/// Builds a static shape from an external shape implementation.
///
/// For a kurbo provided shape implementation, you would normally use the `from` impl instead.
pub fn from_external_shape<External>(shape: External) -> ConcreteShape<External>
where
External: Shape,
{
ConcreteShape::External(shape)
}
}

macro_rules! match_shape {
($x:ident, $it:ident, $e: expr) => {
match $x {
ConcreteShape::PathSeg($it) => $e,
ConcreteShape::Arc($it) => $e,
ConcreteShape::BezPath($it) => $e,
ConcreteShape::Circle($it) => $e,
ConcreteShape::CircleSegment($it) => $e,
ConcreteShape::CubicBez($it) => $e,
ConcreteShape::Ellipse($it) => $e,
ConcreteShape::Line($it) => $e,
ConcreteShape::QuadBez($it) => $e,
ConcreteShape::Rect($it) => $e,
ConcreteShape::RoundedRect($it) => $e,
ConcreteShape::External($it) => $e,
}
};
}

impl<External> Shape for ConcreteShape<External>
where
External: Shape,
{
type PathElementsIter<'iter> = Box<dyn Iterator<Item = crate::PathEl> + 'iter> where External: 'iter;
fn path_elements(&self, tol: f64) -> Box<dyn Iterator<Item = crate::PathEl> + '_> {
match_shape!(self, it, Box::new(it.path_elements(tol)))
}

fn perimeter(&self, acc: f64) -> f64 {
match_shape!(self, it, it.perimeter(acc))
}

fn area(&self) -> f64 {
match_shape!(self, it, it.area())
}

fn winding(&self, pt: Point) -> i32 {
match_shape!(self, it, it.winding(pt))
}

fn bounding_box(&self) -> Rect {
match_shape!(self, it, it.bounding_box())
}
}

#[cfg(test)]
mod test {
use super::*;
#[test]
fn test_collection() {
let r = crate::Rect::from_origin_size((0.0, 0.0), (1.0, 1.0));
let l = crate::Line::new((0.0, 0.0), (0.5, 0.5));
let shapes: Vec<ConcreteShape> = vec![r.into(), l.into()];
assert_eq!(
shapes,
vec![ConcreteShape::Rect(r), ConcreteShape::Line(l),]
);
}
#[test]
fn test_external() {
let l = crate::Line::new((0.0, 0.0), (0.5, 0.5));
assert_eq!(
ConcreteShape::from_external_shape(l),
ConcreteShape::External(l)
);
}
}
Loading