Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for AxisAlignedBoundingBox clone method #6183

Merged

Conversation

hanbollar
Copy link

before the fix - if result was not defined returned constructed AxisAlignedBoundingBox without copying over center value.

The issue with this is that if the user, for their own purposes, constructed a center value with an offset s.t. the minimum and maximum weren't the same distance away from the center parameter, the clone method would not consider this form of construction and just solve for the default center based on max and min values.

Additionally, since the minimum and maximum values are defined in relation to the center (ie as if the center was the origin for them) the clone's box's overall position doesnt match that of the box it's cloning.

@cesium-concierge
Copy link

Signed CLA is on file.

@hanbollar, thanks for the pull request! Maintainers, we have a signed CLA from @hanbollar, so you can review this at any time.


I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@hpinkos
Copy link
Contributor

hpinkos commented Feb 2, 2018

Good catch @hanbollar!

I think we prefer this fix though

if (!defined(result)) {
    return new AxisAlignedBoundingBox(box.minimum, box.maximum, box.center);
}

@hpinkos
Copy link
Contributor

hpinkos commented Feb 2, 2018

Also, please add a unit test

@hanbollar
Copy link
Author

@hpinkos updated with your recommendations - lmk if there's anything else you think I should do before it can merge. Thanks!

@hpinkos
Copy link
Contributor

hpinkos commented Feb 6, 2018

Perfect, thanks @hanbollar!

@hpinkos hpinkos merged commit fd7fbfd into CesiumGS:master Feb 6, 2018
@hanbollar hanbollar deleted the fix-AxisAlignedBoundingBox-clone-method branch February 6, 2018 22:37
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