Skip to content

chore(rstar): Get rid of unnecessary trait bounds - #248

Merged
adamreichold merged 1 commit into
georust:masterfrom
fogti:reduce-bounds
Sep 11, 2026
Merged

adamreichold merged 1 commit into
georust:masterfrom
fogti:reduce-bounds

Conversation

@fogti

@fogti fogti commented Sep 10, 2026

Copy link
Copy Markdown
Contributor
  • I agree to follow the project's code of conduct.
  • I added an entry to rstar/CHANGELOG.md if knowledge of this change could be valuable to users.

This has the same motivation as #247 (but is orthogonal/independent to that), particularly that these trait bounds "bleed" into downstream usage, e.g. in https://docs.rs/polygon_unionfind and https://github.com/mikwielgus/anyangle .

#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Default)]
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
pub struct GeomWithData<R: RTreeObject, T> {
pub struct GeomWithData<R, T> {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is probably the most important one because this is a massive obstacle in downstream usage.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So that I can better understand, can you explain or better yet, link to an example of the "massive obstacle in downstream usage"?

I did peruse https://stackoverflow.com/questions/49229332/should-trait-bounds-be-duplicated-in-struct-and-impl/66369912#66369912 and I don't disagree, but my understanding is it's more so "kind of annoying" as opposed to preventing you from doing something. I suspect I'm missing some context!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It pretty much breaks every place where you want to properly abstract over something involved with RTree without having to drag this association everywhere, e.g. into https://github.com/mikwielgus/anyangle/blob/bf06c7d03f16f995f95a7b44e9f3f9d91a2eb31f/src/mlfa/navmesher.rs#L16

and as soon as multiple crates do this, everything gets quickly bogged down with massive trait bounds, making it harder to understand what trait bounds are necessary and why, and probably at some point also slower to compile (because every trait bound has to be checked).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Like sure, it usually doesn't prevent one from doing something (and if one really wants to force-avoid a bound that would be introduced that way, one can still hide it away using Box<dyn _> and hide all of the stuff behind a vtable that doesn't bleed the bound), except break API stability very broadly when one wants to swap out the backend (one doesn't always need an RTree…) for some operations because the bounds spill everywhere and have to be swapped everywhere. It makes maintenance of code harder with barely or no benefit at all.

Particularly in regard to this current PR, I really see no point in having these bounds, they just make usage more difficult (e.g. by necessitating users to have isomorphic structures to these wrappers laying around + From/Into implementations just to make sure that API boundaries are clean).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok, so in the linked example, after these changes, your code could become:

- pub struct Navmesher<K: RTreeNum, N, D> {
+ pub struct Navmesher<K, N, D> {
    laminate: Laminate<K, PolygonWithData<K, D>>,
    navmeshes: Vec<N>,
}

That's what we're talking about? Based on the level of rhetoric ("bogged", "breaking" "massive"), I thought I was misunderstanding something. Or maybe there's a better example out there.

To be clear, I think it's great to make our library easier to use, and am in favor. I'm a little ambivalent about introducing a second level of generics as in #247, which makes it less of a "clear win" from my perspective, but like I said, I'm ambivalent, not opposed if other maintainers are into it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes. especially because all the trait bounds quickly accumulate (as can be seen in the mentioned crates, albeit there workarounds, like not using GeomWithData even when appropriate, were already applied).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the rhetoric, I'm exhausted from having to thread relatively small improvements and dependency bumps + API breaks through a lot of small crates.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm exhausted from having to thread relatively small improvements and dependency bumps + API breaks through a lot of small crates.

Maybe that helps: I think is the really important work in FOSS.

Implementing something in a free-standing manner is usually straight-forward enough, but even if it is very innovative, only when it becomes part of the larger ecosystem can it really make an impact. And those dependencies and API contracts are the technical reification of the human relationships we as the FOSS community maintain which actually make this into an ecosystem instead of just a bunch of code bases.

Comment thread rstar/src/point.rs Outdated

@adamreichold adamreichold left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In contrast to #247, these make sense to me as they make life easier for downstream users while not changing anything about how the types work.

@adamreichold
adamreichold added this pull request to the merge queue Sep 11, 2026
@adamreichold

Copy link
Copy Markdown
Member

Thanks!

Merged via the queue into georust:master with commit 8a0f397 Sep 11, 2026
6 checks passed
@fogti
fogti deleted the reduce-bounds branch September 11, 2026 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants