add triangle shape #350

juliapaci · 2024-05-29T11:25:57Z

Implements (at least starts to) a triangle shape, im not sure if this is useful enough to merge though.

Also while looking at the source i noticed that that places like here use Rect instead of Self unlike other parts of the codebase. And some parts of the documentation uses "[`Type`]" and other parts just use "Type". Wasnt sure to make an issue or not but should there be a cleanup of the codebase?

also there is alot of conversion between Vec2 and Point right now. is there a better way to handle these situations where i want to use Vec2 methods on a Point but will end up converting right back to a Point afterwards?

triangles have alot of opportunities for more specific methods so its worth noting that this can be heavily expanded upon which could come later. Aswell as this, i have made some decisions like using centroids as the origin of the triangle (https://web.archive.org/web/20131104015950/http://www.jimloy.com/geometry/centers.htm) which might not be the best? let me know.

ill continue working on this, specifically the TODOs (see inlined todos)

juliapaci · 2024-05-30T08:35:10Z

Should we add an Insets for triangle?

juliapaci · 2024-05-30T09:44:16Z

src/triangle.rs

+        Point::new(
+            (self.a.x + self.b.x + self.c.x) / 3.0,
+            (self.a.y + self.b.y + self.c.y) / 3.0,
+        )


One of those situations where we could do this or:

((self.a.to_vec2() + self.b.to_vec2() + self.c.to_vec2()) / 3.0).to_point()

I do prefer the latter. Also, it would be more efficient to multiply by (1.0 / 3.0) but this is a matter of preference. Leaving the division in preserves the case where the resulting coordinate is, say, an integer, while multiplication might not.

Its less likely to be an integer so ill multiply by 1/3

juliapaci · 2024-06-01T03:19:26Z

i realise that changes to other source files like vec2.rs should be another pr but right no this pr depends on them so ill keep it here for no. if this doesn't get merged then ill split my fork and make a pr just for those other source files.

juliapaci · 2024-06-01T03:20:37Z

src/triangle.rs

+    /// Updates [`Triangle`]'s vertice positions
+    /// such that [`Triangle::a`] is topmost, [`Triangle::b`] is leftmost, and [`Triangle::c`] is rightmost
+    #[inline]
+    pub fn organise(self) -> Self {


should British spelling be changed? (probably)

src/point.rs

juliapaci · 2024-06-01T09:40:04Z

src/triangle.rs

    pub fn organise(self) -> Self {
-        Self::new(self.topmost(), self.leftmost(), self.rightmost())
+        let t = Self::new(self.topmost(), self.leftmost(), self.rightmost());
+
+        t
    }


(ignoring the bug which will be fixed, im just trying to find a clean way) there are a few issues / design choices that can be made here:

should organise() mutate self?

a lot of other functions assume an organised position so should we either let the user call organised before these functions OR call organise in these functions

juliapaci · 2024-06-01T09:43:15Z

src/triangle.rs

+    /// The perimeter of the [`Triangle`]
+    #[inline]
+    fn perimeter(&self) -> f64 {
+        self.a.distance(self.b) + self.b.distance(self.c) + self.c.distance(self.a)
+    }
+


we have general perimeter and area functions but we also have specific ones for right angled triangles. should we add more specific ones for equilateral triangles and stuff or is it too much clutter?

also, i dont see the need to implement perimeter and area here when its in the Shape trait? this is also done for Rect.

It should just be the Shape trait impls. I think the reason it's in Rect is historical, those methods were written before the Shape trait existed.

juliapaci · 2024-06-01T09:48:24Z

src/triangle.rs

+    pub fn from_centroid_sizes(centroid: impl Into<Point>, sizes: [impl Into<Size>; 3]) -> Self {
+        let centroid = centroid.into();
+
+        let mut points: [Point; 3] = Default::default();
+        for (i, size) in sizes.into_iter().enumerate() {
+            points[i] = centroid + size.into().to_vec2();
+        }
+
+        Self::new(points[0], points[1], points[2])
+    }
+
+    /// A new [`Triangle`] with vertex distances of `distances` from the `centroid`
+    ///
+    /// NOTE: the new [`Triangle`] does not keep `centroid`
+    #[inline]
+    pub fn from_centroid_distances(centroid: impl Into<Point>, distances: [f64; 3]) -> Self {
+        let centroid = centroid.into();
+
+        Self::new(
+            centroid + (0.0, distances[0]),
+            centroid + distances[1] * Vec2::from_angle(5.0 * FRAC_PI_4),
+            centroid + distances[2] * Vec2::from_angle(7.0 * FRAC_PI_4),
+        )
+    }


maybe a transition from from* to with* like in the builder pattern is better. although im hesitant to do this here because this would require a change for the entire codebase so maybe should be another pr?

we also wouldnt have to include the centroid param this way since users could chain with with_centroid()

juliapaci · 2024-06-01T22:48:06Z

src/triangle.rs

+    pub fn organise(self) -> Self {
+        let mut points = self.as_array();
+        points.sort_by(|x, y| x.rightmost(y));
+
+        let a = self.topmost();
+        let b = *points.iter().find(|&&p| p != a).unwrap();
+        let c = *points.iter().rev().find(|&&p| p != a).unwrap();
+        Self::new(a, b, c)
+    }


we probably can do a better way, we cant just do this though because of double ups.

juliapaci · 2024-06-01T22:49:17Z

and with 8b03193 this pr should be ready for a review!

src/triangle.rs

juliapaci · 2024-06-05T06:58:00Z

on a side note i also think it would be useful to have from/to_triangle/circle/rect(), cause we do it for Ellipse already we may aswell do it for all other shapes which would also be super easy to implement. prob should be another pr tho

raphlinus

Thanks for the PR, and apologies for taking so long to review. Triangle is a beautiful shape with useful special properties, and this PR is a good start. Some general thoughts.

A very large number of items are marked as "not justified." Basically, we want methods that are used by potentially a broad range of applications. If it's only going to be used once, or for a test, it doesn't belong in the public API. It's very easy to add methods, but unfortunately extremely difficult to remove them. In addition, a theme in kurbo is mathematical correctness, which means that some methods which might be convenient (like Point + Point or, recently Affine * Vec2) aren't present. So most methods that aren't present for other shapes are out. That said, when there's something really special to triangles, it makes sense. I like centroid, and I'm also open to incircle (and circumcircle). For other methods, I'm open to an argument why it should be included.

There should be an impl Mul<Triangle> for Affine. Fortunately, it's simple, you can pretty much cut'n'paste the one for line.

Looking forward to the revisions, then I'll do another review.

raphlinus · 2024-08-14T22:33:13Z

src/size.rs

@@ -336,6 +336,16 @@ impl SubAssign<Size> for Size {
    }
 }

+impl From<f64> for Size {


This is not mathematically correct.

i think i only did that for testing in one of the commits but forgot to remove it sorry

raphlinus · 2024-08-14T22:34:13Z

src/triangle.rs

+    /// empty [`Triangle`] at (1.0, 1.0)
+    pub const ONE: Self = Self::from_coords((1.0, 1.0), (1.0, 1.0), (1.0, 1.0));
+
+    /// equilateral [`Triangle`] identity


This is not in fact an equilateral triangle. And I don't think it's justified.

haha sorry about that

i think it is useful for the user though (atleast one out of Triangle::Equilateral and Triangle::ONE) because the user can just scale them and work with scalars instead of triangle data.
Not sure if that makes sense but i can defiantly see this as a use case however if you still want it removed then thats fine

also why is Triangle::ZERO alright to keep?

raphlinus · 2024-08-14T22:40:59Z

src/triangle.rs

+    /// The area of the [`Triangle`]
+    #[inline]
+    pub fn area(&self) -> f64 {
+        let ab = self.a.distance(self.b);


It would be more efficient to compute this using the shoelace rule, no trig necessary. Also, it's usual in kurbo to compute signed area (with the same sign convention as converting into a bezpath).

This should work: 0.5 * (self.b - self.a).cross(self.c - self.a)

raphlinus · 2024-08-14T22:44:03Z

src/triangle.rs

+        (self.b.distance(self.c) * (self.a.y - self.b.y)) / 2.0
+    }
+
+    /// Updates [`Triangle`]'s vertice positions


I'm not convinced this should be in the public API.

"vertex"

Also, the description doesn't clearly state what happens if, for example, a is both topmost and rightmost.

I'm not convinced this should be in the public API.

yeah im not sure why i did that.

now that the functions that relied upon this method are gone should we still keep this?

raphlinus · 2024-08-14T22:46:19Z

src/triangle.rs

+        most_ordering!(self, { |x, y| y.x.total_cmp(&x.x) })
+    }
+
+    /// Maximum x-coordinate of the [`Triangle`]'s vertices


I think these individual functions should just be combined into the Shape::bounding_box implementation.

raphlinus · 2024-08-15T00:43:14Z

src/triangle.rs

+        Self::new(points[0], points[1], points[2])
+    }
+
+    /// A new [`Triangle`] with vertex distances of `distances` from the `centroid`


I don't think this is justified.

raphlinus · 2024-08-15T01:31:25Z

src/triangle.rs

+        self.as_array().iter().find(|&v| v.y == y).copied()
+    }
+
+    /// The topmost vertex of `self` as a [`Point`]


I don't think these are justified, I think it's fairly specialized and users can write it themselves. In addition, there's a naming problem, as "topmost" implies a y-down coordinate system, and one of the conceits of kurbo is that it's agnostic to whether the coordinates are y-up or y-down.

Oh right, their useless now anyway because the organise is useless

raphlinus · 2024-08-15T01:33:42Z

src/triangle.rs

+        )
+    }
+
+    /// Returns the [`Ellipse`] that is bounded by this [`Triangle`].


As mentioned elsewhere, I think incircle and circumcircle are the methods that should be included.

src/triangle.rs

src/point.rs

juliapaci

4d8f906

ill update the tests once you check it out

src/point.rs

juliapaci · 2024-08-15T03:07:45Z

src/size.rs

@@ -336,6 +336,16 @@ impl SubAssign<Size> for Size {
    }
 }

+impl From<f64> for Size {


i think i only did that for testing in one of the commits but forgot to remove it sorry

juliapaci · 2024-08-15T03:09:19Z

src/vec2.rs

@@ -31,6 +31,9 @@ impl Vec2 {
    /// The vector (0, 0).
    pub const ZERO: Vec2 = Vec2::new(0., 0.);

+    /// The vector (1, 1)


I only used this in one of the prior commits so i can just remove it entirely

juliapaci · 2024-08-15T03:10:50Z

src/ellipse.rs

+    ///
+    /// [`with_rotation`]: Ellipse::with_rotation
+    #[inline]
+    pub fn from_triangle(triangle: Triangle) -> Self {


Yeah you're right

juliapaci · 2024-08-15T03:28:26Z

src/triangle.rs

+    /// empty [`Triangle`] at (1.0, 1.0)
+    pub const ONE: Self = Self::from_coords((1.0, 1.0), (1.0, 1.0), (1.0, 1.0));
+
+    /// equilateral [`Triangle`] identity


haha sorry about that

i think it is useful for the user though (atleast one out of Triangle::Equilateral and Triangle::ONE) because the user can just scale them and work with scalars instead of triangle data.
Not sure if that makes sense but i can defiantly see this as a use case however if you still want it removed then thats fine

also why is Triangle::ZERO alright to keep?

juliapaci · 2024-08-15T03:53:39Z

src/triangle.rs

+        )
+    }
+
+    /// The euclidean distance of each vertex from the centroid


Like alot of these, im not sure why its needed either haha.

juliapaci · 2024-08-15T03:59:07Z

src/triangle.rs

+        (self.b.distance(self.c) * (self.a.y - self.b.y)) / 2.0
+    }
+
+    /// Updates [`Triangle`]'s vertice positions


I'm not convinced this should be in the public API.

yeah im not sure why i did that.

now that the functions that relied upon this method are gone should we still keep this?

juliapaci · 2024-08-15T04:03:03Z

src/triangle.rs

+        self.as_array().iter().find(|&v| v.y == y).copied()
+    }
+
+    /// The topmost vertex of `self` as a [`Point`]


Oh right, their useless now anyway because the organise is useless

juliapaci · 2024-08-15T04:12:00Z

src/triangle.rs

+        area / s
+    }
+
+    /// Expand the triangle by a constant amount (`sizes`) in all directions


Oops i forgot to update this. is 4d8f906 inflate fine?

juliapaci · 2024-08-15T04:12:55Z

src/triangle.rs

+        )
+    }
+
+    /// A new [`Triangle`] with each vertecie's ordinates rounded to the nearest integer


me neither, i just took alot of these helpers from rect

juliapaci · 2024-08-15T08:05:48Z

Ive double checked and it looks like my incircle and circumcircle implementations are correct yet when i check them in desmos.com the incircle radius is too small and the circumcircle radius is too large. Have i done something wrong?

also can the circumcenter function be more efficient?

raphlinus

This is looking pretty good, thanks for all the progress. And apologies again for the slow turnaround on review, I'm juggling too many things. I think not much more is needed to land it.

src/triangle.rs

raphlinus · 2024-08-26T19:26:59Z

src/triangle.rs

+    /// The area of the [`Triangle`]
+    #[inline]
+    pub fn area(&self) -> f64 {
+        0.5 * (self.b - self.a).cross(self.c - self.a).abs()


I disagree that there should be an abs here. We default to signed area in kurbo, and by taking the absolute value we'll have situations where the return value of this function doesn't match the area of converting into shape first.

not exactly sure the use of this cause i dont know of any benefits for signed area but ill just do it if its the norm.

should the area test be updated to be negative now?

Yes, or permute the order of vertices. Converting to shape and evaluating area gives a negative result, so that's confirmation that the sign conventions are consistent. And in fact, you can make a good argument that this should be tested explicitly.

Converting to shape and evaluating area gives a negative result, so that's confirmation that the sign conventions are consistent

but doesn't the shape implementation default to triangles pre-existing signed area function?

no problem though, ive changed to test the original and one with a vertex permutation

src/triangle.rs

juliapaci · 2024-08-27T10:41:05Z

do you think there should be an as_triangle in "shape.rs"?

raphlinus · 2024-08-27T23:25:02Z

do you think there should be an as_triangle in "shape.rs"?

Probably not needed. The main purpose of those methods is fast paths for renderers, and triangle is not (unlike circle, rect, etc) not a common specialized shape - for example, it's not in SVG or the Canvas API.

src/triangle.rs

richard-uk1

I like this PR and think it would make a good addition to kurbo, but I think it needs some changes first. The biggest suggestion in my review is to add functions inscribed_circle and circumscribed_circle and remove the functions that calculate radii/centers for these circles.

Feel free to push back if you disagree with any of my suggestions, and we can have a discussion.

src/point.rs

src/triangle.rs

richard-uk1 · 2024-09-22T18:08:21Z

src/triangle.rs

+    use crate::{Point, Triangle, Vec2};
+
+    fn assert_approx_eq(x: f64, y: f64) {
+        assert!((x - y).abs() < 1e-7);


This is quite a loose bound for f64. I'd use something closer to machine epsilon e.g. 1e-13. Out of scope for this PR, but we really need to standardize this (I like float_cmp because it allows big epsilons as long as the expected and actually are <n representable numbers apart in f64).

I copied the bound from "rect.rs" i understood that it was loose but knew that it was only a temporary solution.

i could change it to be more strict but even with 1e-13, Triangle::area() tests are already failing from precision. i still could arbitrarily restrict the bound or could just wait for it to be standardized as you said, which would you prefer?

Just leave it as-is for now - what you have is fine.

src/triangle.rs

juliapaci

Thanks for the review and sorry for the late response.

All suggestions have been implemented (4f17103, 3da32b7, 4d0afb9) except for those which i further commented on, please see those!

src/point.rs

src/triangle.rs

juliapaci · 2024-09-23T07:18:06Z

src/triangle.rs

+
+    /// The circumcenter of the [`Triangle`]
+    #[inline]
+    pub fn circumcenter(&self) -> Point {


i looked a bit through the compiler explorer for some small optimizations and cleanups but i fear that i am missing a faster algorithm in general. Also any thoughts on this (specifically the first paragraph)?

juliapaci · 2024-09-23T07:30:52Z

src/triangle.rs

+    /// Circumradius of [`Triangle`] (the smallest radius of a circle such that it intercepts each vertex)
+    /// with center [`Triangle::circumcenter`]
+    #[inline]
+    pub fn circumradius(&self) -> f64 {


Yeah i have no problem either way.

The only argument against is that we might be computing extra information...

I think its more likely that the user would want the center and the radius rather than just one of them

src/triangle.rs

juliapaci · 2024-09-23T07:40:26Z

src/triangle.rs

+    use crate::{Point, Triangle, Vec2};
+
+    fn assert_approx_eq(x: f64, y: f64) {
+        assert!((x - y).abs() < 1e-7);


I copied the bound from "rect.rs" i understood that it was loose but knew that it was only a temporary solution.

i could change it to be more strict but even with 1e-13, Triangle::area() tests are already failing from precision. i still could arbitrarily restrict the bound or could just wait for it to be standardized as you said, which would you prefer?

juliapaci · 2024-09-24T06:31:42Z

tests are failing from #350 (comment) and #350 (comment)

fix old tests + add new circle tests

one with and without permutated vertices

`is_zero_area`, `is_nan`/`is_finite` docs, typo

see \linebender#371

wasnt supposed to be committed at all

Co-authored-by: Richard Dodd <richard.o.dodd@gmail.com>

Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>

private `from_coords` and instead of `*radius` use `*circle`

name and doc updates + public `from_coords`

remove `as_array` which hasnt been used since `organise`, add doc alias to `inflate`

DJMcNab · 2024-09-29T12:28:10Z

Fwiw, you should be able to dismiss the review, if we think everything has been addressed.

waywardmonkeys · 2024-10-04T15:50:59Z

@juliapaci If you could fix the tests (maybe look at assert_approx_eq), then I think we can land this. Hopefully @derekdreery can approve from his review comments being addressed.

waywardmonkeys · 2024-10-04T23:27:11Z

I will merge this in about 26-28 hours.

oops. for when we change the bound

I think the changes requested have been addressed.

waywardmonkeys · 2024-10-06T13:28:47Z

I think there are probably things that we could follow up on ... but let's get this landed and deal with those separately.

juliapaci mentioned this pull request May 29, 2024

Arrow shape (WIP) voxell-tech/bevy_motiongfx#49

Closed

juliapaci commented May 30, 2024

View reviewed changes

juliapaci commented Jun 1, 2024

View reviewed changes

juliapaci marked this pull request as ready for review June 1, 2024 22:49

juliapaci force-pushed the triangle branch from 592b2c0 to e40449c Compare June 1, 2024 23:02

juliapaci commented Jun 1, 2024

View reviewed changes

src/triangle.rs Show resolved Hide resolved

juliapaci changed the title ~~add triangle shape (WIP)~~ add triangle shape Jun 1, 2024

waywardmonkeys added this to the August, 2024 Release milestone Aug 6, 2024

raphlinus reviewed Aug 15, 2024

View reviewed changes

juliapaci commented Aug 15, 2024

View reviewed changes

raphlinus reviewed Aug 26, 2024

View reviewed changes

waywardmonkeys reviewed Aug 30, 2024

View reviewed changes

src/triangle.rs Outdated Show resolved Hide resolved

src/triangle.rs Outdated Show resolved Hide resolved

src/triangle.rs Outdated Show resolved Hide resolved

waywardmonkeys mentioned this pull request Aug 30, 2024

Release 0.11.1 #374

Merged

DJMcNab requested a review from richard-uk1 September 10, 2024 08:16

xorgy modified the milestones: 0.11.1, 0.11.2 Sep 14, 2024

richard-uk1 previously requested changes Sep 22, 2024

View reviewed changes

juliapaci commented Sep 23, 2024

View reviewed changes

DJMcNab requested a review from richard-uk1 September 27, 2024 08:39

juliapaci force-pushed the triangle branch from 1e5e985 to 3d331fb Compare September 27, 2024 23:16

juliapaci and others added 18 commits September 29, 2024 09:20

working organise\(\)

276076d

proper ellipse size

7358faa

fix ci

0532dc9

chore: format

1126a0f

apply suggestions

8f0e1a8

tests

e1cce3d

fix old tests + add new circle tests

apply cleanup suggestions

0542c60

signed area test

2124dcc

one with and without permutated vertices

match new style

ff39cee

`is_zero_area`, `is_nan`/`is_finite` docs, typo

add is_zero_area doc alias

7e07c00

see \linebender#371

remove from Line

6b10318

wasnt supposed to be committed at all

Apply explicit suggestions from derekdreery

913f0f4

Co-authored-by: Richard Dodd <richard.o.dodd@gmail.com>

remove point comparisons

3670f6e

Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>

apply implicit suggestions

84b0609

private `from_coords` and instead of `*radius` use `*circle`

meta cleanup

1f4e1ad

name and doc updates + public `from_coords`

cleanup

c5dc9fb

remove `as_array` which hasnt been used since `organise`, add doc alias to `inflate`

little doc cleanup

452b296

end comments with a period

bebbb78

juliapaci force-pushed the triangle branch from 3d331fb to bebbb78 Compare September 28, 2024 23:27

fix tests

99d9c91

use assert_approx_eq for point test

ca336fc

oops. for when we change the bound

waywardmonkeys approved these changes Oct 6, 2024

View reviewed changes

waywardmonkeys added this pull request to the merge queue Oct 6, 2024

Merged via the queue into linebender:main with commit 5a4b2d9 Oct 6, 2024
15 checks passed

add triangle shape #350

add triangle shape #350

Conversation

juliapaci commented May 29, 2024 • edited Loading

juliapaci commented May 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliapaci commented Jun 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliapaci commented Jun 1, 2024

juliapaci commented Jun 5, 2024

raphlinus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliapaci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliapaci commented Aug 15, 2024

raphlinus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliapaci commented Aug 27, 2024

raphlinus commented Aug 27, 2024

richard-uk1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliapaci left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliapaci commented Sep 24, 2024

DJMcNab commented Sep 29, 2024

waywardmonkeys commented Oct 4, 2024

waywardmonkeys commented Oct 4, 2024

waywardmonkeys commented Oct 6, 2024

juliapaci commented May 29, 2024 •

edited

Loading

juliapaci left a comment •

edited

Loading