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

Integrate bullet into ign-physics3 #208

Merged
merged 73 commits into from
May 20, 2021

Conversation

Blast545
Copy link
Contributor

@Blast545 Blast545 commented Feb 3, 2021

This PR includes bullet integration into ign-physics, supporting the set of basic features described here:

The following elements are available:

  • Creating basic shapes and meshes, with their associated collisions
  • Step simulation
  • Revolute joints with their getter/setters
  • Diff drive plugin integration
  • Fixed joints

Unit tests are not included in this PR, the elements in this PR were tested with the following example from gazebo4:
shapes.sdf, joint_controller.sdf, joint_position_controller.sdf. Meshes were tested on their own with #178.
Revolute joints and ground vehicles were tested with a modified version of diff_drive.sdf (replacing the ball joint with a fixed one). And using the subt simulation with the model X1_SENSOR_CONFIG_1. This test was run with the command:

ign launch -v4 cave_circuit.ign circuit:=cave worldName:=simple_cave_01 robotName1:=X1 robotConfig1:=X1_SENSOR_CONFIG_1 engine:=bullet

And using this subt fork: https://github.com/Lobotuerk/subt/tree/bullet_test.

Known issues:

  • There is an issue with the axis of some revolute joint combinations, this was found running this command:

gimball_problem_

  • The angle getter from bullet resets its initial count if the angle difference between two consecutive readings is higher than a hardcoded threshold. To address this issue in the joint controllers a threshold in the max torque was set. (An extra eyes into the best approach to address this would be appreciated). Link to the threshold applied in bullet Im talking about.

  • Fixed joints are not fixing all the possible degrees of freedom, @Lobotuerk ran into this testing flying vehicles.

@Blast545 Blast545 requested a review from mxgrey as a code owner February 3, 2021 19:19
@github-actions github-actions bot added the 🔮 dome Ignition Dome label Feb 3, 2021
@Blast545 Blast545 added the Bullet Bullet engine label Feb 3, 2021
@chapulina chapulina mentioned this pull request Feb 3, 2021
@chapulina chapulina linked an issue Feb 3, 2021 that may be closed by this pull request
@Blast545
Copy link
Contributor Author

Blast545 commented Feb 8, 2021

When testing vehicles availables from the subt simulation files, I found that some of those have overlapping parts (by checking the collision model), is this expected from the models? This causes problems with bullet.

image

With 55dd0f9 I added a workaround, a collision group for each model, this way links added to the same model won't crash with each other. Although this will work only for .sdf files with a maximum of 31 models in total.

@Blast545 Blast545 force-pushed the bulletDev/blast545/bulletPhysicsDome branch 2 times, most recently from e864fd9 to 16bc0bc Compare February 12, 2021 21:30
@Blast545
Copy link
Contributor Author

With 6742e5d it was added the changes proposed in bulletDev/lobotuerk/ParameterTuning and an extra variable mu3 used for testing friction of the models.

@Blast545 Blast545 force-pushed the bulletDev/blast545/bulletPhysicsDome branch from 16bc0bc to 60b50a9 Compare February 18, 2021 17:29
@codecov
Copy link

codecov bot commented Feb 18, 2021

Codecov Report

Merging #208 (d38c5f8) into ign-physics3 (068425f) will decrease coverage by 8.77%.
The diff coverage is 28.39%.

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

@@               Coverage Diff                @@
##           ign-physics3     #208      +/-   ##
================================================
- Coverage         83.27%   74.49%   -8.78%     
================================================
  Files               106      115       +9     
  Lines              3951     4661     +710     
================================================
+ Hits               3290     3472     +182     
- Misses              661     1189     +528     
Impacted Files Coverage Δ
bullet/src/FreeGroupFeatures.cc 0.00% <0.00%> (ø)
bullet/src/JointFeatures.cc 0.00% <0.00%> (ø)
bullet/src/KinematicsFeatures.cc 0.00% <0.00%> (ø)
bullet/src/ShapeFeatures.cc 0.00% <0.00%> (ø)
bullet/src/SimulationFeatures.cc 0.00% <0.00%> (ø)
dartsim/src/CustomMeshShape.cc 62.28% <ø> (ø)
dartsim/src/EntityManagementFeatures.cc 73.41% <ø> (ø)
dartsim/src/ShapeFeatures.cc 36.42% <ø> (ø)
include/ignition/physics/BoxShape.hh 100.00% <ø> (ø)
include/ignition/physics/Cloneable.hh 100.00% <ø> (ø)
... and 29 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 101eabc...3792981. Read the comment docs.

Copy link
Contributor

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

Still reviewing, but so far I see a lot of manual memory management with raw pointers. I think we should generally avoid this in new code unless there is a very specific reason. Is there one in this case?

/// GenerateIdentity.
/// 3) Hold explicit copies of raw pointers that can be deallocated

// todo(anyone): Handle cleaning these pointers
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be addressed now. There is a strong chance of leaking these.

I think these could be made std::unique_ptrs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the same as you, I think those can be std::unique_ptrs as well, but I implemented those with std::shared_ptrs to copy the same pattern used with dart implementation. I have in this commit in a test branch: Smart pointers.

However, when I add this, the program crashes when it's closed, showing a malloc_consolidate error. or a free pointer, invalid chunk size. I think bullet has problem when the things are not deleted in reverse order and there is no guarantee with ignition on its own.

Copy link
Contributor Author

@Blast545 Blast545 Mar 12, 2021

Choose a reason for hiding this comment

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

Never mind, I was able to use shared_ptrs in this PR, take a look to bd154bb. With this I think we've addressed your comments, let me know if this is OK

struct LinkInfo
{
std::string name;
btRigidBody* link;
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is the only owner, prefer a smart pointer.

bullet/src/Base.hh Outdated Show resolved Hide resolved
bullet/src/Base.hh Outdated Show resolved Hide resolved
Comment on lines +193 to +249
public: using WorldInfoPtr = std::shared_ptr<WorldInfo>;
public: using ModelInfoPtr = std::shared_ptr<ModelInfo>;
public: using LinkInfoPtr = std::shared_ptr<LinkInfo>;
public: using CollisionInfoPtr = std::shared_ptr<CollisionInfo>;
public: using JointInfoPtr = std::shared_ptr<JointInfo>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for shared vs unique here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those in particular can't be changed because the GenerateIdentity templated function from ignition takes a shared pointer to generate new identities.

bullet/src/Base.hh Outdated Show resolved Hide resolved
Comment on lines 210 to 212
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}
}
}
} // namespace bullet
} // namespace physics
} // namespace ignition

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 all missing with ab71031

Comment on lines 78 to 79
std::unordered_map<std::size_t, LinkInfoPtr>::iterator it =
this->links.begin();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
std::unordered_map<std::size_t, LinkInfoPtr>::iterator it =
this->links.begin();
auto it = this->links.begin();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed all similar iterators with 607b609

const JointInfoPtr &jointInfo = this->joints.at(_id.id);
const int jointType = jointInfo->constraintType;
// Check the type of joint and act accordignly
if (jointInfo->constraintType ==
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the intent to handle other constraints? In that case I think a switch makes more sense here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I changed it to a switch statement with 7d6d9d2

@Blast545
Copy link
Contributor Author

Blast545 commented Mar 4, 2021

@mjcarroll There is no specific reason to use raw pointers, thanks for the comments, I'll address them today.

@Blast545
Copy link
Contributor Author

Blast545 commented Mar 4, 2021

Added some extra commits:

  • c583765 , 4cf6b4e , aa22784 These commits were added to set and test mu and mu2 friction values as close as possible to DART physics implementation. With the worlds added ./bullet/worlds/test_friction_mu.sdf and ./bullet/worlds/test_friction_mu2.sdf it can be seen the axis where those friction components are applied.

  • d022c34 This commit was added to remove collision groups from this PR. Links within a model will collide with each other if they overlap only if they are not sharing a joint. However, that's not a common scenario.

  • 6c1dbd6 ae99a36 These commits were added to remove/update outdated comments in the code. PTAL @Lobotuerk

  • 9f332c9 Remove joints from bullet world when a model is removed, PTAL @Lobotuerk

  • dc325c0 Divide inertia from torque to get acceleration. Can someone with more physics experience help me out figuring out if this makes sense?

@Blast545
Copy link
Contributor Author

@Lobotuerk As this branch does not include smart pointers for memory management (at least yet) I added the missing delete with 607b609

Blast545 and others added 20 commits May 19, 2021 18:04
Signed-off-by: Jorge Perez <[email protected]>
Signed-off-by: Tomas Lorente <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: Ian Chen <[email protected]>

Co-authored-by: Ian Chen <[email protected]>
Signed-off-by: Tomas Lorente <[email protected]>
Signed-off-by: Tomas Lorente <[email protected]>
Signed-off-by: Tomas Lorente <[email protected]>
Signed-off-by: Tomas Lorente <[email protected]>
Signed-off-by: Tomas Lorente <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Tomas Lorente <[email protected]>
Signed-off-by: Jorge Perez <[email protected]>
Signed-off-by: Tomas Lorente <[email protected]>
Signed-off-by: Jorge Perez <[email protected]>
Signed-off-by: Tomas Lorente <[email protected]>
Signed-off-by: Jorge Perez <[email protected]>
Signed-off-by: Tomas Lorente <[email protected]>
Signed-off-by: Jorge Perez <[email protected]>
Signed-off-by: Tomas Lorente <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>
Signed-off-by: Tomas Lorente <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>
Signed-off-by: Tomas Lorente <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Tomas Lorente <[email protected]>
Signed-off-by: Tomas Lorente <[email protected]>
Signed-off-by: Jorge Perez <[email protected]>
Signed-off-by: Tomas Lorente <[email protected]>
Signed-off-by: Tomas Lorente <[email protected]>
Signed-off-by: Jorge Perez <[email protected]>
Signed-off-by: Tomas Lorente <[email protected]>
Signed-off-by: Tomas Lorente <[email protected]>
@Lobotuerk Lobotuerk force-pushed the bulletDev/blast545/bulletPhysicsDome branch from 371fbea to 2592a48 Compare May 19, 2021 21:05
@Blast545
Copy link
Contributor Author

Blast545 commented May 20, 2021

2 approvals, 10/10 checks passing, merging. Thanks a lot to all reviewers! 🚀

@Blast545 Blast545 merged commit 0ec547b into ign-physics3 May 20, 2021
@Blast545 Blast545 deleted the bulletDev/blast545/bulletPhysicsDome branch May 20, 2021 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bullet Bullet engine 🔮 dome Ignition Dome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate Bullet
7 participants