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

Elham/go1 #31

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Elham/go1 #31

wants to merge 2 commits into from

Conversation

Lhumd
Copy link
Member

@Lhumd Lhumd commented May 25, 2023

Description

I add go1 to a demo of this package.

How I Tested

I ran it on my computer.

Copy link
Contributor

@MaximilienNaveau MaximilienNaveau left a comment

Choose a reason for hiding this comment

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

I think you can merge this if you are happy with it.

@@ -12,6 +12,7 @@
from mim_control.robot_impedance_controller import RobotImpedanceController
from bullet_utils.env import BulletEnvWithGround
from robot_properties_solo.solo12wrapper import Solo12Robot, Solo12Config
from robot_properties_go1.go1wrapper import Go1Robot, Go1Config
Copy link
Contributor

Choose a reason for hiding this comment

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

would it be possible to add a try catch here for the optional dependencies?

for example one wants to run on solo but not on go1 and still need to install go1 to run the demo...
It would be nice to allow the users to run it without solo if you run go1 and the other way around.

WDYT?

@@ -14,6 +14,7 @@ This packages contains generic controllers that can be used for a biped, quadrup
- BulletUtils (Optional, needed to run demos)
- Robot_Properties_Solo (Optional, needed to run demos)
- Robot_Properties_Bolt (Optional, needed to run demos)
- Robot_Properties_Go1 (Optional, needed to run demos)
Copy link
Contributor

Choose a reason for hiding this comment

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

we miss something in the CMakeLists.txt to check if those optional dependencies are installed or not...
It is not needed per say but would be nice IMHO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants