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

Added Ellipsoid and Capsule shapes to TPE #203

Merged
merged 12 commits into from
Jun 25, 2021

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Jan 22, 2021

Added ellipsoid and capsule to TPE. This PR builds on top of this other PR #202

Related with #196 and #195

It's a draft for now, I need to include some more tests.

Signed-off-by: ahcorde [email protected]

@ahcorde ahcorde added the TPE Trivial Physics Engine label Jan 22, 2021
@ahcorde ahcorde self-assigned this Jan 22, 2021
Base automatically changed from ahcorde/dartsim/ellipsoid_capsule to main March 24, 2021 05:02
@ahcorde ahcorde force-pushed the ahcorde/tpe/ellipsoid_capsule branch from 088722e to 6c4336f Compare April 12, 2021 20:20
@ahcorde ahcorde changed the base branch from main to ign-physics4 April 12, 2021 20:21
ahcorde added 2 commits April 12, 2021 22:26
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
@ahcorde ahcorde marked this pull request as ready for review April 12, 2021 20:45
@ahcorde ahcorde requested a review from mxgrey as a code owner April 12, 2021 20:45
@ahcorde
Copy link
Contributor Author

ahcorde commented Apr 12, 2021

I opened this PR for review, I can add more tests if required

@codecov
Copy link

codecov bot commented Apr 12, 2021

Codecov Report

Merging #203 (d9433ea) into ign-physics4 (0860af0) will increase coverage by 0.39%.
The diff coverage is 87.59%.

❗ Current head d9433ea differs from pull request most recent head a001912. Consider uploading reports for the commit a001912 to get more accurate results
Impacted file tree graph

@@               Coverage Diff                @@
##           ign-physics4     #203      +/-   ##
================================================
+ Coverage         74.86%   75.26%   +0.39%     
================================================
  Files               118      122       +4     
  Lines              5228     5376     +148     
================================================
+ Hits               3914     4046     +132     
- Misses             1314     1330      +16     
Impacted Files Coverage Δ
tpe/lib/src/Shape.cc 81.70% <81.63%> (-0.04%) ⬇️
tpe/plugin/src/ShapeFeatures.cc 72.10% <88.52%> (+7.76%) ⬆️
tpe/lib/src/Collision.cc 84.31% <100.00%> (+2.09%) ⬆️
tpe/lib/src/Shape.hh 100.00% <100.00%> (ø)
tpe/plugin/src/SDFFeatures.cc 84.66% <100.00%> (+1.10%) ⬆️
include/ignition/physics/detail/CapsuleShape.hh 100.00% <0.00%> (ø)
include/ignition/physics/EllipsoidShape.hh 100.00% <0.00%> (ø)
include/ignition/physics/detail/EllipsoidShape.hh 100.00% <0.00%> (ø)
include/ignition/physics/CapsuleShape.hh 100.00% <0.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 0860af0...a001912. Read the comment docs.

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

It's a draft for now, I need to include some more tests.

I wasn't sure about the status of this PR, so I went ahead and gave it a quick review. It's looking good to me, I think it would be nice to add tests to CollisionDetector_TEST.


For future reference, I think it's also worth mentioning that TPE pretty much treats all shapes as boxes for the purpose of collision checking, because it only checks their AABBs. But Having these shapes supported means that worlds using complex shapes with other engines will work reasonably with TPE too.

tpe/lib/src/Shape.hh Outdated Show resolved Hide resolved
tpe/plugin/worlds/shapes.world Show resolved Hide resolved
@ahcorde ahcorde requested a review from chapulina June 18, 2021 10:58
@chapulina chapulina added the 🏢 edifice Ignition Edifice label Jun 22, 2021
@chapulina
Copy link
Contributor

The Homebrew and Ubuntu warnings are not introduced by this PR, but the Windows warnings are

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

ahcorde commented Jun 23, 2021

@chapulina fixed windows warnings

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

LGTM, I just have some minor final comments.

tpe/lib/src/CollisionDetector_TEST.cc Show resolved Hide resolved
tpe/plugin/src/ShapeFeatures.cc Outdated Show resolved Hide resolved
Signed-off-by: ahcorde <[email protected]>
@ahcorde ahcorde requested a review from chapulina June 25, 2021 07:33
@chapulina chapulina merged commit a20dbba into ign-physics4 Jun 25, 2021
@chapulina chapulina deleted the ahcorde/tpe/ellipsoid_capsule branch June 25, 2021 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏢 edifice Ignition Edifice TPE Trivial Physics Engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants