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

Buoyancy #252

Merged
merged 50 commits into from
Aug 17, 2020
Merged

Buoyancy #252

merged 50 commits into from
Aug 17, 2020

Conversation

nkoenig
Copy link
Contributor

@nkoenig nkoenig commented Jul 16, 2020

Redo of the buoyancy PR. Original is here.

This will resolve issue #159.

@nkoenig nkoenig requested a review from chapulina as a code owner July 16, 2020 20:56
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Jul 16, 2020
Signed-off-by: Nate Koenig <[email protected]>
@codecov
Copy link

codecov bot commented Jul 16, 2020

Codecov Report

Merging #252 into ign-gazebo3 will decrease coverage by 0.75%.
The diff coverage is 24.63%.

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-gazebo3     #252      +/-   ##
===============================================
- Coverage        65.71%   64.95%   -0.76%     
===============================================
  Files              127      131       +4     
  Lines             6238     6351     +113     
===============================================
+ Hits              4099     4125      +26     
- Misses            2139     2226      +87     
Impacted Files Coverage Δ
src/systems/user_commands/UserCommands.hh 100.00% <ø> (ø)
src/systems/physics/Physics.cc 32.58% <16.07%> (-2.67%) ⬇️
src/systems/user_commands/UserCommands.cc 11.85% <16.66%> (+0.23%) ⬆️
...e/ignition/gazebo/components/AngularVelocityCmd.hh 100.00% <100.00%> (ø)
...clude/ignition/gazebo/components/CenterOfVolume.hh 100.00% <100.00%> (ø)
...de/ignition/gazebo/components/LinearVelocityCmd.hh 100.00% <100.00%> (ø)
include/ignition/gazebo/components/Volume.hh 100.00% <100.00%> (ø)
src/LevelManager.cc 86.22% <100.00%> (ø)
src/SdfGenerator.cc 95.83% <100.00%> (+0.07%) ⬆️
src/systems/physics/Physics.hh 100.00% <100.00%> (+90.90%) ⬆️
... and 6 more

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 ca72bee...9eda1a7. Read the comment docs.

@chapulina chapulina linked an issue Jul 20, 2020 that may be closed by this pull request
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 think the only unaddressed comment from the original PR is this one from @azeey :


@azeey

"It would be good to document the reference frame of the position."

@nkoenig

"What language would you recommend? There is no frame attached to this component, so a system can use what ever frame they desire."

@azeey

"That's true, but I think that would lead to confusion. I would recommend maybe saying that the position is relative to the pose of the entity this component is attached to. If we need to specify it in world frame, we can create a CenterOfVolumeInWorld component like we did for Pose and WorldPose. "


Also, I'd appreciate it if you could also take another look, @azeey .

@nkoenig
Copy link
Contributor Author

nkoenig commented Aug 17, 2020

LGTM. I think the only unaddressed comment from the original PR is this one from @azeey :

@azeey

"It would be good to document the reference frame of the position."

@nkoenig

"What language would you recommend? There is no frame attached to this component, so a system can use what ever frame they desire."

@azeey

"That's true, but I think that would lead to confusion. I would recommend maybe saying that the position is relative to the pose of the entity this component is attached to. If we need to specify it in world frame, we can create a CenterOfVolumeInWorld component like we did for Pose and WorldPose. "

Also, I'd appreciate it if you could also take another look, @azeey .

Add position documentation in 9eda1a7

@nkoenig nkoenig merged commit 60e48fe into ign-gazebo3 Aug 17, 2020
@nkoenig nkoenig deleted the buoyancy branch August 17, 2020 19:32
@nkoenig nkoenig mentioned this pull request Aug 31, 2020
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.

Add buoyancy plugin
2 participants