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

Ros2 Humble - Translated flatland viz tools to rviz plugins #92

Merged
merged 56 commits into from
Feb 7, 2023

Conversation

TheZambi
Copy link

This PR adds a new package to flatland called flatland_rviz_plugins that enables the smooth transition from flatland_viz to rviz.
The plugins added are:

  • Pause simulation time tool;
  • Spawn model;
  • Interact tool that deals with interactive markers (toggles their visibility).

Note: these are already in use in our ros2 branch in our turtlebot_flatland repo.

JoaoCostaIFG and others added 30 commits November 19, 2022 15:20
Fix usage of get_parameter in YamlPreprocessor. Change integer in yaml out-file to floating point number (result of a calculation in lua).
This fixes a problem where rviz doesn't start on time to catch the publish of the layer markers.
When pressed, it toggles the paused state of the simulation.
The message that will be used by the change rate service to change the simulation speed in run time.
Simulation manager needs a setter for the simulation rate, so the ChangeRate service can alter it in runtime.
Cleans the memory reserved for the ros WallRate object
Used by the change rate tool, so the user can choose the desired rate
@JoaoCostaIFG
Copy link

Added a service and rviz tool to change the simulation update rate in runtime.

@josephduchesne
Copy link
Member

Added a service and rviz tool to change the simulation update rate in runtime.

This is a really neat feature. Probably a bit dangerous but no more than changing the update rate normally is. I can see a typo in calling the service causing really funny errors! I look forward to trying it out.

@josephduchesne
Copy link
Member

Added a service and rviz tool to change the simulation update rate in runtime.

Would it also make sense to control the simulation timestep in the same way (either as the same service call, or a similar one)? The two variables somewhat work together, since the real-time factor is timestep*update rate.

@josephduchesne
Copy link
Member

josephduchesne commented Dec 21, 2022

For any new cpp/hpp files, can you add the BSD 3 clause license:

License Template
/*
 * @copyright Copyright 2022 YOUR_NAME or YOUR_ORGANIZATION
 * @name	FILE_NAME.h
 * @brief DESCRIPTION
 * @author YOUR_NAME
 *
 * Software License Agreement (BSD License)
 *
 *  Copyright (c) 2022, YOUR_NAME or YOUR_ORGANIZATION
 *  All rights reserved.
 *
 *  Redistribution and use in source and binary forms, with or without
 *  modification, are permitted provided that the following conditions
 *  are met:
 *
 *   * Redistributions of source code must retain the above copyright
 *     notice, this list of conditions and the following disclaimer.
 *   * Redistributions in binary form must reproduce the above
 *      copyright notice, this list of conditions and the following
 *     disclaimer in the documentation and/or other materials provided
 *     with the distribution.
 *   * Neither the name of the Avidbots Corp. nor the names of its
 *     contributors may be used to endorse or promote products derived
 *     from this software without specific prior written permission.
 *
 *  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
 *  "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
 *  LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
 *  FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
 *  COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
 *  INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
 *  BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
 *  LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
 *  CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
 *  LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN
 *  ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
 *  POSSIBILITY OF SUCH DAMAGE.
 */

We do not have a contributor license agreement (CLA) requiring that you transfer copyright to Avidbots to merge, but we do need each file to contain an appropriate license. In this case Flatland uses BSD-3 clause (like most ROS packages), so that would be preferred.

For modified files, the existing license headers is fine. Any contributions merged under those existing licenses are assumed (and possibly required) to be under the existing header license. (It would be impractical to license portions of code in the same file under different licenses). Copyright could be done line by line, but I don't think anyone wants that much beurocratic overhead.

I'm trying to figure out why I'm getting a large number of lint errors (including missing copyright headers) on my local build and not on the github action for this branch (well, actually I'm getting different errors on githubactions).

Locally I'm running:
colcon test --event-handlers desktop_notification- status- terminal_title- console_cohesion+ --executor sequential --ctest-args -j1 --packages-skip flatland_viz which is fairly similar to what the github action runs.

Todo items:

  • Add copyright headers to all new cpp/hpp files
  • Fix cpplint formatting (https://www.youtube.com/watch?v=2gIyu09UEC8 explains how to automatically update the style of source files. I'm not 100% sure this will fix everything but it might be a good start)
  • Fix lint_cmake and uncrustify errors if possible (also output on my machine running the above colcon test ... command

@JoaoCostaIFG
Copy link

Added a service and rviz tool to change the simulation update rate in runtime.

Would it also make sense to control the simulation timestep in the same way (either as the same service call, or a similar one)? The two variables somewhat work together, since the real-time factor is timestep*update rate.

Ye that makes sense. It could probably be part of the same menu/rviz plugin.

@TheZambi
Copy link
Author

We've finished correcting the errors that the code had, however there were some that left us not knowing how to proceed:

  • The copyright license has been added to the new files, however when executing the tests, a warning appears indicating that the copyright isn't present, thus failing the tests.
  • Other tests that use cppcheck 2.7 fail: “cppcheck 2.7 has known performance issues and therefore will not be used, set the AMENT_CPPCHECK_ALLOW_SLOW_VERSIONS environment variable to override this.”. Setting the environment variable leads to passing the tests.
  • Cpplint forces an include order for the included libraries, and the first library to be in included must be a .h corresponding to the .cpp including it (for example, load_model_dialog.cpp must have load_model_dialog.h as it's first include). Since we do not have a load_model_dialog.h created, but a load_model_dialog.hpp, this check also fails.

@josephduchesne
Copy link
Member

josephduchesne commented Feb 7, 2023

@TheZambi I have created a copy of your branch and fixed the various errors a few different ways:

  • I've disabled the fairlky agressive ament linting which isn't compatible with our existing coding style
  • I have added the ALLOW_SLOW_VERSIONS flag to the CI parameters

I also tweaked the header paths and includes for flatland_rviz_plugins since they were a bit inconsistant (some headers in the src folder, others in include, some with source relative paths, others include relative). I think they're consistant now. Local testing, building and CI all seem to work but I haven't tested running flatland yet.

Can you merge my branch into yours and push it so this PR updates to include both changes? https://github.com/avidbots/flatland/tree/ros2-rviz-plugins

I'd like to merge my changes in the flatland_turtlebot master branch into your proposed Humble/ros2 port, along with pointing it at nav2, which if combined correctly should allow a turtlebot 1 like simulation in ros2 with nav2 and flatland. That said, I don't think I'll be able to get to that soon. If you want to try, feel free. My most recent merge into master bundles a lot of the now deprecated turtlebot1 launchfiles and dependencies so that turtlebot_flatland isn't dependant on the turtlebot repo as much. This might help with the nav2/ros2 version.

@TheZambi
Copy link
Author

TheZambi commented Feb 7, 2023

Finished pushing the merge. Sadly, we will most likely not be able to look into the other merges that deal with the turtle_bot as the members of the group are starting to work on the master's thesis, and we will be occupied with it for the next months. Thank you for your help throughout the development and all the best.

@josephduchesne
Copy link
Member

No problem. Thanks for the hard work. This has really helped push the ros2 version of flatland ahead. I'll see if I can get a demo repo working with ros2. Good luck with your academic persuits!

@josephduchesne josephduchesne merged commit 4e2f6b0 into avidbots:ros2-humble Feb 7, 2023
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.

3 participants