swri-robotics / marti_common

Common utility functions for MARTI
BSD 3-Clause "New" or "Revised" License
54 stars 63 forks source link

swri_transform_util: transform manager in ROS2 should allow a non-null local_xy_util pointer to be passed into the UTM and WGS84 transformers #619

Closed vknisley-swri closed 3 years ago

vknisley-swri commented 3 years ago

In earlier versions of swri_transform_util, the constructors of the Wgs84Transformer and UtmTransformer classes did not take any arguments. As of version 3.3.2 (on the dashing-devel branch), each of these classes take a LocalXyWgs84UtilPtr in their constructor, which is then assigned to local_xy_util.

If local_xy_util is null, then these transformers will fail to initialize, making it impossible to get transforms from them. However, the transform manager specifically uses null pointers when creating the transformer objects. This means that the transform manager is not able to transform to/from the UTM or WGS84 frames in the same way as in previous versions of this package.

I will fork this repository and work on passing a pointer to an actual LocalXyWgsUtil object into the transformers' constructors.

matt-attack commented 3 years ago

This is surprising to me that this is the case, as one would think having this working would be a prerequisite for mapviz in ROS2 which was done quite a while ago. Looking at the code it seems these get initialized later when you call Initialize on the transform manager here: https://github.com/swri-robotics/marti_common/blob/dashing-devel/swri_transform_util/src/transform_manager.cpp#L75

It seems like this would function correctly. Perhaps there is some kind of initialization order problem if it still doesn't work after calling this?

After more poking the order of operations here might be problematic: https://github.com/swri-robotics/marti_common/blob/dashing-devel/swri_transform_util/src/transformer.cpp#L45

vknisley-swri commented 3 years ago

I looked at what you had pointed out, and you're right about local_xyutil being initialized correctly later on. Looking at the order in the Initialize(tf_buffer_, local_xy_util_) function, it doesn't seem to really be problematic (at least in this case), because all it does is set the value of initialized_ : https://github.com/swri-robotics/marti_common/blob/56b32be63be3979244d406ec0cb8668c548da223/swri_transform_util/src/transformer.cpp#L45 and later on, if initialized_ is found to be false (when trying to get a transform), then Initialize() is simply run again, this time with a local_xyutil that isn't null. This is the case for both the WGS84 and UTM transformers, but the example of this in the UtmTransformer (lines 65-72) is here:

   if (!initialized_)
            {
              Initialize()
              if (!initialized_)
              {
                return false;
              }
            }

I had thought that the null pointers were the problem because of the original error messages I was getting. By adding more debug statements, it looks like the real problem was that local_xyutil itself was not getting initialized. I can go ahead and close this issue, if you'd like.