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 TPE poseDirty getter #182

Merged
merged 3 commits into from
Dec 28, 2020
Merged

Fix TPE poseDirty getter #182

merged 3 commits into from
Dec 28, 2020

Conversation

luca-della-vedova
Copy link
Member

Classic = vs == typo that wasn't caught at compile time because of the pimpl's raw pointer not enforcing const correctness.
It disabled the optimization that skipped updating the AABB tree for entities that didn't move, so the PR should help improve performance.

Signed-off-by: Luca Della Vedova <luca@openrobotics.org>
@mxgrey
Copy link
Contributor

mxgrey commented Dec 21, 2020

I think it's worth mentioning that there's no use for PIMPL in an ignition-physics physics engine plugin anyway.

We already get an API+ABI firewall for this library by loading it using ignition-plugin, so PIMPL doesn't accomplish anything.

@iche033
Copy link
Contributor

iche033 commented Dec 21, 2020

I think we did pimpl at the beginning because there was an idea that the TPE library (in tpe/lib) would live outside of ign-physics or could be migrated later if needed. But that's no longer the case as we're now committing to having the library inside ign-phyics so we could just stop using pimpl in future classes

Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

bad mistake, thanks for catching this!

tpe/lib/src/Entity.cc Outdated Show resolved Hide resolved
@chapulina
Copy link
Contributor

there was an idea that the TPE library (in tpe/lib) would live outside of ign-physics or could be migrated later if needed. But that's no longer the case as we're now committing to having the library inside ign-phyics

Is anything is TPE so specific to Ignition Physics that it couldn't be used on its own?

@iche033
Copy link
Contributor

iche033 commented Dec 21, 2020

Is anything is TPE so specific to Ignition Physics that it couldn't be used on its own?

nope, TPE is not specific to ign-physics. It's designed to be a standalone library.

@scpeters
Copy link
Member

fix for homebrew CI problems in gazebo-tooling/release-tools#366

luca-della-vedova and others added 2 commits December 24, 2020 09:59
Signed-off-by: Luca Della Vedova <luca@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
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.

I took the liberty of addressing the codechecker warning on a4faa4b so we don't need a new PR, then a merge into this PR, etc.

LGTM with happy CI.

@codecov
Copy link

codecov bot commented Dec 24, 2020

Codecov Report

Merging #182 (a4faa4b) into ign-physics2 (3f5a8a1) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##           ign-physics2     #182   +/-   ##
=============================================
  Coverage         83.10%   83.10%           
=============================================
  Files               106      106           
  Lines              3983     3983           
=============================================
  Hits               3310     3310           
  Misses              673      673           
Impacted Files Coverage Δ
dartsim/src/EntityManagementFeatures.cc 73.41% <ø> (ø)
tpe/lib/src/Entity.cc 86.50% <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 3f5a8a1...a4faa4b. Read the comment docs.

@chapulina chapulina merged commit 41926bf into ign-physics2 Dec 28, 2020
@chapulina chapulina deleted the luca/fix_tpe_getter branch December 28, 2020 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants