search

open-dynamic-robot-initiative / robot_properties_solo

BSD 3-Clause "New" or "Revised" License
40 stars 20 forks source link

Fix gepetto viewer display And add the kino_dyn_planner config file #4

Closed MaximilienNaveau closed 5 years ago

MaximilienNaveau commented 5 years ago

What has been changed

jviereck commented 5 years ago

add the config files from the kino_dyn_planner.

Why are you adding these files here? Not sure the base repo should contain these files.

MaximilienNaveau commented 5 years ago

add the config files from the kino_dyn_planner.

Why are you adding these files here? Not sure the base repo should contain these files.

This is not the base repos. This is the one dedicated to all Solo models no?.

jviereck commented 5 years ago

I see. Let's try to find a consensus on what the "robot_properties" packages are about and from that figure out if we should include the config files or not? If we cannot find consensus on this within 1-2 messages, I propose to chat about this in person and write down the conclusion here (to be more time efficient.)

My understanding is that the "robot_properties" repos should contain all the basic files/properties about the robot. This, but not more. Everything else that is specific to that robot should go into a different repo. This is to keep the repo small. For instance, ppl at IIT are using the repo already to get an idea for a quadruped. They might not be interested in our conig files.

For the ocnfig files, these shouldn't go into this repo anyways. They should stick to the repo that is dedicated to the research they are involved in.

MaximilienNaveau commented 5 years ago

I think this repos contains indeed all the files/properties specialized for this robot. I do not think about the "basic terms.

I do not think these files should be in the planner code either.

If we really do not want to put them here, the place I would put them is the dg_demos where I actually would rename this folder robot_demos. Where we could have robot specific files/configuration/controller/planner_config/... as well.

jviereck commented 5 years ago

@avadesh02 , what's your take on this?

avadesh02 commented 5 years ago

Hi,

I agree with @Julian. I do not feel the config files should be in the robot properties solo because it does not contain any information that describes the robot. Rather the config files use this information. Also, I think its better to keep that repo as small as possible and minimise data dumping there.

I like @MaximilienNaveau https://github.com/MaximilienNaveau idea of putting the config files in a demo folder separately.

Regards, Avadesh

On Tue, 8 Oct 2019 at 11:15, Julian Viereck notifications@github.com wrote:

@avadesh02 https://github.com/avadesh02 , what's your take on this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/open-dynamic-robot-initiative/robot_properties_solo/pull/4?email_source=notifications&email_token=AGB2E66W6LG2PHQVKDN2E5LQNSPY7A5CNFSM4I5XZ2R2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAURCOI#issuecomment-539562297, or mute the thread https://github.com/notifications/unsubscribe-auth/AGB2E66CAP66M7AK2F5534LQNSPY7ANCNFSM4I5XZ2RQ .

MaximilienNaveau commented 5 years ago

So if you guys accept the current version (no config files from the planner) I would like to land this pull request. This is important to use the gepettto-viewer

jviereck commented 5 years ago

@MaximilienNaveau , merged to unblock your current work. There are a few small comments. Please address the important ones (especially the sleep one). If you won't have time to work on them right away, please open an issue on this repo such that we can keep track on them. thx