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.

Sign up for GitHub

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

Jump to bottom

Add colours to trajectory_visualisation display #362

Merged
merged 6 commits into from
Nov 18, 2016
Merged

Add colours to trajectory_visualisation display #362

merged 6 commits into from
Nov 18, 2016

Conversation

bmagyar
Copy link
Member

@bmagyar bmagyar commented Nov 15, 2016 •
edited
Loading

Description

I needed a way to overlay trajectory displays in an easy to distinguish way so I've added colouring to the trajectory_visualisation display.

A video can be found here:
https://www.youtube.com/watch?v=Vj0KNAOtFrw

screenshot_2016-11-15_16-21-48

Please note that this is a PR to discover interest for this feature.
Code is probably very dirty and doesn't meet any of the merging requirements, I haven't even rebased it on the current codebase yet, it's a few weeks old. If I get a few thumbs up, I'd gladly tidy things up to be merged!

PS: I love this checklist 👍

Checklist

  • Required: Code is auto formatted using clang-format
  • Extended the tutorials / documentation, if necessary reference
  • Include a screenshot if changing a GUI
  • Optional: Created tests, which fail without this PR reference
  • Optional: Decide if this should be cherry-picked to other current ROS branches (Indigo, Jade, Kinetic)

@davetcoleman
Copy link
Member

+1 to proceeding - I've run into this limitation before and the only current way to differentiate a trajectory is by the transparency level

can you share a screenshot of the updated display properties also?

@bmagyar
Copy link
Member Author

bmagyar commented Nov 15, 2016

screenshot_2016-11-15_16-27-49

I've added a boolean property to disable this functionality, this could be off by default.

@bmagyar
Copy link
Member Author

bmagyar commented Nov 15, 2016

Also I'd only colour the actual moving parts of the robot, I can add that too.

@davetcoleman
Copy link
Member

I'd suggest the wording "Color Enabled" to match how the RobotState display words "Visual Enabled" and "Collision Enabled"

@v4hn
Copy link
Contributor

v4hn commented Nov 16, 2016 via email

@bmagyar bmagyar force-pushed the coloured_trajectory_visualisation branch 3 times, most recently from 2189b86 to 9224b50 Compare November 16, 2016 15:25
@bmagyar bmagyar force-pushed the coloured_trajectory_visualisation branch from 9224b50 to 4a58328 Compare November 16, 2016 17:29
@bmagyar
Copy link
Member Author

bmagyar commented Nov 16, 2016

I ran clang-format, but discovered some inconsistencies around pointers and references.

I'd personally prefer the .h version everywhere.

Change it did in .h:
void setRobotColor(rviz::Robot* robot, const QColor& color);

Change it did in .cpp:
void TrajectoryVisualization::setRobotColor(rviz::Robot *robot, const QColor &color)

@davetcoleman
Copy link
Member

clang-format: I hadn't realized that discrepancy. Feel free to investigate and propose a fix to https://github.com/ros-planning/moveit/blob/kinetic-devel/.clang-format

davetcoleman
davetcoleman approved these changes Nov 17, 2016
View reviewed changes
Copy link
Member

@davetcoleman davetcoleman left a comment

Choose a reason for hiding this comment

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

Looks good to me - is this ready to merge?

@bmagyar
Copy link
Member Author

bmagyar commented Nov 17, 2016

Thanks!

I'll change that clang format back by hand first, then investigate.

I'd also like to tweak the behaviour a bit first, now when the .rviz config
is saved with colours on they don't active when loaded. I need to fiddle a
bit there. I'll attempt getting it all done on Friday.

On 17 November 2016 at 18:02, Dave Coleman notifications@github.com wrote:

@davetcoleman approved this pull request.

Looks good to me - is this ready to merge?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#362 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADXH4X1iJwW-q5LKv2ty6X58N2HsmstZks5q_JbMgaJpZM4KzBnV
.

@bmagyar
Copy link
Member Author

bmagyar commented Nov 18, 2016

My last commit fixed the issue with saved rviz configuration of the tickbox & moved those &-s and *-s where they should be.

All green from me 👍

@davetcoleman davetcoleman mentioned this pull request Nov 18, 2016
Merged
@davetcoleman
Copy link
Member

We should not try to override the clang-format default, otherwise all Travis tests will fail in the future. Rather, I have investigated your observation about clang-format and made a upstream fix here: #367

Until then you should do what clang-format suggests so we can merge this

@bmagyar
Copy link
Member Author

bmagyar commented Nov 18, 2016

Right! Actually my hand-fixed stuff is not failing but some of the new
piece of code I touched while adding the final bits. Pushed update.

On 18 November 2016 at 17:01, Dave Coleman notifications@github.com wrote:

We should not try to override the clang-format default, otherwise all
Travis tests will fail in the future. Rather, I have investigated your
observation about clang-format and made a upstream fix here: #367
#367

Until then you should do what clang-format suggests so we can merge this

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#362 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADXH4VT05q_LNtBvRLJoJAwagmzDDeSoks5q_dn0gaJpZM4KzBnV
.

@davetcoleman
Copy link
Member

I've confirmed this PR works in my system - having the colored trajectory is great thanks!

@davetcoleman davetcoleman merged commit 7fd7315 into moveit:kinetic-devel Nov 18, 2016
@davetcoleman
Copy link
Member

davetcoleman commented Nov 18, 2016 •
edited
Loading

Please open PRs for Jade and Indigo if you'd like this backported (requires C++11 removal)

@bmagyar bmagyar changed the title WIP: Add colours to trajectory_visualisation display Add colours to trajectory_visualisation display Nov 19, 2016
@bmagyar
Copy link
Member Author

bmagyar commented Nov 19, 2016

Directly to the branches of this repo, right?

@davetcoleman
Copy link
Member

Yes - create a branch in your fork for i.e. jade and open a pull request to jade-devel in this repo

@bmagyar bmagyar mentioned this pull request Nov 23, 2016
Merged
5 tasks
@bmagyar bmagyar mentioned this pull request Nov 23, 2016
Merged
5 tasks
davetcoleman pushed a commit to davetcoleman/moveit that referenced this pull request Jan 2, 2017
@bmagyar @davetcoleman
WIP: Add colours to trajectory_visualisation display (moveit#362)
0861a7a 
* Add colours to trajectory_visualisation display

* Change label

* Cleanup & change tickbox default

* clang-format

* force-refresh to account for saved display config

* clang-format again
JafarAbdi added a commit to JafarAbdi/moveit that referenced this pull request Mar 24, 2022
@JafarAbdi
Bug fixes in main branch (moveit#362)
97dd653 
* robot_trajectory: Fix bugs in getRobotTrajectoryMsg function
* controller_manager: Use Duration(-1) as infinite timeout
* ActionBasedControllerHandle: fix dangling reference in case of timeout
* TfPublisher: tf frame name can't start with '/'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davetcoleman davetcoleman davetcoleman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bmagyar @davetcoleman @v4hn