Revert back to min/max representation of empty AABB #184
Conversation
|
This would be my personally preferred alternative to #181 and #182 trying to keep #161 working. Generally speaking, I am really unsure whether the problems caused by fixed-precision integer coordinates are worth it, but this fix seems to be rather targeted and should not add measurable costs to the majority of floating-point coordinate users. |
adamreichold
force-pushed
the
new-empty-min-max
branch
from
8a2820f
to
4c16830
Compare
|
(Whatever choice we make, we should probably yank 0.12.1 after releasing 0.12.2 to crates.io.) |
This reverts back to the min/max representation of empty AABB due to regressions introduced by the numerically more tame but not invariant for merging representation. To avoid regressing #161, it also adds a single check to avoid computing the distance to an empty envelope (of the root node of an empty tree). Integer coordinates are always prone to overflow but in the empty case we are forcing this onto the caller whereas every non-empty tree will have AABB that the caller supplied and where we can reasonably ask them to control for overflow or use a custom `Point` impl based on saturating arithmetic.
adamreichold
force-pushed
the
new-empty-min-max
branch
from
4c16830
to
20014b7
Compare
|
OK – shall we merge this, release a patch version, and yank? |
If there are no objections from elsewhere, yes, I think that would be best to provide a quick resolution for affected projects. I think the risk of an error in the fix is small for most users (as we are going back to the previous behaviour) and we'll just have to deal with any fallout from fixed-precision integer coordinate users. I tried to avoid it but I am admittedly not confident that it can be avoided. |
|
0.12.1 is yanked |
I think it is customary to yank only after an upgrade is available. Otherwise, the degree of eyebrow raising can be problematic. But I suspect you'll push 0.12.2 soon as well? |
|
Released! |
At first I thought this made sense, but now I'm not sure. Assuming the fix is patch compatible (0.0.x++), if you wait to have the fix published before running 
If you already have the patch-compatible version published, So it can be used to prevent people from updating. But it's not useful for "skipping" the broken one, since cargo already installs the most recent semver compatible one when running |
|
To summarize, it's my conclusion that |
|
Delightfully, we can unyank. |
|
A lot of CI runs tools like |
I would not recommend that due to as written, |
I phrased this poorly — In case there was confusion, I'm not recommending I'll try again: You should yank as soon as you know there is a problem, to keep people from updating to the broken version, even before a fix is available, ensuring legacy users stay on the old version until the fix is available.
As @adamreichold points out, there is some utility to yanking, even if you've already shipped the fix. |
rstar/CHANGELOG.mdif knowledge of this change could be valuable to users.This reverts back to the min/max representation of empty AABB due to regressions introduced by the numerically more tame but not invariant for merging representation.
To avoid regressing #161, it also adds a single check to avoid computing the distance to an empty envelope (of the root node of an empty tree). Integer coordinates are always prone to overflow but in the empty case we are forcing this onto the caller whereas every non-empty tree will have AABB that the caller supplied and where we can reasonably ask them to control for overflow or use a custom
Pointimpl based on saturating arithmetic.Fixes #183