generic geom + data container

[urschrei]

• [x ] I agree to follow the project's code of conduct.

• [x] I added an entry to rstar/CHANGELOG.md if knowledge of this change could be valuable to users.

This is a generic implementation of PointWithData and #73. All fields are private and require getters, and The geometry field can't be modified after creation in order to prevent a logic error if it's in an RTree. This PR also adds a deprecation notice to PointWithData.

For discussion:

• The name
• struct fields
• Use of getters
• What should we be deriving?
• Can we derive serde traits?
• Is there any need for a Default on either field?

[urschrei]

@rmanoka @adamreichold @jblindsay please have a look and discuss.

[urschrei]

One aspect of this I don't like is that because we impl RTreeObject on GeomWithData the "parent" struct has one set of methods for calculating distance / containment / etc, but you have to go through the setter for anything specific (e.g. position, project_point). It's not terrible, and we have a compiler to help out, but it's not the cleanest design.

[rmanoka]

Nice implementation @urschrei . I like that we're making the geom immutable (so only getter).

Regarding derives: I think we should derive as many standard traits as possible as the users can anyway not implement them on this type. So, Debug, Clone, Copy, PartialEq, Eq, Default. Note that in all these cases, the derive macro only derives assuming both T, R also implement the trait which makes sense. PartialEq is especially useful as it provides RTree::contains

PartialOrd and Ord: I suppose most users would expect to have it sorted by the ordering of the geometry. So, we should ensure that we declare the geom before the data and derive these so that the lexicographic ordering is intuitive.

Hash also seems like a standard trait that people like to derive. Would be good to derive serde traits under the same feature gate as with RTree (as otherwise RTree containing it can't be serialized)

[adamreichold]

@rmanoka Considering the derives, I wonder we should actually not use the default derives but implement this manually to be restricted to the geom field and ignore the data field. This way RTree::contains/remove would work based solely on the geometry ignoring the associated data. I think there are use cases for both approaches.

[urschrei]

Thanks both, superficial comments are all addressed. @adamreichold your suggestion to restrict the derives to the geom field only makes sense to me. Can you and @rmanoka:

1. sketch out which derives you want, and where, to make sure I haven't misunderstood you;
2. make a decision about which option to go with.

And then let's try to get this merged – if only because I feel bad about pushing James for a PR only to veer off in a (better, IMO) direction.

[rmanoka]

@rmanoka Considering the derives, I wonder we should actually not use the default derives but implement this manually to be restricted to the geom field and ignore the data field. This way RTree::contains/remove would work based solely on the geometry ignoring the associated data. I think there are use cases for both approaches.

I suppose you are talking specifically about PartialEq and Eq? I think the natural definition of these traits is to use both fields. Further, if the user wants to check contains / remove with just the geometry, they could do it via the nearest_neighbor and pop_nearest_neighbor methods anyway. Also, note that PointWithData currently follows this semantic, and directly derives all these traits

[Stoeoef]

In regards to the derive discussion: I would also suggest to derive as much as possible and include both data and the geometry. That's usually what PartialEq is supposed to do, steering away from that would potentially be more confusing IMO.

There is also a simple workaround should you need a more specialized contains method by calling rtree.locate_in_envelop(some_aabb).filter(some_special_filter).

[urschrei]

• Temp test removed
• Order flipped so geom precedes data, as implied by the name
• Derives same as PointWithData, serde derives are gated.
• Changelog updated
• Commits squashed

[urschrei]

I found some more minor doc nits, and squashed everything. I'm going to go ahead and merge then release 0.9.1 (because this was a feature request, and I don't think #67 is close to landing yet) tomorrow morning (UTC) sometime. Thanks for your input, everyone.

[urschrei]

Bors r+

[bors[bot]]

Build succeeded:

• ci result