Add TriangulateEarcut algorithm.
#1007
Conversation
frewsxcv
force-pushed
the
frewsxcv/triangulate
branch
from
d7a4a64 to
8722d72
Compare
frewsxcv
force-pushed
the
frewsxcv/triangulate
branch
from
8722d72 to
99d5b23
Compare
| impl<T: CoordFloat> Iter<T> { | ||
| fn triangle_index_to_coord(&self, triangle_index: usize) -> crate::Coord<T> { | ||
| coord! { | ||
| x: self.0.vertices[triangle_index * 2], |
There was a problem hiding this comment.
Here you use vertices elsewhere you use vertexes, can you consolidate around vertices which is used elsewhere in `geo?
|
|
||
| /// The raw result of triangulating a polygon from `earcutr`. | ||
| #[derive(Debug, PartialEq, Clone)] | ||
| pub struct Raw<T: CoordFloat> { |
There was a problem hiding this comment.
Since this is public, I feel like a more descriptive name is in order. Maybe Triangulation ?
There was a problem hiding this comment.
Perhaps worth bearing in mind that if we went with Triangulation it would also have to be suitable for any future triangulations (e.g. Delaunay) we might add.
There was a problem hiding this comment.
This is namespaced to the module, so triangulate_earcut::Triangulation v.s. the hypothetical delaunay::Triangulation. I think it'd be fine.
That said, I'm also fine with the clarity/verboseness tradeoff of triangulate_earcut::EarcutTriangulation
I just think Raw is a bit inexplicable.
There was a problem hiding this comment.
There is a case to be made for triangulate_earcut::Triangulation, but I wonder if we should have a larger discussion about naming and namespacing to be consistent with our other traits. For example we also don't do geo::euclidean_distance::Distance nor geo::geodeisc_length::Length.
There was a problem hiding this comment.
geo::euclidean_distance::Distance nor geo::geodeisc_length::Length
I don't understand the comparison to EuclideanDistance and GeodesicLength which both return scalars. We don't currently have bespoke types for those operations.
We could introduce something like:
mod euclidean_distance {
+ struct Distance<T: CoordNum>(T);
trait EuclideanDistance<T> {
- fn euclidean_distance(&self) -> T;
+ fn euclidean_distance(&self) -> Distance<T>;
}
}Is that what you mean? I don't see the value in it, but maybe I'm missing something.
In the case of this triangulation module, we already have this bespoke type: Raw, I'm saying we should give it a more descriptive name than "Raw".
There was a problem hiding this comment.
Disregard my previous comment! I interpreted your message:
This is namespaced to the module, so
triangulate_earcut::Triangulation...
to mean that you were suggesting renaming the TriangulateEarcut trait to Triangulation and forgot this was a discussion about the struct that is being returned.
There was a problem hiding this comment.
Ah, right! Sorry for not being clearer from the get-go. I probably should have just led with:
| pub struct Raw<T: CoordFloat> { | |
| pub struct Triangulation<T: CoordFloat> { |
|
|
||
| fn flat_line_string_coords_2<T: CoordFloat>( | ||
| line_string: &crate::LineString<T>, | ||
| vertexes: &mut Vec<T>, |
There was a problem hiding this comment.
This "vertex" naming confused me a bit. It's not actually a Vec of vertices, it's a vec of the flattened scalars of the vertices. flattened_vertex_scalars is kind of a mouthful though.
I can't think of anything better, so no change requested unless you can think of something clearer.
Co-authored-by: Michael Kirk <michael.code@endoftheworl.de>
|
bors r=michaelkirk |
|
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
CHANGES.mdif knowledge of this change could be valuable to users.#1002