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

Add option to continuously transform visuals in point cloud plugin #1687

Open
wants to merge 2 commits into
base: noetic-devel
Choose a base branch
from

Conversation

AndreasR30
Copy link
Contributor

Hello,

I made a new pull request, which is essentially the same as #1652, which was merged but then reverted as it broke the visualization of other users. But now the changes are less invasive and do not affect old visualizations as long the option is unchecked (the default). The new option triggers the same behavior as the activated frame_locked flag in the Marker Message.

…w.r.t. fixed frame

This option is particularly useful for messages, whose frame is moving w.r.t. fixed frame.
@rhaschke
Copy link
Contributor

Dear @AndreasR30,
in principle, I support the idea to enable your new feature optionally with a corresponding checkbox. However, the feature should be very well motivated and described. I would like @Martin-Oehler to chime in. Please discuss together, whether this new feature is really necessary. As far as I interpret #1652 (comment), he wasn't convinced yet.
I won't have cycles to dive into this topic any time soon. So, please discuss between the two of you and come up with a consensus.

@Martin-Oehler
Copy link

Hello everyone,

I still don't think, that this feature is necessary. The problem described by @AndreasR30 occurs, because they publish data in the map frame but visualize it in the base link. Therefore, they select a fixed frame for their visualization which is not fixed in regards to the data. You could simply fix your visualization by selecting "map" as your fixed frame and lock the camera to your base link.

The frame_locked parameter in the marker message is valid, because markers don't necessarily visualize data and a moving marker might be useful. The same is not the case for data, where a wrong transform invalidates its meaning.

Best regards,
Martin

@AndreasR30
Copy link
Contributor Author

Hello @Martin-Oehler ,
I understand your first point. However, we (and I think also many others) often want to visualize data at the same time, which is not all in the same frame. For example, I also added an analog pull request for Odometry Display visualizing the Odometry message.

Here we have one Odometry message (red), which represents wheel odometry (dead reckoning) and lives in the odom frame. It describes the pose of base_link in odom frame. At the same time, we have another Odometry message (blue), which represents our ground truth from a inertial navigation system (IMU + GPS) and lives in utm frame. It describes the pose of base_link in utm (or map) frame:

Screenshot from 2021-12-17 14-56-17

With your description it would be possible to visualize one, but not both at the same time in Rviz. However, this is quite useful if you want to compare how well dead reckoning is working w.r.t. ground truth.

Even in the case where your description works because you only want to visualize messages in the same frame, I don't find it optimal when the user has to think about, which fixed frame he has to use to make the visualization work. In my opinion the visualization should work always irrespective of the fact what is the fixed frame. So, for me, the fixed frame option should just change where the camera is fixed to but not whether a visualization works or not.

I don't understand your second point:

The frame_locked parameter in the marker message is valid, because markers don't necessarily visualize data and a moving marker might be useful. The same is not the case for data, where a wrong transform invalidates its meaning.

Can you make an example scenario where you want the marker to move but not the data? Why do you think the transform is "wrong"? I think the transforms used in this pull request are correct.

I admit that this pull request for PointCloud2 is less important, than the one for Odometry display. Because in our scenario it just makes the visualization smoother, when the TF w.r.t. to fixed frame is updated more frequently than the message rate. But I still think it is important for many users. Maybe we can at least agree that it is very useful for Odometry message?

Kind regards,
Andreas

@Martin-Oehler
Copy link

Martin-Oehler commented Dec 21, 2021

With your description it would be possible to visualize one, but not both at the same time in Rviz. However, this is quite useful if you want to compare how well dead reckoning is working w.r.t. ground truth.

You could do the same in the UTM frame, as you can see from the screenshot here, that I took from your bag.

odom_comparison

However, rviz does not handle these high numbers very well. You could zero your utm frame at the starting position and publish a separate NavSatFix message. See also rviz_satellite.

Even in the case where your description works because you only want to visualize messages in the same frame, I don't find it optimal when the user has to think about, which fixed frame he has to use to make the visualization work. In my opinion the visualization should work always irrespective of the fact what is the fixed frame. So, for me, the fixed frame option should just change where the camera is fixed to but not whether a visualization works or not.

Well, with your change, only data that is already transformed to the correct fixed frame will be displayed correctly. All other data is transformed wrong, which is much worse, I think.

I think the transforms used in this pull request are correct.

It is correct for the special case where your data is already transformed to its fixed frame. In all other cases, old data will be transformed with new transforms, leading to wrong results. I can add the equations, if you are interested.

Can you make an example scenario where you want the marker to move but not the data?

No idea honestly. I don't think it makes much sense in either case.

Best regards,
Martin

@AndreasR30
Copy link
Contributor Author

You could do the same in the UTM frame, as you can see from the screenshot here, that I took from your bag.

Not sure what you were doing but in my Rviz the visualization is wrong. It does not show the drift of the odom frame over time. . I also used utm in global options and in Views:

Screenshot from 2021-12-21 19-59-55

However, rviz does not handle these high numbers very well. You could zero your utm frame at the starting position and publish a separate NavSatFix message. See also rviz_satellite.

My pull request handles these high numbers automatically. Why not simply use it without having to introduce new intermediate frames to avoid large TFs. In this scenario I want to compare Odometry Messages against each other and not aerial image against Odometry Message.

Well, with your change, only data that is already transformed to the correct fixed frame will be displayed correctly. All other data is transformed wrong, which is much worse, I think.

No that is wrong. You can visualize the point cloud when the fixed frame != message frame. Message in odom frame and fixed frame is base_link and it still works perfectly:

demo-2021-12-21_19.35.29.mp4

It does not matter which fixed frame you choose. Unlike in the old implementation:

demo-2021-12-20_15.22.01.mp4

Furthermore, I want to stress that this is just an option now, which can be activated if one wants to.

It is correct for the special case where your data is already transformed to its fixed frame. In all other cases, old data will be transformed with new transforms, leading to wrong results. I can add the equations, if you are interested.

I would prefer to have a minimal rosbag available of your Gazebo Simulation, where you show problems with the new implementation. I want to understand where the problem comes from.

No idea honestly. I don't think it makes much sense in either case.

I can tell you where this feature is useful. For example we recorded UTM coordinates while where were driving on our track. Now we want to visualize this static track by means of markers. By using the frame_locked feature it is enough to publish the marker once because its pose w.r.t. fixed frame is continuously re-computed. Here is the code:

#!/usr/bin/env python
import rospkg
import rospy
import scipy.io
import time
from geometry_msgs.msg import Point, Vector3
from std_msgs.msg import ColorRGBA
from visualization_msgs.msg import Markerrospy.init_node('visualize_test')
​
rospack = rospkg.RosPack()
​
path = rospack.get_path('mpc_path_follower') + '/paths/cpg_fast_lap_BagT.mat'
mat = scipy.io.loadmat(path)
​
x = mat['x'][0]
y = mat['y'][0]
​
marker = Marker()
marker.header.stamp = rospy.Time.now()
marker.header.frame_id = "utm"
marker.ns = "path_points"
marker.id = 1
marker.type = Marker.SPHERE_LIST
marker.action = Marker.ADD
marker.pose.position = Point(x[0], y[0], 0)
marker.pose.orientation.w = 1
marker.scale = Vector3(.2, .2, .2)
marker.color = ColorRGBA(1, 0, 0, 1)
marker.lifetime = rospy.Duration(0)
marker.frame_locked = True  # important: tells Rviz plugin to continuously transform into 'fixed_frame' -> no need to publish message periodicallyfor i in range(x.shape[0]):
    marker.points.append(Point(x[i] - marker.pose.position.x, y[i] - marker.pose.position.y, 550))
​
pub = rospy.Publisher('path_points', Marker, queue_size=10)
time.sleep(1)
​
pub.publish(marker)
​
rospy.spin()
path-2021-12-14_16.35.38_short.mp4

We think such a feature would be essential for nearly any visual not only Markers.

Finally, I want to show you that there are multiple Displays, which constantly re-transform into fixed frame in the update step with the latest stamp available:

  1. (As already mentioned) Markers with frame_locked flag.
  2. Robot model display, see here and here.
  3. Axes display: see here
  4. Effort display: see here
  5. Grid display: see here
  6. Map display: While searching I found an option in Map Display "Use Timestamp", which does exactly the same as I do for other visuals, see here.
  7. And much more...

Would you also say that their transforms are wrong/bad?

@rhaschke
Copy link
Contributor

The displays, you mention @AndreasR30, don't display data recorded at a specific timestamp, but they visualize data w.r.t. to the current time. An exception might be the Effort display, which displays effort data at some timestamp but using the current/latest joint pose.

@AndreasR30
Copy link
Contributor Author

@rhaschke and @Martin-Oehler Please give me a rosbag (pointcloud topic, /tf, /tf_static) or a synthetic example, where the new visualization option fails. I am quite sure that there is an issue with the messages' frame_id or stamp. Or with your TF tree.

It's a pity that you simply ignore the issues you can definitely see in the synthetic example above. The point cloud there is in map frame and should clearly stay at the same position w.r.t. the map frame. This means the parent frame has to move in the screen because the camera is moving w.r.t. to map frame. The visualization is only correct at the moments, when there is a new message but in between it is wrong.

@AndreasR30
Copy link
Contributor Author

AndreasR30 commented Dec 22, 2021

@rhaschke Also the Map display shows data from a specific frame and stamp. E.g. you can generate a OccupancyGrid message from a single laser scan. With the option the grid's visualization remains valid even when some time elapsed since the last published message. In general OccupancyGrid and a PointCloud2 are a quite similar thing, when you publish a PointCloud2 in a world fixed frame. Why then not make their visualization similar?

@Martin-Oehler
Copy link

Martin-Oehler commented Dec 23, 2021

Not sure what you were doing but in my Rviz the visualization is wrong. It does not show the drift of the odom frame over time. . I also used utm in global options and in Views:

From your screenshot, I am not sure what you are doing differently. But since both odom paths will end at the current robot position by definition and they are not the same path, you are bound to see the drift.

My pull request handles these high numbers automatically. Why not simply use it without having to introduce new intermediate frames to avoid large TFs. In this scenario I want to compare Odometry Messages against each other and not aerial image against Odometry Message.

That's a nice addition then. You should probably split these changes into a separate PR. However, it won't fix the rviz camera view, which for me did not work properly anymore.

No that is wrong. You can visualize the point cloud when the fixed frame != message frame. Message in odom frame and fixed frame is base_link and it still works perfectly:

The odom frame is still a fixed frame with regards to the data. Your changes break visualization, when the data is in the base link, e.g. data from a lidar or depth camera.

I would prefer to have a minimal rosbag available of your Gazebo Simulation, where you show problems with the new implementation. I want to understand where the problem comes from.

Okay, here you go: https://drive.google.com/drive/folders/17E2ejoBJm8qcFWe0rZeAEmz0XQKXY8ql?usp=sharing

The bag contains a lidar scan of a rotating VLP-16, published in the laser frame. An rviz config is provided.

This video shows the visualization with the correct behavior previous to your patch:

correct_visualization.mp4

This video shows the broken behavior with "Continuous Transform" enabled:

wrong_visualization_small.mp4

Also the Map display shows data from a specific frame and stamp.

The map is different because it does not represent data from a specific sensor. It is expected to be published in a world-fixed frame, unlike point cloud data.

@rhaschke
Copy link
Contributor

Not sure what you were doing but in my Rviz the visualization is wrong.

From your screenshot, I am not sure what you are doing differently.

Maybe, provide your rviz config, @Martin-Oehler?

@Martin-Oehler
Copy link

Not sure what you were doing but in my Rviz the visualization is wrong. It does not show the drift of the odom frame over time. . I also used utm in global options and in Views:

Oh well, I did some more investigation and that one is on me. For some reason, our build server delivered on out-dated version of rviz which actually still had the proposed change from #1686. I agree, that this change makes sense for odometry, but we should discuss this in the other PR.

The main difference is that point cloud data can be from a fixed frame (map, odom) or from a frame attached to the robot body. Odom is typically published in some fixed frame.

I put a bit more thought into this and think that the new behavior is fine as an option. Using the timestamp is still the more general way, since we don't know the origin of the point cloud, but if the user knows, that the cloud is already given in a fixed frame, this feature can be useful. However, I would change the name of the option to something like "Ignore timestamp" to make the naming in line with the option in the Map display (which is "Use timestamp" and does the opposite).

@AndreasR30
Copy link
Contributor Author

AndreasR30 commented Dec 28, 2021

@Martin-Oehler Thank you very much for providing your rosbag. It looks fine. Very interesting application of the lidar sensor by the way.

I think our discussion mainly comes from the fact that we follow different philosophies for visualization:
In my opinion, sensor data with a frame, which is attached to the robot, should move with the robot. And only sensor data with a world fixed frame should stick to the world.

This, of course, can lead to strange behavior (as one see in your videos). In my philosophy, it would be necessary to transform the sensor data into a world fixed frame based on its time stamp. (That is why the time stamp can be ignored afterwards). I understand it can be annoying to do this extra step. However, I think this is the more powerful and flexible approach as it can handle multiple (pseudo-) fixed frames like odom and utm frames in the odometry example above. Also a multi robot scenario, where each has its own lidar sensor and odom frame, can be handled. Furthermore, I guess, in order to generate an accumulated point cloud, as in the video above, which can be used in a downstream perception task, you have to transform the individual scans to a world fixed frame anyway (because of sensor motion).

However, I also understand your approach, because one gets a nice visualization quite quickly without having to transform sensor data before. It works well when there is only one fixed frame to consider, which is the typical use case with point clouds. A very big reason, why we do not work with Views -> Current View -> Target Frame option (we always set it to <Fixed Frame>), which is an important part in @Martin-Oehler's approach, is that we observe jittering of movable links in our robot model, when the Target Frame != Fixed Frame:

https://www.dropbox.com/s/aefs56f6zx0nt9g/jittering.mp4?dl=0

vs:

https://www.dropbox.com/s/fp0odav15knkz4c/no_jittering.mp4?dl=0

However, this is not a problem of @Martin-Oehler's approach itself but rather because of problems with robot model display. We are already working on a pull request, which fixes this.

I would be happy to get get this pull request accepted, so the user can decide whether he or she wants to transform the sensor data into world fixed frame by him-/herself in order to be more flexible or whether rviz should do this.

I put a bit more thought into this and think that the new behavior is fine as an option. Using the timestamp is still the more general way, since we don't know the origin of the point cloud, but if the user knows, that the cloud is already given in a fixed frame, this feature can be useful. However, I would change the name of the option to something like "Ignore timestamp" to make the naming in line with the option in the Map display (which is "Use timestamp" and does the opposite).

I am happy that you understand why this option can be useful for some applications. I also agree that your approach is the more typical one for point clouds. So the option should be off by default.

I'm not completely happy with "Ignore timestamp" / "Use timestamp" naming, but I can accept it to be consistent with other displays.

@rhaschke What do you think?

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