-
Notifications
You must be signed in to change notification settings - Fork 6
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 dynamic generation and loading sdf file into gazebo functionality. #14
Conversation
Signed-off-by: yaswanth1701 <[email protected]>
Signed-off-by: yaswanth1701 <[email protected]>
Signed-off-by: yaswanth1701 <[email protected]>
Signed-off-by: yaswanth1701 <[email protected]>
Signed-off-by: yaswanth1701 <[email protected]>
Signed-off-by: yaswanth1701 <[email protected]>
Signed-off-by: yaswanth1701 <[email protected]>
DART is working now with sdf files. |
oops, I just merged #11, which conflicts with your changes to the python notebook (this is a drawback of python notebooks to be honest) |
worlds/boxes.world.erb
Outdated
complex = ENV['complex'].to_i | ||
dt = ENV['dt'].to_f | ||
engine = ENV['engine'] | ||
collision = ENV['collision'].to_i |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an alternative to environment variables is to set variables on the command-line (as suggested in https://stackoverflow.com/a/39675039 and used in srcsim) and you could use defined?(variable_name)
to see if a value was set or not (see https://stackoverflow.com/a/288726)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, can you please review it again?
oh I see, I will pull the changes and force push. |
Signed-off-by: yaswanth1701 <[email protected]>
Signed-off-by: yaswanth1701 <[email protected]>
Signed-off-by: yaswanth1701 <[email protected]>
Signed-off-by: yaswanth1701 <[email protected]>
boxes.cc
Outdated
// Load a blank world (no ground plane) | ||
Load("worlds/blank.world", true, _physicsEngine); | ||
Load("worlds/boxes.world", true, _physicsEngine); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Load("worlds/boxes.world", true, _physicsEngine); | |
Load(WORLDS_FILE_PATH "/boxes.world", true, _physicsEngine); |
you can tell the loader where the world file is so you don't have to set the GAZEBO_RESOURCE_PATH
worlds/boxes.world.erb
Outdated
@@ -23,8 +44,8 @@ | |||
%> | |||
<sdf version="1.5"> | |||
<world name="default"> | |||
<physics type="ode"> | |||
<max_step_size>0.001</max_step_size> | |||
<physics type="<%= engine %>"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't technically need to encode the physics engine in the world file for boxes.cc
because we can specify the physics engine by name when loading the world; these other parameters are good though
boxes.cc
Outdated
command << "erb " | ||
<< "engine=" << _physicsEngine << " model_count=" << _modelCount << " collision=" << _collision | ||
<< " complex=" << _complex << " dt=" << _dt << " " << WORLDS_FILE_PATH << "/boxes.world.erb > " | ||
<< WORLDS_FILE_PATH << "/boxes.world"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could consider encoding the parameters in the name of the generated world:
boxes_collision1_complex0_dt1e-3_modelCount1000.world
boxes_collision0_complex1_dt1e-2_modelCount10.world
then one could share the world and an output log file to document their results
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this would be nice, I have made the suggested changes could you please review it again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great! just a few small comments
boost::format( | ||
"%1%/%2%/" | ||
"boxes_collision%3%_complex%4%_dt%5$.0e_modelCount%6%.world") % | ||
WORLDS_DIR_PATH % TEST_NAME % _collision % _complex % _dt % _modelCount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not for formatting the dt
value, I would recommend using a stringstream
instead of boost::format
as I find it more readable to have the values expressed inline. It's too bad boost::format
doesn't use the same formatting syntax as the c++20 formatting library.
We intentionally avoid using boost in gz-*
libraries. Since gazebo-classic is already using boost, I think it's fine here, but we may want to avoid it when integrating with the newer gazebo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I see, I was using boost because it was already being used by gazebo classic. I will make sure to stick to stringstream
for newer gazebo.
boxes.cc
Outdated
ASSERT_EQ(v0, link->WorldCoGLinearVel()); | ||
ASSERT_EQ(w0, link->WorldAngularVel()); | ||
ASSERT_EQ(I0, link->GetInertial()->MOI()); | ||
ASSERT_NEAR(link->GetWorldEnergy(), E0, 1e-6); | ||
ASSERT_NEAR(link->GetWorldEnergy(), E0, 1e-5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these checks will only be executed for the link
in the last model of models
since they are outside the loop
boxes.cc
Outdated
} | ||
// adding a small delay (waiting for the model to load properly) | ||
common::Time::MSleep(50); | ||
|
||
ASSERT_EQ(v0, link->WorldCoGLinearVel()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it could be nice to also check that the moment of inertia matrix in the body-fixed frame matches I0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Steve, I have made the suggested changes could you please review them again ?
thanks for your patience; this looks great! |
Thanks!! for reviewing Steve |
Tested on: Ubuntu 20 and gazebo classic - 11
Description: Now instead of creating the whole model through gazebo C++ API using model msg, one can easy load sdf files generated from
erb.world
files. This feature allow for generation of desired configuration.world
files at runtime to improve compatibility with other open-source simulators.Issues:
boxes_dt
tests work well, however,boxes_model_count
fails in the case ofsimbody
. I have currently raised an issue atsimbody
regarding the error that is encountered in this case.Steps to test the updates:
To run all tests:
To test individual cases:
NOTE: Currently I have commented Simbody test cases in the boxes_model_count file as they are not working properly.