nobleo / rviz_satellite

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

Bugfix frame usage #55

Closed m-naumann closed 4 years ago

m-naumann commented 5 years ago

This is what it does:

See the demo launchfile for an example with 4 frames:

BTW, that is "Option 2" as explained here: https://github.com/gareth-cross/rviz_satellite/issues/35#issuecomment-315535588

RonaldEnsing commented 5 years ago

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

beetleskin commented 5 years ago

Thanks for the contribution! But I don't get why you pull in another frame (_nav_sat_fixbase) 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 commented 5 years ago

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 commented 5 years ago

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.

m-naumann commented 5 years ago

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 commented 5 years ago

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 https://github.com/tud-cor/rviz_satellite/commit/a8d2e207a53a13e46424c5c7c36a8c41e23df6a1 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 commented 5 years ago

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 commented 5 years ago

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 https://github.com/tud-cor/rviz_satellite/commit/a8d2e207a53a13e46424c5c7c36a8c41e23df6a1 is to allow any fixed frame to be selected. However, https://github.com/tud-cor/rviz_satellite/commit/a8d2e207a53a13e46424c5c7c36a8c41e23df6a1 does require the utm->robot tf's to be setup. The utm frame includes orientation, in my case ENU convention. With https://github.com/tud-cor/rviz_satellite/commit/a8d2e207a53a13e46424c5c7c36a8c41e23df6a1, I am able to select any fixed frame and get the tiles in the right orientation. In addition, https://github.com/tud-cor/rviz_satellite/commit/a8d2e207a53a13e46424c5c7c36a8c41e23df6a1 does not suffer from https://github.com/gareth-cross/rviz_satellite/issues/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 commented 5 years ago

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 commented 5 years ago

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 https://github.com/tud-cor/rviz_satellite/commit/a8d2e207a53a13e46424c5c7c36a8c41e23df6a1. I cannot get the version of this PR to work in my setup.

m-naumann commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 https://github.com/tud-cor/rviz_satellite/commit/a3a7b5cd1f6d101814603a41c11fbf5763d5301d fixes utm meridian convergence).

schra commented 4 years ago

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