search

nobleo / rviz_satellite

Display internet satellite imagery in RViz
Apache License 2.0
530 stars 226 forks source link

Use frame of NavSatFixMsg #35

Closed m-naumann closed 4 years ago

m-naumann commented 7 years ago

Hello Gareth,

great work, I really like this plugin!

Concerning the robot frame property, I misunderstood the plugin: I expected the plugin to read the frame that corresponds to the (lat,lon)-coordinates from the NavSatFix message, as the latter contains that information (NavSatFix). With this, I thought the robot frame was there to load tiles around an area where no GPS-coordinates are explicitely available, such as in simulation, where only Cartesian x and y (using UTM as in ROS geodesy for example) are at hand.

Therefore, I would suggest adding this feature, using the upper ROS library, or set the robot frame property read-only (and read it from the NavSatFix message). I could do either and open a merge request, just let me know what you prefer.

Cheers Max

CesMak commented 7 years ago

Hey m-naumann, I think I would like to do the same as you: I want the coordinate system to be fixed! Plot the latitude longitude position of the robot in the map. And just load those tiles of the that are required if the robot goes over the edges ....

Best Markus

PS: you can also write me an email at 2f4yor@gmail.com

m-naumann commented 7 years ago

Hey Markus,

what the plugin currently does is fixing the (lat,lon) of the NavSatFixMsg to the frame robot_frame.

I think it should

and 1) either load the tiles only around the frame of the NavSatFixMsg and make the robot frame read only 2) or load the tiles around the frame robot frame, using (lat,lon) corrdinates that are calculated from the deviation of the robot frame to the NavSatFixMsg-frame via UTM

I don't know the validity range of the latlon2xy conversion used here:

so I cannot say whether it's okay to use UTM, or how far the distance in between the robot frame and the NavSatFixMsg-frame might be. So maybe it's better to chose option 1.

Do you agree Markus and Gareth?

schra commented 5 years ago

@m-naumann I rewrote parts of how the frames are being transformed, see the current implementation. Can you confirm that this issue still applies to the current version 1.2.0?

Furthermore, I don't really understand the motivation behind this issue. If it only concerns simulation, couldn't you just publish GPS coordinates inside your simulation?

m-naumann commented 5 years ago

Hi @schra

yes, the problem is still the same: you ignore the frame of the NavSatFix-Message and do not list it in the documentation link (current implementation).

There are four relevant frames: The robot's frame, Rviz's fixed frame, the ENU/ NED/ NWU world fixed frame, and the tile frame.

The interesting point is the robot frame: It has two purposes: 1. You don't want to load the tiles of the whole world and 2. You need to know one "base point" for the lat-lon to x-y conversion.

Technically, this can be two separate things: a NavSatFix for the "base point", and a robot_frame_id to load the tiles around. What you do is linking the manually defined robot_frame_id to the NavSatFix-lat-lon-Coordinates and ignoring the frame_id of that message.

What I proposed is

  1. either load the tiles only around the frame of the NavSatFixMsg and make the robot_frame_id read only
  2. or load the tiles around the frame robot_frame_id , using (lat,lon) corrdinates that are calculated from the deviation of the robot frame to the NavSatFixMsg-frame via UTM or any other projection; see for example lanelet2_projection

From the NavSatFixMsg + a projection method, the map_frame (which I would rather call rviz_satellite_map because there might be different valid map frames) is defined. The tile frame is a frame you don't even define (if I am correct), so I wouldn't mention it.

Furthermore, I don't really understand the motivation behind this issue. If it only concerns simulation, couldn't you just publish GPS coordinates inside your simulation?

The motivation is that currently, the NavSatFixMsg is misused. What I did for using it in simulation is exactly publishing a NavSatFixMsg, but if users changed the robot frame, the hoped that the tiles where loaded according to the position of the robot (Option 2 from above). But what actually happened was that the tiles were attached to the robot.

If not in simulation, you most likely have GPS with something around 1Hz, but some 100Hz Odometry. Currently, the tiles would follow the robot 99 times and at the 100th time, the tile jumps away under the robot.

I hope that this helps clarifying . Feel free to ask further questions.

beetleskin commented 5 years ago

I don't quite understand the problem you describe with the current setup. Whatever robot-fixed frame I set, I can't observe any glitches or jumps, even with setting the gps rate to 1 and tf-updates to 100. Can you provide a basic setup/video to reproduce what you mean? Its hard without anything visual. Despite that, are you really sure you're using the latest version? What fixed_frame, what camera frame are you using?

However, its seems indeed a flaw that the NavSatFixMsg's frame_id is ignored. That one should be used instead of the user-selected one. Or is there any specific reason to select this frame? So I vote for option 1:

either load the tiles only around the frame of the NavSatFixMsg and make the robot_frame_id read only

@m-naumann would you be willing to provide a PR for that?

RonaldEnsing commented 5 years ago

I've created this PR #53. As far as I know the frame_id can safely be deduced from the frame_id in the header of the gps message. Please let me know if I misunderstood.

m-naumann commented 5 years ago

You are right, that's option 1. The only thing I would change is keep the robot frame property but make it read-only, such that one sees what frame is currently used.

RonaldEnsing commented 5 years ago

I think that making robot frame a read-only property would also be fine. However, wouldn't having a robot frame property at all, may already be confusing? I think that it would be safe to expect that the frame_id of the Topic would be used, like for other common rviz plugins.

If there would be a selectable frame property, then I would expect it to be an option for world frame. Somehow the AerialMapDisplay needs to know which frame is aligned with geographic coordinates (ENU/NED/NWU). This is currently hard-coded to be map [1]. However, I think it's a good default value, since it conforms to REP-105 [2].

In addition, I also observe the tile jumping a little bit, probably because the frequencies of tf and the NavSatFix do not match. It happens with my fixed frame is set to the (ENU-aligned) map frame, subscribing to a gps/filtered topic containing the filtered location robot’s world frame position, transformed into GPS coordinates, which must be consistent with the map->robot tf's [3].

[1] https://github.com/gareth-cross/rviz_satellite/blob/master/src/aerialmap_display.cpp#L494 [2] https://www.ros.org/reps/rep-0105.html [3] http://docs.ros.org/melodic/api/robot_localization/html/navsat_transform_node.html

beetleskin commented 5 years ago

I think that making robot frame a read-only property would also be fine. However, wouldn't having a robot frame property at all, may already be confusing? I think that it would be safe to expect that the frame_id of the Topic would be used, like for other common rviz plugins.

If there would be a selectable frame property, then I would expect it to be an option for world frame. Somehow the AerialMapDisplay needs to know which frame is aligned with geographic coordinates (ENU/NED/NWU). This is currently hard-coded to be map [1]. However, I think it's a good default value, since it conforms to REP-105 [2].

Sounds reasonable. I vote for using the _frameid of navsat for the robot frame (I agree, this is fair to assume and default everywhere) - and either replace "robot frame" with "map frame", or remove it completely and set "map" as default (as is).

In addition, I also observe the tile jumping a little bit, probably because the frequencies of tf and the NavSatFix do not match. It happens with my fixed frame is set to the (ENU-aligned) map frame, subscribing to a gps/filtered topic containing the filtered location robot’s world frame position, transformed into GPS coordinates, which must be consistent with the map->robot tf's [3].

@schra we also observed this, would you open an issue'?

m-naumann commented 5 years ago

If there would be a selectable frame property, then I would expect it to be an option for world frame. Somehow the AerialMapDisplay needs to know which frame is aligned with geographic coordinates (ENU/NED/NWU). This is currently hard-coded to be map [1]. However, I think it's a good default value, since it conforms to REP-105 [2].

Now I am a little confused: The purpose of the NavSatFixMsg is to provide exactly this information, or am I wrong? It states which frame('s origin 0,0,0) corresponds to which geographic(lat/lon/ele) coordinates. If there was one fixed transformation, such as "the UTM base frame is map", then why do you need a NavSatFixMsg, you can as well load tiles around a PoseStamped (by computing where this Pose is within the map frame and re-projecting these x/y/z coordinates into lat/lon/ele). Right?

In addition, I also observe the tile jumping a little bit, probably because the frequencies of tf and the NavSatFix do not match. It happens with my fixed frame is set to the (ENU-aligned) map frame, subscribing to a gps/filtered topic containing the filtered location robot’s world frame position, transformed into GPS coordinates, which must be consistent with the map->robot tf's [3].

That's why I rather prefer the above mentioned "option 2", where this fixed transformation is set by the NavSatFixMsg (which should not change), but in order to determine which tiles you load, you look at where the robot frame is with respect to the NavSatFixMsg-frame

RonaldEnsing commented 5 years ago

Now I am a little confused: The purpose of the NavSatFixMsg is to provide exactly this information, or am I wrong? It states which frame('s origin 0,0,0) corresponds to which geographic(lat/lon/ele) coordinates. If there was one fixed transformation, such as "the UTM base frame is map", then why do you need a NavSatFixMsg, you can as well load tiles around a PoseStamped (by computing where this Pose is within the map frame and re-projecting these x/y/z coordinates into lat/lon/ele). Right?

The NavSatFix message does not provide orientation. If you would be able to configure the fixed frame to any frame (#54), then the visualization could have an incorrect orientation. Suppose that you would set your fixed frame to the frame of the NavSatFix (the robot) and your robot moves (rotates) around a corner, the visualization will not rotate accordingly, making it look like the robot moves sideways.

I think that using only the tf's, including utm and a selectable robot frame, and avoiding the use of the NavSatFix at all, would also avoid the jumping behaviour. Or is this also what you mean with the statement below?

That's why I rather prefer the above mentioned "option 2", where this fixed transformation is set by the NavSatFixMsg (which should not change), but in order to determine which tiles you load, you look at where the robot frame is with respect to the NavSatFixMsg-frame

RonaldEnsing commented 5 years ago

@m-naumann You may want to take a look at this commit (https://github.com/tud-cor/rviz_satellite/commit/a8d2e207a53a13e46424c5c7c36a8c41e23df6a1) which requires a tf containing utm frame which is connected to your robot, downloads the tile according to the NavSatFix messsage, and allows any frame to be selected as the fixed frame.

This basically means that all that the NavSatFix does is defining which tile to use. Transforming the tile to the right position and orientation is done using tf.

m-naumann commented 5 years ago

@RonaldEnsing I implemented my suggestion starting from what you did. See the PR for details.

https://github.com/gareth-cross/rviz_satellite/pull/55

beetleskin commented 5 years ago

I think that using only the tf's, including utm and a selectable robot frame, and avoiding the use of the NavSatFix at all, would also avoid the jumping behaviour. Or is this also what you mean with the statement below?

Why would that prevent the jumping?

RonaldEnsing commented 5 years ago

I think that using only the tf's, including utm and a selectable robot frame, and avoiding the use of the NavSatFix at all, would also avoid the jumping behaviour. Or is this also what you mean with the statement below?

Why would that prevent the jumping?

Suppose that your NavSatFix sensor is rigidly attached to your robot, your fixed frame is set to map and you publish the map->robot using tf. The tile should now have a static position in rviz (until you drive into another tile number of course), and the robot moves. Every time a NavSatFix message is received, the tile is transformed and visualized. Both tf [1] and the NavSatFix message itself [2] is used to transform the tile. The tf did not yet have the time to update based on the latest NavSatFix message, while you do transform the tile based on the latest NavSatFix message and an older tf.

This [3] modified version of the AerialMapDisplay fixes that. And PR https://github.com/gareth-cross/rviz_satellite/pull/55 is a continuation of that. I still need to take a closer look at PR https://github.com/gareth-cross/rviz_satellite/pull/55.

[1] https://github.com/gareth-cross/rviz_satellite/blob/master/src/aerialmap_display.cpp#L518 [2] https://github.com/gareth-cross/rviz_satellite/blob/master/src/aerialmap_display.cpp#L547 [3] https://github.com/tud-cor/rviz_satellite/commit/a8d2e207a53a13e46424c5c7c36a8c41e23df6a1

schra commented 5 years ago

What I proposed is

  1. either load the tiles only around the frame of the NavSatFixMsg and make the robot_frame_id read only
  2. or load the tiles around the frame robot_frame_id , using (lat,lon) corrdinates that are calculated from the deviation of the robot frame to the NavSatFixMsg-frame via UTM or any other projection; see for example lanelet2_projection

Just to summarize the current situation:

If there would be a selectable frame property, then I would expect it to be an option for world frame.

According to http://www.ros.org/reps/rep-0105.html#map the map frame has to exist. Thus I don't see why we should make this frame configurable?

@schra we also observed this, would you open an issue?

I created #56

m-naumann commented 5 years ago

What I proposed is

  1. either load the tiles only around the frame of the NavSatFixMsg and make the robot_frame_id read only
  2. or load the tiles around the frame robot_frame_id , using (lat,lon) corrdinates that are calculated from the deviation of the robot frame to the NavSatFixMsg-frame via UTM or any other projection; see for example lanelet2_projection

Just to summarize the current situation:

* #53 is implementing the first proposal. Additionally it also completely removes the robot_frame_id which I also prefer.

* #55 is implementing the second proposal.

If there would be a selectable frame property, then I would expect it to be an option for world frame.

According to http://www.ros.org/reps/rep-0105.html#map the map frame has to exist. Thus I don't see why we should make this frame configurable?

@schra we also observed this, would you open an issue?

I created #56

That's true, however #55 also allows for the first option, by simply setting the robot frame to the NavSatFix frame.

RonaldEnsing commented 5 years ago

If there would be a selectable frame property, then I would expect it to be an option for world frame.

According to http://www.ros.org/reps/rep-0105.html#map the map frame has to exist. Thus I don't see why we should make this frame configurable?

I agree. It does not have to be configurable. I could have emphasized the "If". I wanted to make clear that I do not expect a configurable robot frame if it has to be set to the frame_id of the NavSatFix message anyway. However, in #35, the robot frame is sometimes referred to as the frame to load the tiles around. See this https://github.com/gareth-cross/rviz_satellite/issues/35#issuecomment-472070957 for more details:

The interesting point is the robot frame: It has two purposes: 1. You don't want to load the tiles of the whole world and 2. You need to know one "base point" for the lat-lon to x-y conversion."

PR https://github.com/gareth-cross/rviz_satellite/pull/53 is much simpler because it keeps the current behavior of loading the tiles around the NavSatFix instead of a selectable frame. I think that PR https://github.com/gareth-cross/rviz_satellite/pull/53 is quite safe to merge. PR https://github.com/gareth-cross/rviz_satellite/pull/55 is basically a continuation of PR https://github.com/gareth-cross/rviz_satellite/pull/53. It re-introduces the robot frame, but then with another meaning: The frame to download the tiles around.

m-naumann commented 5 years ago

If there would be a selectable frame property, then I would expect it to be an option for world frame.

IMHO the world frame cannot be configurable, because the NavSatFix defines it.

PR #53 is much simpler because it keeps the current behavior of loading the tiles around the NavSatFix instead of a selectable frame. I think that PR #53 is quite safe to merge. PR #55 is basically a continuation of PR #53. It re-introduces the robot frame, but then with another meaning: The frame to download the tiles around.

True, PR #53 keeps the current behavior regarding where to load the tiles.

But it also regards rviz' fixed frame as the frame where the ENU (or any other) convention is applied. Thus, it contraints the usage a lot, because you cannot set any moving frame as fixed frame (for example the frame of LIDAR scanner on a vehicle, because you want to speed up the pointcloud visualization). So it has strong assumtions regarding the fixed frame, which are fixed again in #55

RonaldEnsing commented 5 years ago

IMHO the world frame cannot be configurable, because the NavSatFix defines it.

The NavSatFix gives the geographic coordinates of the receiver (the associated frame_id) [1]: "_header.frameid is the frame of reference reported by the satellite receiver, usually the location of the antenna. This is a Euclidean frame relative to the vehicle, not a reference ellipsoid." A map or world frame would be static w.r.t. geographic coordinates. I agree that we can expect users to follow REP 105 [2] regarding the naming convention of their tf frames so that it does not need to be configurable.

PR #53 is much simpler because it keeps the current behavior of loading the tiles around the NavSatFix instead of a selectable frame. I think that PR #53 is quite safe to merge. PR #55 is basically a continuation of PR #53. It re-introduces the robot frame, but then with another meaning: The frame to download the tiles around.

True, PR #53 keeps the current behavior regarding where to load the tiles.

But it also regards rviz' fixed frame as the frame where the ENU (or any other) convention is applied. Thus, it contraints the usage a lot, because you cannot set any moving frame as fixed frame (for example the frame of LIDAR scanner on a vehicle, because you want to speed up the pointcloud visualization). So it has strong assumtions regarding the fixed frame, which are fixed again in #55

I agree that this version has contraints. However, these constraints are not the result of PR https://github.com/gareth-cross/rviz_satellite/pull/53, but these constraints were already there. I think it wouldn't be a bad idea to incrementally fix different issues.

[1] http://docs.ros.org/melodic/api/sensor_msgs/html/msg/NavSatFix.html [2] https://www.ros.org/reps/rep-0105.html

m-naumann commented 5 years ago

I agree that this version has contraints. However, these constraints are not the result of PR #53, but these constraints were already there. I think it wouldn't be a bad idea to incrementally fix different issues.

So then, why not use #55 which extends #53 which extends the current version?

I just want to use the NavSatFix message + orientation convention to clearly and uniquely define the geo-coordinates to tf conversion/projection without any further specifications that confuse people.

What you propose is to use some parts of the NavSatFix and then also other frames, tf's, ... If you provide a NavSatFix with utm as base frame for example, that's a conflict. What do you do then?

beetleskin commented 4 years ago

@schra this is done, right? with which commit/PR?

schra commented 4 years ago

53 fixed the original problem, so we can close this issue. As we don't have an application for having a separate frame to load the frames in, I don't see that we will add this feature in this repository. However, we could continue discussion in another issue if this is something that you really want to have included.