Introduce num_coords method on all geometry types.

[frewsxcv]

I'm going to use these methods in a project to preallocate vectors of
coordinates.

• I agree to follow the project's code of conduct.
• I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

---

[michaelkirk]

This LGTM, but depending on how it's going to be used - I wonder if it'd make sense to tie the implementation and the documentation closer to CoordsIter - like:

pub trait CoordsIter<'a, T: CoordinateType> {
    ...
    fn coords_iter(&'a self) -> Self::Iter;
+   fn coords_count(&self) -> usize {
+     self.coords_iter().count()
+   }
}

[michaelkirk]

Hm... I'm curious to see how this is used! Is there a commit you can point me to?

[michaelkirk]

tie the implementation and the documentation closer to CoordsIter

From a perf perspective, self.coords_iter().count is O(n)... so probably we want to avoid that. But my question was moreso about discoverability - I had a hunch that you were using this in tandem with CoordsIter, and if that's going to be the common case, then maybe it makes sense to have them in the same trait, even if not implemented exactly as I showed in my example.

[frewsxcv]

Hm... I'm curious to see how this is used! Is there a commit you can point me to?

For earcutr, you need to create a flat coordinate Vec buffer as input, and here's where I'm currently using it. For the time being, I only need need to know the number of coordinates in polygons, but I thought it could be useful to have this functionality on all geometry types.

From a perf perspective, self.coords_iter().count is O(n)... so probably we want to avoid that. But my question was moreso about discoverability - I had a hunch that you were using this in tandem with CoordsIter, and if that's going to be the common case, then maybe it makes sense to have them in the same trait, even if not implemented exactly as I showed in my example.

Adding them to the CoordsIter sounds good to me. Would it be crazy to rename the CoordsIter to just Coords?

[michaelkirk]

Thanks for sharing the example!

fn polygon_to_earcutr_input(polygon: &geo::Polygon<f64>) -> bevy_earcutr::EarcutrInput {
    let mut vertices = Vec::with_capacity(polygon_num_coords(polygon) * 2);
    let mut interior_indices = Vec::with_capacity(polygon.interiors().len());

    flat_line_string_coords_2(polygon.exterior(), &mut vertices);

    for interior in polygon.interiors() {
        interior_indices.push(vertices.len() / 2);
        flat_line_string_coords_2(interior, &mut vertices);
    }

    bevy_earcutr::EarcutrInput {
        vertices,
        interior_indices,
    }
}

I was incorrectly assuming you were doing something simpler like:

fn polygon_to_earcutr_input(polygon: &geo::Polygon<f64>) -> bevy_earcutr::EarcutrInput {
    let mut vertices = Vec::with_capacity(polygon.num_coords() * 2);       
    for coord in polygon.coords_iter() {
        vertices.push(coord.x)
        vertices.push(coord.y)
    }
    vertices
}

I didn't consider that you need to iterate them separately for the inner line strings, so my initial argument is somewhat weakened. That said, I still have a slight preference for combining related traits for the purposes of discoverability, but up to you.

[frewsxcv]

Going to move this to the CoordsIter trait now

[frewsxcv]

Okay, it's been moved to CoordsIter and ready for a re-review!

[michaelkirk]

It seems unfortunate to have both a coords_count method and a num_coords method - especially since num_coords doesn't exist for most geometries. I can easily imagine getting them confused and annoyed as I reach for it and it's only sometimes there.

What would you think of either:

1. renaming coords_count to num_coords
   -or-
2. leaving them as coords_count but deprecating num_coords

[michaelkirk]

Does _count imply some O(n) operation? 🤔

[frewsxcv]

oh i meant to change this to line_string.coords_count() within the closure on this line. i didn't want to remove num_coords because that would be a geo-types breaking change and i didn't want to deal with that. the standard library uses count and count_*, but not num_, so that's why i changed it (along with your earlier suggestion). so i think i'm slightly in favor of your second option.

Does _count imply some O(n) operation? 🤔

i don't think so! the default Iterator#count implementation is O(n), but many iterators overwrite it to be O(1), like std::vec::IntoIter (e.g. vec.into_iter().count())

[michaelkirk]

ok! That sounds good to me. This all LGTM. Thanks for you patience with my review. 😬

[michaelkirk]

Unrelated, but it occurred to me that this is why we have a map_coords_in_place method which takes a function rather than a coords_iter_mut(). It wouldn't make sense to return 4 mutable coordinates for a Rect. And it would be very surprising if coords_iter returned 4 coords while coords_iter_mut returned 2 coords.

[frewsxcv]

this is a very good point. the change in the pull request (returning 4) matches the coords_iter implementation above. what do you think about us opening a separate issue for this inconsistency? or do you feel like it blocks this pull request?

[michaelkirk]

My comment was just referring to an earlier request I'd made long ago that we should add a coords_iter_mut(). But now I see that it wouldn't be reasonable to do so.

I think it makes sense that coords_iter iterates all the coords from the polygon representation of the rect and I definitely think it makes sense that coords_count is consistent with that.

So, I don't see any issue. Certainly nothing blocking this PR - sorry for the confusion.

[frewsxcv]

bors r=michaelkirk

[bors]

Build succeeded:

• geo-types
• coverage
• geo-postgis
• geo

[vipera]

My crate depends on geo-types and with this change I'm losing the ability to count the number of points in a polygon without depending on geo due to this functionality being moved to geo's algorithms module.

I think that perhaps geometries such as point, line and polygon that have sizes known in advance - or determinable in O(1) time due to some underlying data structure (Vec) having that information - ought to have public methods that allow their sizes to be queried as part of geo-types's API itself, as I perceive that to be a property of the type.

In that case, geo::algorithm's implementation could use that property if it is available on the type having its points counted, and calculate it in an appropriate way (sum of several properties) for the more complex types.

Maybe my perception is just wrong, so I wonder if anyone has any other thoughts on the matter 🤔

[michaelkirk]

Hi @vipera - Specifically you're running into the functioning but deprecated line_string.num_coords() right? Or is something currently broken for you?

The reason we go through the effort of marking things as deprecated like this is so that we can hopefully have conversations before we really break things for people.

I perceive that to be a property of the type.

geo-types is primarily used as an interchange format - e.g. serialization format crates like json and wkt can output geo-types. Algorithm crates like geo, proj, and polyline accept geo-types as input.

So, the goal of geo-types is not to encapsulate all possible "properties of the types", but rather to implement a minimal set of functionality that allows other geometry crates to interoperate.

Said differently: If something can go into geo rather than geo-types, then it usually should. Adding functionality to geo-types itself should be somewhat of a last resort.

That said, all rules have exceptions, and at the end of the day we're trying to balance tradeoffs that work well for the most people. I agree that num_coords is towards one end of a spectrum.

Can you say more about what exactly you're trying to do?

Would you be able to do linestring.0.len() where you're currently calling line_string.num_coords()?

For the sake of my understanding, is there a reason you don't use the geo crate?

[vipera]

Hi @michaelkirk, yes line_string.num_coords() and its deprecation is what I was referring to. In fact, I apologize that I didn't outline the scope and purpose of my comment to avoid confusion.

So, to clarify, I'm in no immediate peril from this change and I understood the deprecation to be a call for dialog for anyone uncertain or in disagreement with the intent.

Your comment gave me some better perspective on the intended organization of geo vs geo-types, so I can say the change makes much more sense to me now, even ignoring the much more important point -- I completely missed that linestring.0 is public. I guess I just never expected the internals to be out in the open like that! In any case, that fact completely voids my idea that information previously available through geo-types has been lost.

I'm defining new operations for types from geo-types, but would have no particular qualm with depending on geo instead, except that accessing the number of points seemed much too trivial a thing to warrant such a change, so I was wondering if it was an oversight. But it turns out that linestring.0.len() is its slightly surprising replacement.