Bugfix frame usage #55

m-naumann · 2019-05-18T00:23:48Z

This is what it does:

It connects the base frame of the NavSatFix to the geo coordinates (lat/lon) of the NavSatFix, assuming xyz->enu.
It looks where the robot is (tf frame given), computes where the robot is in geo coordinates (lat/lon), based on the UTM difference to the NavSatFix base
It loads the tiles underneath the robot frame

See the demo launchfile for an example with 4 frames:

fixed_frame: the arbitrarily chosen fixed frame in rviz
map: some base frame you might want to use
nav_sat_fix_base: the one frame you know the GPS coordinate, that's where XYZ > ENU is assumed
robot: the robot that moves around in the world, orientation does not matter for loading the tiles, as XYZ > ENU is assumed for the above nav_sat_fix_base

BTW, that is "Option 2" as explained here: #35 (comment)

use the NavSatFix as link between geo coordinates and Cartesian, update readme and demo accordingly, closes #35

RonaldEnsing · 2019-05-20T12:49:08Z

Would this also work regarding the orientation if your NavSatFix sensor is rigidly attached to your robot?

beetleskin · 2019-05-20T13:56:44Z

Thanks for the contribution! But I don't get why you pull in another frame (nav_sat_fix_base) and do the complex computation of "where is my robot relative to nav_sat_fix_base expressed in WGS84 coordinates".
Why is it not sufficient to query tiles with origin at the navsat-fix header frame id?

Further, why are you deleting FRAME_CONVENTION_XYZ_NED and FRAME_CONVENTION_XYZ_NWU support?

RonaldEnsing · 2019-05-20T15:14:25Z

Would it be an idea to let the user of this plugin decide on how to use (or fuse) the NavSatFix and the orientation of the NavSatFix device outside of this plugin? The user would then be able to decide to either fuse sensor information, e.g. with the robot localization package [1], or simply transform the altitude and longitude to UTM coordinates, and publish it as a tf. The AerialMapDisplay would require less configuration in rviz. It would also reduce mistakes in the configuration of the plugin since you do not need to set the frames to the 'right' values. Since only tf will be used, it not suffer from this jumping behavior and it will also allow any fixed frame to be selected. It does require the user to publish a utm frame.

[1] http://wiki.ros.org/robot_localization

m-naumann · 2019-05-20T17:12:51Z

Would this also work regarding the orientation if your NavSatFix sensor is rigidly attached to your robot?

yes, there are no constraints regarding the frame, just keep in mind that if you do that, the orientation of the robot is defined as "north", which is probably not what you want

Thanks for the contribution! But I don't get why you pull in another frame (nav_sat_fix_base) and do the complex computation of "where is my robot relative to nav_sat_fix_base expressed in WGS84 coordinates".
Why is it not sufficient to query tiles with origin at the navsat-fix header frame id?

To avoid the often mentioned "jumping" and to avoid the need for multiple NavSatFix publishers if you want to follow different robots. (In an automated driving application for example, you don't need to publish the NavSatFix for every car, not even for one car, but you just publish a static one of somewhere in your map, and the plugin always loads the tiles from where the car is).

Further, why are you deleting FRAME_CONVENTION_XYZ_NED and FRAME_CONVENTION_XYZ_NWU support?

In order to keep the transformations simple, if you want another frame convention, you can simply rotate the nav_sat_fix_base. But you can add it again if you like, @RonaldEnsing was actually removing it, I just added the only available option as "option" again, as a hint to users of the previous version.

Would it be an idea to let the user of this plugin decide on how to use (or fuse) the NavSatFix and the orientation of the NavSatFix device outside of this plugin? The user would then be able to decide to either fuse sensor information, e.g. with the robot localization package [1], or simply transform the altitude and longitude to UTM coordinates, and publish it as a tf. The AerialMapDisplay would require less configuration in rviz. It would also reduce mistakes in the configuration of the plugin since you do not need to set the frames to the 'right' values. Since only tf will be used, it not suffer from this jumping behavior and it will also allow any fixed frame to be selected. It does require the user to publish a utm frame.

Instead of the NavSatFix you could also ask for the UTM base frame, but then you would also have to ask for the UTM zone, which I found less convenient than the NavSatFix message.

schra

I agree that it's not a good idea to require the frame nav_sat_fix_base to be in ENU.

Could you elaborate on why you are using UTM in the first place? a8d2e20 says that it's due to allow a selectable rviz fixed frame (what does this mean?)

Personally, I prefer #53 over this PR. It's much simpler. Could you elaborate on the additional value of this PR in contrast to #53?

schra · 2019-05-20T17:46:30Z

src/aerialmap_display.cpp

-      new RosTopicProperty("Topic", "", QString::fromStdString(ros::message_traits::datatype<sensor_msgs::NavSatFix>()),
-                           "nav_msgs::Odometry topic to subscribe to.", this, SLOT(updateTopic()));
+  topic_property_ = new RosTopicProperty(
+      "NavSatFix", "", QString::fromStdString(ros::message_traits::datatype<sensor_msgs::NavSatFix>()),


I wouldn't rename the property from Topic to NavSatFix in order to keep backwards compatibility.

Well, but "topic" is not the best name for a plugin that does not simply visualize a message. What kind of backwards compatibility do you mean?

schra · 2019-05-20T17:47:24Z

src/aerialmap_display.cpp

@@ -96,11 +100,11 @@ AerialMapDisplay::AerialMapDisplay() : Display(), dirty_(false), received_msg_(f
  blocks_property_->setMax(maxBlocks);

  frame_convention_property_ =
-      new EnumProperty("Frame Convention", "XYZ -> ENU", "Convention for mapping cartesian frame to the compass", this,
-                       SLOT(updateFrameConvention()));
+      new EnumProperty("Frame Convention", "XYZ -> ENU",


I don't see the value of having a Frame Convention property with only one option.

That's a hint to people previously using other frame conventions, and I prefer it over simply deleting this option without telling which one is used now.

schra · 2019-05-20T17:49:12Z

src/aerialmap_display.cpp


-  frame_property_ = new TfFrameProperty("Robot frame", "world", "TF frame for the moving robot.", this, nullptr, false,
-                                        SLOT(updateFrame()), this);
+  frame_property_ = new TfFrameProperty("Robot frame", "robot", "TF frame for the moving robot, must be connected to "


Due to backwards compatibility this name shouldn't be Robot frame and something different.

Well this actually is the robot frame (for the first time), so why shouldn't it be named that way?

schra · 2019-05-20T17:51:22Z

src/aerialmap_display.cpp


-  frame_property_ = new TfFrameProperty("Robot frame", "world", "TF frame for the moving robot.", this, nullptr, false,
-                                        SLOT(updateFrame()), this);
+  frame_property_ = new TfFrameProperty("Robot frame", "robot", "TF frame for the moving robot, must be connected to "


What is your reasoning behind making Robot frame configurable? Isn't this basically always the frame map?

No, it is the frame of the robot, below which you want the tiles to be loaded

schra · 2019-05-20T17:54:33Z

src/aerialmap_display.cpp

+    setStatus(StatusProperty::Error, "Transform", QString::fromStdString(errMsg));
+    setStatus(StatusProperty::Error, "README",
+              QString::fromStdString("Tiles might be visible, but in the wrong place!"));
+    // todo: replace this readme by hiding the tiles in a way


This todo has to be fixed.

True, but it's not an issue I introduced, it always existed, and I do want to contribute, yet not fix everything ;)

see #51

m-naumann · 2019-05-20T18:17:58Z

I agree that it's not a good idea to require the frame nav_sat_fix_base to be in ENU.

Feel free to add that functionality, again, I didn't remove it.

Could you elaborate on why you are using UTM in the first place? a8d2e20 says that it's due to allow a selectable rviz fixed frame (what does this mean?)

Personally, I prefer #53 over this PR. It's much simpler. Could you elaborate on the additional value of this PR in contrast to #53?

Yes it is simpler, but also has less functionality. If you have a robot moving around in the world, it does not necessarily carry a GPS receiver, still you might want to visualize the environment in which it is moving. So you provide one GPS location in the area, and with UTM we can always compute the correct tiles to be loaded. If you have multiple robots running around in your setup, they can currently not be far away from each other, because a maximum of 8 tiles is loaded around the given position. With this PR, you can simply change the robot frame.

Tiles are in lat/lon, tf is Cartesian, so x/y -> we need a conversion/projection, and UTM is the most common one. See https://github.com/m-naumann/rviz_satellite/blob/bugfix_frame_usage/src/aerialmap_display.cpp#L556-L582, it's not that complicated I would say.

RonaldEnsing · 2019-05-21T10:58:42Z

yes, there are no constraints regarding the frame, just keep in mind that if you do that, the orientation of the robot is defined as "north", which is probably not what you want

In your example, demo.launch, the /gps/fix (NavSatFix) topic has frame_id set to nav_sat_fix_base. A common setup is to have the NavSatFix sensor attached to the robot. If such a configuration would result in an incorrect orientation of the robot, then I would call that a constraint.

In an automated driving application for example, you don't need to publish the NavSatFix for every car, not even for one car, but you just publish a static one of somewhere in your map, and the plugin always loads the tiles from where the car is

You wouldn't need to publish the NavSatFix for every car. However, this PR seems to require a static tf somewhere in your map. Why not publish UTM as a tf instead of the nav_sat_fix_base?

In order to keep the transformations simple, if you want another frame convention, you can simply rotate the nav_sat_fix_base. But you can add it again if you like, @RonaldEnsing was actually removing it, I just added the only available option as "option" again, as a hint to users of the previous version.

I removed it because I only tested tud-cor@a8d2e20 in the case of ENU convention. My intention was to add it again if tested for NED/NWU.

Instead of the NavSatFix you could also ask for the UTM base frame, but then you would also have to ask for the UTM zone, which I found less convenient than the NavSatFix message.

I agree that it would not be convenient to require the user to configure a UTM frame, since that information can be derived from a NavSatFix. Because of that, I think that it is a good idea to keep the property to select a NavSatFix.

Could you elaborate on why you are using UTM in the first place? a8d2e20 says that it's due to allow a selectable rviz fixed frame (what does this mean?)

You can choose the 'fixed frame' in rviz in the 'Displays' window in the 'Global Options' field.

m-naumann · 2019-05-21T16:40:27Z

yes, there are no constraints regarding the frame, just keep in mind that if you do that, the orientation of the robot is defined as "north", which is probably not what you want

In your example, demo.launch, the /gps/fix (NavSatFix) topic has frame_id set to nav_sat_fix_base. A common setup is to have the NavSatFix sensor attached to the robot. If such a configuration would result in an incorrect orientation of the robot, then I would call that a constraint.

Well, the question is, what is the alternative?

You have to specify the coordinate system, in which the ENU (or whatever convention) is applied. What you are doing, is just assuming that it applies to the "fixed frame" of rviz, meaning that if someone changes the fixed frame to the vehicle frame, for example to speed up point cloud visualizations around the car, then the plugin draws the map with the wrong orientation, as stated above. And with your implementation, you can't do anything about that. My implementation, however, allows for any correct usage of the frames, without constraining rviz' fixed frame, for example.

In an automated driving application for example, you don't need to publish the NavSatFix for every car, not even for one car, but you just publish a static one of somewhere in your map, and the plugin always loads the tiles from where the car is

You wouldn't need to publish the NavSatFix for every car. However, this PR seems to require a static tf somewhere in your map. Why not publish UTM as a tf instead of the nav_sat_fix_base?

You always need a tf between the frame you draw the things around and the fixed frame of rviz, so similar to your implementation, you always need a tf except if fixed frame, nav_sat_fix_base and the robot frame are identical. If the latter is the case, however, the map is always drawn ENU in this frame, which again is probably not what you like.

Instead of the NavSatFix you could also ask for the UTM base frame, but then you would also have to ask for the UTM zone, which I found less convenient than the NavSatFix message.

I agree that it would not be convenient to require the user to configure a UTM frame, since that information can be derived from a NavSatFix. Because of that, I think that it is a good idea to keep the property to select a NavSatFix.

ok

RonaldEnsing · 2019-05-21T17:18:29Z

In your example, demo.launch, the /gps/fix (NavSatFix) topic has frame_id set to nav_sat_fix_base. A common setup is to have the NavSatFix sensor attached to the robot. If such a configuration would result in an incorrect orientation of the robot, then I would call that a constraint.

Well, the question is, what is the alternative?

The alternative would be to let the user of this plugin decide on how to use the NavSatFix message and any orientation sensor to create a utm -> robot connected tf tree. This is use case specific. A NavSatFix message is an incomplete description of the robot pose, since it does not include orientation. I would avoid making assumptions about the use case or orientations of particular frames.

You have to specify the coordinate system, in which the ENU (or whatever convention) is applied. What you are doing, is just assuming that it applies to the "fixed frame" of rviz, meaning that if someone changes the fixed frame to the vehicle frame, for example to speed up point cloud visualizations around the car, then the plugin draws the map with the wrong orientation, as stated above. And with your implementation, you can't do anything about that. My implementation, however, allows for any correct usage of the frames, without constraining rviz' fixed frame, for example.

I am not making assumptions about the fixed frame in rviz. In fact, the reason why I created tud-cor@a8d2e20 is to allow any fixed frame to be selected. However, tud-cor@a8d2e20 does require the utm->robot tf's to be setup. The utm frame includes orientation, in my case ENU convention. With tud-cor@a8d2e20, I am able to select any fixed frame and get the tiles in the right orientation. In addition, tud-cor@a8d2e20 does not suffer from #56.

You always need a tf between the frame you draw the things around and the fixed frame of rviz, so similar to your implementation, you always need a tf except if fixed frame, nav_sat_fix_base and the robot frame are identical. If the latter is the case, however, the map is always drawn ENU in this frame, which again is probably not what you like.

The point I wanted to make is that I think that it would make sense to connect utm using tf over an arbitrary nav_sat_fix_base.

m-naumann · 2019-05-21T17:38:39Z

A NavSatFix message plus orientation convention, such as ENU, fully describes everything you need.

Why using some extra UTM, maybe some people use UTM with an offset, as OGRE uses float and not double, which is only centimeter accurate in some UTM tiles. That's all not necessary, when you just add the orientation convention to the NavSatFix, of course the convention applies to the frame specified within that message, I often call it nav_sat_fix_base, with only the name being arbitrary ;)

The need for a tffrom your rviz fixed frame to the nav_sat_fix_base is always required when using the NavSatFix as intended.

This PR of course also does not suffer from #56, as you can imagine

RonaldEnsing · 2019-05-21T19:46:28Z

A NavSatFix message plus orientation convention, such as ENU, fully describes everything you need.

Does this not have the implicit assumption that the NavSatFix's frame_id is aligned with ENU/NED/NWU? Then how would you deal with a NavSatFix sensor rigidly attached to your robot? I have a working system with tud-cor@a8d2e20. I cannot get the version of this PR to work in my setup.

m-naumann · 2019-05-21T20:12:58Z

A NavSatFix message plus orientation convention, such as ENU, fully describes everything you need.

Does this not have the implicit assumption that the NavSatFix's frame_id is aligned with ENU/NED/NWU? Then how would you deal with a NavSatFix sensor rigidly attached to your robot? I have a working system with tud-cor@a8d2e20. I cannot get the version of this PR to work in my setup.

It does have the explicit assumption that the NavSatFix's frame_id is aligned with ENU/NED/NWU. What you simply have to do is provide a frame for which this assumption is correct, which is rotated w.r.t. the robot frame.

In your setup, I assume you have a GPS any maybe some additional sensor to help determining the orientation(, or you determine it simply by GPS differences, which would be quite jumpy). So I'd suggest to use one frame for the GPS sensor with an ENU frame, such as gps_enu, and then you have a frame that determines the actual position of the robot, with orientation, say robot. For the robot, you are interested in a continuous pose estimate, I suppose, so it's position should be relative to some odom frame, in which you also detect objects and so on. For the GPS, you are interested in a global position, so gps_enu is defined in map, as translation only.
If you don't need the distinction between a continuous odom and a jumpy map frame, you can simply connect gps_enu and robot via a rotation only, using your current orientation estimate.

schra · 2019-05-22T16:28:00Z

I have to agree with Ronald here. We should avoid making assumptions about the use case or orientations of frames. The plugin should work out of the box for most use cases.

Yes it is simpler, but also has less functionality. If you have a robot moving around in the world, it does not necessarily carry a GPS receiver, still you might want to visualize the environment in which it is moving. So you provide one GPS location in the area, and with UTM we can always compute the correct tiles to be loaded.

I think that this is a pretty niche use case. If there is more demand for this feature though, I'm happy to add this functionality. Currently, I'd prefer if you do the transform from the NavSatFix to the robot frame outside of this plugin and then publish a fake NavSatFix with that frame. That should work with #53

m-naumann · 2019-05-22T17:02:53Z

I have to agree with Ronald here. We should avoid making assumptions about the use case or orientations of frames. The plugin should work out of the box for most use cases.

Exactly this is the motivation of this PR

Yes it is simpler, but also has less functionality. If you have a robot moving around in the world, it does not necessarily carry a GPS receiver, still you might want to visualize the environment in which it is moving. So you provide one GPS location in the area, and with UTM we can always compute the correct tiles to be loaded.

I think that this is a pretty niche use case. If there is more demand for this feature though, I'm happy to add this functionality. Currently, I'd prefer if you do the transform from the NavSatFix to the robot frame outside of this plugin and then publish a fake NavSatFix with that frame. That should work with #53

Where do you apply the ENU convention then? I think the latter is the one making assumptions about the use case or orientations of frames...

beetleskin · 2019-05-22T17:16:57Z

Where do you apply the ENU convention then? I think the latter is the one making assumptions about the use case or orientations of frames...

From https://www.ros.org/reps/rep-0105.html#map

When defining coordinate frames with respect to a global reference like the earth:

The default should be to align the x-axis east, y-axis north, and the z-axis up at the origin of the coordinate frame.

If there is no other reference the default position of the z-axis should be zero at the height of the WGS84 ellipsoid.

For your setup, you might want to look into the earth frame definition.

In order to be compliant to the earth frame definition, the user would need to select "map_1", "map_2", etc. However, no one ever had this feature request. Until now? ;)

m-naumann · 2019-05-22T17:51:14Z

Where do you apply the ENU convention then? I think the latter is the one making assumptions about the use case or orientations of frames...

From https://www.ros.org/reps/rep-0105.html#map

When defining coordinate frames with respect to a global reference like the earth:

The default should be to align the x-axis east, y-axis north, and the z-axis up at the origin of the coordinate frame.

If there is no other reference the default position of the z-axis should be zero at the height of the WGS84 ellipsoid.

For your setup, you might want to look into the earth frame definition.

In order to be compliant to the earth frame definition, the user would need to select "map_1", "map_2", etc. However, no one ever had this feature request. Until now? ;)

Okay, I agree, that might be valid for most use cases. It should be added to the description of the frame_convention_property_ though.

What remains is the jumping (#56), in case the tf of the nav_sat_fix_base (which currently is the robot frame) is updated with a higher frequency than the NavSatFix message itself. The latter, however, is the case for any robot using odometry to update it's own position. This PR solves the issue, as it is capable of dealing with a robot frame different from the nav_sat_fix_base, such that the robot can move relative to the latter in-between GPS updates.

I could update this PR to use map for the ENU convention, such that it "only" fixes the jumping when you select a robot_frame different from the nav_sat_fix_base but it doesn't care about the nav_sat_fix_base or the robot_frame (which could be set to be identical) orientation when loading tiles

RonaldEnsing · 2019-06-13T15:43:35Z

A bit of a late reply from my side. I think I just have a different preference compared to @m-naumann, since I would like to visualize the tiles based on where my sensor fusion algorithm thinks the car is. Because of that, I have a preference to use tf, including the use of a utm frame, since tf contains my estimation of the robot location in geographic coordinates, while the NavSatFix contains sensor information that has not been fused (yet).

Regarding the float precision in Ogre and the use of a UTM frame: If you're interested, I have made some modifications here https://github.com/tud-cor/rviz_satellite/tree/761df89985a1e11d6bdacef5b70574db8598bd1d, that allows transforming the tile in a frame closer to the car (using double precision) before drawing (single precision). I do not see differences. Be aware that this is only a visual comparison and only tested on one specific data set. (edit: the subsequent commit tud-cor@a3a7b5c fixes utm meridian convergence).

schra · 2020-04-18T00:11:15Z

As we went with #53 and soon will merge #72, I'm closing this.

RonaldEnsing and others added 2 commits May 17, 2019 16:23

Require utm tf to allow selectable rviz fixed frame.

a8d2e20

Implement loading tiles below the robot frame,

5135eea

use the NavSatFix as link between geo coordinates and Cartesian, update readme and demo accordingly, closes #35

This was referenced May 18, 2019

Use frame of NavSatFixMsg #35

Closed

Don't show map if transform is missing #51

Open

schra added the enhancement label May 20, 2019

schra reviewed May 20, 2019

View reviewed changes

schra closed this Apr 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix frame usage #55

Bugfix frame usage #55

m-naumann commented May 18, 2019 •

edited

Loading

RonaldEnsing commented May 20, 2019

beetleskin commented May 20, 2019 •

edited

Loading

RonaldEnsing commented May 20, 2019

m-naumann commented May 20, 2019

schra left a comment

schra May 20, 2019

m-naumann May 20, 2019

schra May 20, 2019

m-naumann May 20, 2019

schra May 20, 2019

m-naumann May 20, 2019

schra May 20, 2019

m-naumann May 20, 2019

schra May 20, 2019

m-naumann May 20, 2019

m-naumann commented May 20, 2019

RonaldEnsing commented May 21, 2019

m-naumann commented May 21, 2019

RonaldEnsing commented May 21, 2019 •

edited

Loading

m-naumann commented May 21, 2019

RonaldEnsing commented May 21, 2019

m-naumann commented May 21, 2019

schra commented May 22, 2019

m-naumann commented May 22, 2019

beetleskin commented May 22, 2019 •

edited

Loading

m-naumann commented May 22, 2019

RonaldEnsing commented Jun 13, 2019 •

edited

Loading

schra commented Apr 18, 2020

Bugfix frame usage #55

Bugfix frame usage #55

Conversation

m-naumann commented May 18, 2019 • edited Loading

RonaldEnsing commented May 20, 2019

beetleskin commented May 20, 2019 • edited Loading

RonaldEnsing commented May 20, 2019

m-naumann commented May 20, 2019

schra 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

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

m-naumann commented May 20, 2019

RonaldEnsing commented May 21, 2019

m-naumann commented May 21, 2019

RonaldEnsing commented May 21, 2019 • edited Loading

m-naumann commented May 21, 2019

RonaldEnsing commented May 21, 2019

m-naumann commented May 21, 2019

schra commented May 22, 2019

m-naumann commented May 22, 2019

beetleskin commented May 22, 2019 • edited Loading

m-naumann commented May 22, 2019

RonaldEnsing commented Jun 13, 2019 • edited Loading

schra commented Apr 18, 2020

m-naumann commented May 18, 2019 •

edited

Loading

beetleskin commented May 20, 2019 •

edited

Loading

RonaldEnsing commented May 21, 2019 •

edited

Loading

beetleskin commented May 22, 2019 •

edited

Loading

RonaldEnsing commented Jun 13, 2019 •

edited

Loading