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

[Citadel] [TPE] Add function to get an Entity's bounding box #59

Merged
merged 5 commits into from
Jun 3, 2020

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented May 29, 2020

Add helper function that returns an entity's bounding box by recursively merging it's children's bounding boxes.

@iche033 iche033 requested a review from mxgrey as a code owner May 29, 2020 00:42
@github-actions github-actions bot added 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome labels May 29, 2020
@codecov
Copy link

codecov bot commented May 29, 2020

Codecov Report

Merging #59 into ign-physics2 will increase coverage by 0.27%.
The diff coverage is 98.27%.

Impacted file tree graph

@@               Coverage Diff                @@
##           ign-physics2      #59      +/-   ##
================================================
+ Coverage         81.53%   81.81%   +0.27%     
================================================
  Files               103      104       +1     
  Lines              3434     3492      +58     
================================================
+ Hits               2800     2857      +57     
- Misses              634      635       +1     
Impacted Files Coverage Δ
tpe/lib/src/Entity.hh 0.00% <ø> (ø)
tpe/lib/src/Collision.cc 76.31% <75.00%> (-0.16%) ⬇️
tpe/lib/src/Entity.cc 83.33% <100.00%> (+3.33%) ⬆️
tpe/lib/src/Utils.cc 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09773fb...96d6445. Read the comment docs.

Copy link
Contributor

@claireyywang claireyywang left a comment

Choose a reason for hiding this comment

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

LGTM in general with one minor comment. Also it looks like it's having the same link error LNK2001 and LNK1120 on windows again.

namespace tpelib {

//////////////////////////////////////////////////
math::AxisAlignedBox transformAxisAlignedBox(
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add more documentation on the math behind this function? I find it a bit hard to understand by just reading the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added more doc and testing linker fix in 558cbb7

Copy link
Contributor

@claireyywang claireyywang left a comment

Choose a reason for hiding this comment

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

Thanks for adding documentation, LGTM! The CI error seems to be about dartsim and unrelated to this PR.

Signed-off-by: Ian Chen <[email protected]>
@iche033
Copy link
Contributor Author

iche033 commented Jun 2, 2020

all checks passed, merging!

actually.. need to merge with with ign-physics2 and wait for one more round of CI"

@iche033 iche033 merged commit 6511872 into ign-physics2 Jun 3, 2020
@iche033 iche033 deleted the entity_bbox branch June 3, 2020 01:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel 🔮 dome Ignition Dome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants