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

Added drone circuit assets for Behavior Metrics #134

Merged

Conversation

UtkarshMishra04
Copy link
Member

fix #132

@pariaspe pariaspe self-requested a review July 6, 2021 18:29
Copy link
Collaborator

@pariaspe pariaspe left a comment

Choose a reason for hiding this comment

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

Are of the circuit models duplicated of the ones in CustomRobots? Iris models seems the same too.
I'm not sure about the convenience of duplicating the models. If it is only changing worlds and launch files, they can be located in exercise folder.

@UtkarshMishra04
Copy link
Member Author

@sergiopaniego Can you please participate in this conversation?

  1. The Iris models are same but the world and launch files are circuit specific.
  2. The assets should move to /opt/jderobot/

@jmplaza
Copy link
Member

jmplaza commented Jul 7, 2021

I would use the Iris models from JdeRobot/drones repo. The circuit files may be used from CustomRobots (as they are already there) and the launch files + world files in the DeepLearningStudio repo.... We are facing here the typical reuse problem. Let's avoid replicating files along different applications....

There are three repositories for specific "assets" and drivers in JdeRobot (not directly included or ready-to-use in general ROS packages): JdeRobot/drones, JdeRobot/IndustrialRobots and JdeRobot/CustomRobots. They should be usable from several different applications. Maybe unifying the installation directory to /opt/jderobot for all of them.

@pariaspe, this PR comes from a GSoC project that @UtkarshMishra04 is doing, which is related to DeepLearningStudio and BehaviorMetrics. It is not related to RoboticsAcademy. We use there drones and Formula1 cars.

What do you think?

@UtkarshMishra04
Copy link
Member Author

Thanks for the comment @jmplaza
The launch and world files cannot be directly used for drones as I am using the PX4 launch file as the base code and drone positioning and circuits in the world files.
We need to move everything to /opt/jderobot/, that's the only concern!

@pariaspe
Copy link
Collaborator

pariaspe commented Jul 7, 2021

Ok @jmplaza, then there isn't a "exercise" to locate the non-duplicated files, but they can be located in DL-Studio repo as you have suggested.

As far as I know, /opt/jderobot/ is a legacy from jderobot_assets. Is there any reason to keep using it?
Of course we can move the installation, but doing it several changes have to be done in RoboticsAcademy exercises.

Sorry @UtkarshMishra04, I do not understand this sentence:

The launch and world files cannot be directly used for drones as I am using the PX4 launch file as the base code and drone positioning and circuits in the world files.

Launch files are similar to mavros_px4_sitl.launch, just changing world and pose arguments.

Of course we can move everything to /opt/jderobot/, but I would really appreciate if we could discuss it and take the best solution.

@UtkarshMishra04
Copy link
Member Author

UtkarshMishra04 commented Jul 8, 2021

Hi @jmplaza

So after a short discussion with @pariaspe, we concluded that we can put all the circuit models in the drone_assets/models/ directory and I can copy that directory directly to /opt/jderobot/.
And for the additional launch and world files, a different package should work. This package will do all the copying tasks.
Let me know your suggestion and @pariaspe, kindly review the PR once again.

@@ -0,0 +1,34 @@
cmake_minimum_required(VERSION 3.0.2)
project(jderobot_assets)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest to change package name. jderobot_assets is an old and deprecated dependency

DESTINATION ${CATKIN_PACKAGE_SHARE_DESTINATION}
)

## JDEROBOT INSTALL ##
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional: jderobot install can be ignored. If we decide to remove it, envvars should be modified to new location

drone_circuit_assets/CMakeLists.txt Show resolved Hide resolved
<!-- Updates According to Package format 3 https://www.ros.org/reps/rep-0149.html -->
<package format="3">

<name>jderobot_assets</name>
Copy link
Collaborator

Choose a reason for hiding this comment

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

package name

<author email="utkarsh75477@gmail.com">Utkarsh A. Mishra</author>

<license>MIT</license>
<buildtool_depend>catkin</buildtool_depend>
Copy link
Collaborator

Choose a reason for hiding this comment

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

add drone_assets dependency

@pariaspe pariaspe merged commit 3ccbc1c into JdeRobot:noetic-devel Jul 8, 2021
@jmplaza
Copy link
Member

jmplaza commented Jul 8, 2021

Perfect

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.

None yet

3 participants