Skip to content

Avoid unclear diameter terminology in AABB::from_center.#223

Open
adamreichold wants to merge 1 commit intomasterfrom
clarify-from-center
Open

Avoid unclear diameter terminology in AABB::from_center.#223
adamreichold wants to merge 1 commit intomasterfrom
clarify-from-center

Conversation

@adamreichold
Copy link
Member

  • 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.

@adamreichold adamreichold requested a review from michaelkirk March 2, 2026 11:46
@adamreichold
Copy link
Member Author

Trying to address #222 (comment)

Copy link
Member

@michaelkirk michaelkirk left a comment

Choose a reason for hiding this comment

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

I was expecting "width". I guess it's a little funny because width == height.

To my (US_en) eye, "distance" is more or less synonymous with diameter.

In particular: the distance from the center to the edge is not the same as the distance from the center to the corner.

Is there another popular rectangle API that uses this "distance" term? Everything I find is either min/max points or origin,width,height.

I can't find another implementation of our funny width==height constructor.

@adamreichold
Copy link
Member Author

My idea behind the choice of "distance" was that this is the smallest AABB which includes all points within the given distance of the center? (This should even be true no matter which metric on R^n is chosen, shouldn't it?)

However, I am personally not particularly invested in any terminology and will happily change this if you have a clear favourite? (Or anybody else for that matter.)

To my (US_en) eye, "distance" is more or less synonymous with diameter.

At least to me, "distance" would be synonymous to radius instead of diameter.

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.

2 participants