Image display with mouse clicking functionality #1737

miguelriemoliveira · 2022-04-11T09:35:27Z

Description

The idea is that the user can click an image display in rviz and receive, through a published ros topic, the image pixel coordinates where the click occurred. This could be useful in many situations, for example for manually annotating the images.
Here's the issue that is requesting this functionality.

The MouseClick class handles mouse clicking functionalities integrated into the ImageDisplay, which:

uses a qt event filter to capture mouse clicks;
handles image resizing;
checks if the click was inside or outside the image;
scales the pixel coordinates to get them w.r.t. the image (not the window) size;
handles disabling and enabling the image display;
handles inserting of a non valid topic name (stops publishing, but does no crash);

Mouse clicks image pixel coordinates are published as ros geometry_msgs PointStamped. The z component is ignored.

The topic name where the mouse clicks are published is created after the subscribed image topic as: <image_topic>/mouse_click.

Checklist

If you are addressing rendering issues, please provide:
- Images of both, broken and fixed renderings.

This is not a rendering issue.

Source code to reproduce the issue, e.g. a YAML or rosbag file with a MarkerArray msg.

For testing, I have used this bagfile which contains images from two separate cameras, camera_2 and camera_3.
The topics are:

  /camera_2/rgb/image_raw   35 msgs    : sensor_msgs/Image
  /camera_3/rgb/image_raw   35 msgs    : sensor_msgs/Image

Use rosbag to play the bag:

 rosbag play two_rgb_cameras.bag -l

and then run rviz and create an image display and configure the topic name (or you can use this rviz configuration for this bag file ).
At this point a rostopic list should show the clicked mouse topic created as:

<image_topic>/mouse_click

which you can listen using, e.g., for camera_2:

rostopic echo /camera_2/rgb/image_raw/mouse_click

Now if you click somewhere on the image in rviz the pixel coordinates will be shown on the rostopic echo terminal.

This image shows my workspace while testing the functionality. Terminal title bars show the commands that are running:

If you are changing GUI, please include screenshots showing how things looked before and after.

This does not change the appearance of the gui, but integrated with a ros node that produces new images based on the image clicks, we can have something like this, where the clicked points are being used to build a polygon annotated on the image which is being subscribed.

Choose the proper target branch: latest release branch, for non-ABI-breaking changes, future release branch otherwise.
Due to the lack of active maintainers, we cannot provide support for older release branches anymore.

I am not entirely sure but from what I read there are no ABI-breaking changes, so the target is the latest release branch.

Did you change how RViz works? Added new functionality? Do not forget to update the tutorials and/or documentation on the ROS wiki

I will do it once/if the PR is accepted.

miguelriemoliveira · 2022-04-20T13:11:54Z

Hello again,

I noticed that the Npr__rviz__ubuntu_focal_amd64 — Build finished. failed. I looked into the details and I think its nothing about the code I commited, but other warnings of different files in rviz. Should I do something or just wait for your review?

rhaschke · 2022-04-20T13:15:51Z

I didn't have time to look into this yet (and won't within the upcoming week). The failure of the ROS build farm is due to warnings in the python wrapper we cannot silent. No need for action.

miguelriemoliveira · 2022-04-20T13:23:41Z

OK, thanks for the feedback.

miguelriemoliveira · 2022-07-01T06:48:48Z

Dear @rhaschke ,

I imagine you are very busy, but I wanted to ask if you plan to integrate this functionality into RVIZ. From our tests it works fine, and I am sure it will be useful to many.

I am asking because we are about to release a ros calibration framework that requires this functionality to run some image annotations.

Right now we have to ask people to download our rviz fork

https://lardemua.github.io/atom_documentation/getting_started/#clone-rviz-fork

which is far from ideal. It would be much better if the users could just use rviz out of the box.

Could I please ask you to take a look at this? Thank you so much for any time you can spare.

Thank you,
Miguel

rhaschke

In general, this looks good. I have a few remarks though. Please also consider formatting issues raised by CI.
Why do you constrain this functionality to ImageDisplay and don't include CameraDisplay as well? Implementing this at the level of ImageDisplayBase should cover both, shouldn't it?

rhaschke · 2022-07-08T08:29:18Z

src/rviz/default_plugin/image_display.cpp

+void ImageDisplay::setTopic(const QString& topic, const QString& datatype)
+{
+  ImageDisplayBase::setTopic(topic, datatype);
+  mouse_click->setTopic(topic);
+}
+


There is no need to override ImageDisplayBase::setTopic() as this will call updateTopic() anyway.

OK, I will remove this override.

rhaschke · 2022-07-08T08:30:48Z

src/rviz/default_plugin/image_display.cpp

@@ -72,6 +72,8 @@ ImageDisplay::ImageDisplay() : ImageDisplayBase(), texture_()
                      this, SLOT(updateNormalizeOptions()));

  got_float_image_ = false;
+
+  mouse_click = new MouseClick();


Nitpick: For deletion, you rely on RenderPanel being mouse_click's parent.
However, this is only set in onInitialize()!

I am sorry but my cpp is a bit rusty. Like every sane person, I've moved to python a few years ago : - )

What do you suggest we do here?

Pass a parent QObject* to MouseClick's constructor a forward this argument to the base class constructor (see below).

src/rviz/default_plugin/image_display.h

rhaschke · 2022-07-08T08:37:43Z

src/rviz/image/mouse_click.h

+  boost::shared_ptr<ros::NodeHandle> node_handle_;
+  boost::shared_ptr<ros::Publisher> publisher_;


Why do you need to use pointers here?

Its been some years since I've used cpp, but at the time I was using boost pointers over regular pointers whenever possible because I thought they were safer ... not sure if that makes sense tough.

I will set them as regular pointers.

My point was to not use pointers at all for these, but regular object instances.

Hi @rhaschke ,

However, for the publisher_ I think I need it to be a pointer so I can create a new publisher on a different topic in these methods:

https://github.com/miguelriemoliveira/rviz/blob/61974117adaf5f632800ccc906fe84d3204d5a93/src/rviz/image/mouse_click.cpp#L28-L40

If the publisher_ is a regular object I don't know how to do this ...

The node_handle_ must also be a pointer so it can be set on the mouse click constructor.

I will leave these two as a pointers for now...

src/rviz/image/mouse_click.cpp

miguelriemoliveira · 2022-07-13T09:50:35Z

Hi @rhaschke ,

thank you for taking time to look into this. I will work on it by the end of this week and get back to you.

Regards,
Miguel

Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com>

miguelriemoliveira

Hi @rhaschke ,

I went through your suggested changes. They all make sense to me.

I do not have a lot of experience with this pull request / review mechanisms so I am a bit lost:
Should I implement something from my side for you to review, or is accepting the changes you proposed enough?

About the question of why I did not add this mouse click functionality at the level of the ImageDisplayBase it was just because I did not think of that.

My cpp is rusty, and this rviz code is not easy to grasp.

If you want I can do a new PR with this change (and the others).

miguelriemoliveira · 2022-07-16T07:54:59Z

Please also consider formatting issues raised by CI.

You mean these?

I go in the links but cannot find any relevant information. How can I check these formatting issues.

Thanks for the help.

rhaschke

I do not have a lot of experience with pull request / review mechanisms so I am a bit lost:
Should I implement something from my side for you to review, or is accepting the changes you proposed enough?

Accepting and applying my suggestions is the first step. This will add more commits to your PR branch. However, some of those suggested changes (e.g. variable renames) require actions in other locations as well. Some review comments require genuine actions of you.
Its your task to work on this and improve your PR branch until it becomes acceptable for merging.

If you want I can do a new PR with the requested changes.

You don't do a new PR, but you just push additional commits to your existing PR branch.
https://stackoverflow.com/questions/7947322/preferred-github-workflow-for-updating-a-pull-request-after-code-review

Please also consider formatting issues raised by CI.

I was referring to the CI job called Format / pre-commit: Here is the log.

rhaschke · 2022-07-31T23:09:18Z

src/rviz/image/mouse_click.cpp

+namespace rviz
+{
+
+MouseClick::MouseClick() : QObject()


Provide a proper parent in the constructor as suggested in #1737 (comment):

Suggested change

MouseClick::MouseClick() : QObject()

MouseClick::MouseClick(QObject* parent) : QObject(parent)

I have done this but it had an implication which I am not sure is ok.

Because I only get the parent render_panel_ during the ImageDisplay::onInitialize() I have to call the cnstructor of the MouseClick from there, instead of the ImageDisplay::ImageDisplay() ...

src/rviz/default_plugin/image_display.h

rhaschke · 2022-07-31T23:14:08Z

src/rviz/image/mouse_click.h

+  boost::shared_ptr<ros::NodeHandle> node_handle_;
+  boost::shared_ptr<ros::Publisher> publisher_;


My point was to not use pointers at all for these, but regular object instances.

miguelriemoliveira · 2022-08-08T12:47:14Z

Hi @rhaschke ,

thanks for the tips, and sorry for the delayed response. You caught me on vacations!

I will try to work on this tonight and get back to you asap.

miguelriemoliveira · 2022-08-10T01:02:51Z

In general, this looks good. I have a few remarks though. Please also consider formatting issues raised by CI.
Why do you constrain this functionality to ImageDisplay and don't include CameraDisplay as well? Implementing this at the level of ImageDisplayBase should cover both, shouldn't it?

Hi @rhaschke ,

So I tried to move the mouse_click functionality to the ImageDisplayBase but could not get pass this point:

https://github.com/miguelriemoliveira/rviz/blob/61974117adaf5f632800ccc906fe84d3204d5a93/src/rviz/default_plugin/image_display.cpp#L121-L136

because my mouse_click.onInitialize() method requires a QObject as argument, which I get from the

render_panel_ = new RenderPanel();

But this does not exist in the ImageDisplayBase, only on the ImageDisplay ...

So I don't know how to move the mouse_click into the ImageDisplayBase ... not sure its possible ... any ideas?

miguelriemoliveira · 2022-08-10T02:13:13Z

Hi @rhaschke ,

I was working on this tonight and I think I have a version ready for you to review (
0135a80)

Above I gave point to point comments to the things you suggested.

I could not move the functionality to the ImageDisplayBase, as I explain above.

Please let me know how I can further improve the code.

Thanks for your help,
Miguel

- Rename variables and functions for more clarity - Drop shared_ptr whereever possible - Drop MouseClick::updateTopic()

miguelriemoliveira · 2024-04-25T10:11:34Z

Hi @rhaschke ,

I just saw some changes on this from your side. Are you coming back to this issue?

rhaschke · 2024-04-25T10:13:31Z

I just saw some changes on this from your side. Are you coming back to this issue?

Yes, I want to merge this PR now. Sorry for the looong delay. This slipped out of my attention.

miguelriemoliveira · 2024-04-25T10:16:03Z

I just saw some changes on this from your side. Are you coming back to this issue?

Yes, I want to merge this PR now. Sorry for the looong delay. This slipped out of my attention.

Great news. I thought this was lost forever. I was going around telling my friends I was "almost" an RViz contributor :).

Thanks.

peci1 · 2024-05-09T09:41:52Z

@miguelriemoliveira I see a problem with this PR: When I click Add->By topic, all image streams now also show a PointStamped display in addition to the Camera/DepthCloud/Image displays. This is almost certainly wrong. And it only happens to image topics which have already been added as a display.

Also, the frame_id of the published points could be copied from the image stream, couldn't it?

Last, the current implementation only allows a single topic for each source image stream. But what if I had e.g. two displays with displaying the same topic but wanted to distinguish which one has been clicked? I understand this is a corner case I don't really face right now, but it might be something to consider.

peci1 · 2024-05-09T12:29:48Z

Also, the timestamp of the click is ros::Time::now(). In some cases, it would be more intuitive to get the timestamp of the currently displayed image.

(Hopefully) last comment: rqt_image_view uses geometry_msgs/Point instead of PointStamped. It's a pity these two are not aligned, but I don't know what way to fix this dichotomy would be better. On one hand, this is a new functionality so it should be easier to bend it, on the other hand, having the header might be useful sometimes.

miguelriemoliveira · 2024-05-09T13:17:31Z

Hi @peci1 ,

I filled this PR about an year ago.
At that time it was quite hard to pick up on the cpp code from rviz, as I usually use python.

At the moment I do not have enough time to go back to this. Perhaps in a couple of weeks it will be possible.

Also, this was already merged, right? So should we work on a different PR?

miguelriemoliveira added 3 commits April 10, 2022 19:32

Mouse click functionality for ImafeDisplay.

f799beb

MouseClick functionality ... missing files.

8648d55

Cleaning up couts ...

20fb106

This was referenced Apr 11, 2022

Clickable image #916

Open

Creating an image display rviz plugin with clicking functionality lardemua/atom#378

Closed

rhaschke requested changes Jul 8, 2022

View reviewed changes

miguelriemoliveira and others added 4 commits July 16, 2022 08:29

Update src/rviz/default_plugin/image_display.h

32f8cb5

Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com>

Update src/rviz/image/mouse_click.cpp

6663a9d

Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com>

Update src/rviz/image/mouse_click.cpp

25fc5c6

Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com>

Update src/rviz/image/mouse_click.cpp

6197411

Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com>

miguelriemoliveira commented Jul 16, 2022

View reviewed changes

rhaschke requested changes Jul 31, 2022

View reviewed changes

(ros-visualization#1737) Trying to implement rhaschke's comments.

0135a80

rhaschke added 5 commits April 25, 2024 10:02

Fix clang-format

22fb342

Cleanup

1a48217

- Rename variables and functions for more clarity - Drop shared_ptr whereever possible - Drop MouseClick::updateTopic()

Drop have_dimensions_ member

8469d18

Allow continouos dragging

b55497d

Install/Remove event filter on enable()/disable()

71c4603

rhaschke merged commit 23d9e81 into ros-visualization:noetic-devel Apr 25, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Image display with mouse clicking functionality #1737

Image display with mouse clicking functionality #1737

miguelriemoliveira commented Apr 11, 2022 •

edited

Loading

miguelriemoliveira commented Apr 20, 2022

rhaschke commented Apr 20, 2022

miguelriemoliveira commented Apr 20, 2022

miguelriemoliveira commented Jul 1, 2022

rhaschke left a comment

rhaschke Jul 8, 2022

miguelriemoliveira Jul 16, 2022

rhaschke Jul 8, 2022

miguelriemoliveira Jul 16, 2022

rhaschke Jul 31, 2022

rhaschke Jul 8, 2022

miguelriemoliveira Jul 16, 2022 •

edited

Loading

rhaschke Jul 31, 2022

miguelriemoliveira Aug 10, 2022 •

edited

Loading

miguelriemoliveira commented Jul 13, 2022

miguelriemoliveira left a comment

miguelriemoliveira commented Jul 16, 2022

rhaschke left a comment

rhaschke Jul 31, 2022

miguelriemoliveira Aug 10, 2022

rhaschke Jul 31, 2022

miguelriemoliveira commented Aug 8, 2022

miguelriemoliveira commented Aug 10, 2022

miguelriemoliveira commented Aug 10, 2022 •

edited

Loading

miguelriemoliveira commented Apr 25, 2024

rhaschke commented Apr 25, 2024

miguelriemoliveira commented Apr 25, 2024

peci1 commented May 9, 2024 •

edited

Loading

peci1 commented May 9, 2024

miguelriemoliveira commented May 9, 2024

		boost::shared_ptr<ros::NodeHandle> node_handle_;
		boost::shared_ptr<ros::Publisher> publisher_;

	MouseClick::MouseClick() : QObject()
	MouseClick::MouseClick(QObject* parent) : QObject(parent)

Image display with mouse clicking functionality #1737

Image display with mouse clicking functionality #1737

Conversation

miguelriemoliveira commented Apr 11, 2022 • edited Loading

Description

Checklist

miguelriemoliveira commented Apr 20, 2022

rhaschke commented Apr 20, 2022

miguelriemoliveira commented Apr 20, 2022

miguelriemoliveira commented Jul 1, 2022

rhaschke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelriemoliveira Jul 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelriemoliveira Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

miguelriemoliveira commented Jul 13, 2022

miguelriemoliveira left a comment

Choose a reason for hiding this comment

miguelriemoliveira commented Jul 16, 2022

rhaschke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelriemoliveira commented Aug 8, 2022

miguelriemoliveira commented Aug 10, 2022

miguelriemoliveira commented Aug 10, 2022 • edited Loading

miguelriemoliveira commented Apr 25, 2024

rhaschke commented Apr 25, 2024

miguelriemoliveira commented Apr 25, 2024

peci1 commented May 9, 2024 • edited Loading

peci1 commented May 9, 2024

miguelriemoliveira commented May 9, 2024

miguelriemoliveira commented Apr 11, 2022 •

edited

Loading

miguelriemoliveira Jul 16, 2022 •

edited

Loading

miguelriemoliveira Aug 10, 2022 •

edited

Loading

miguelriemoliveira commented Aug 10, 2022 •

edited

Loading

peci1 commented May 9, 2024 •

edited

Loading