search

tu-darmstadt-ros-pkg / hector_gazebo

hector_gazebo provides packages related to the simulation of robots using gazebo (gazebo plugins, world files etc.).
http://www.ros.org/wiki/hector_gazebo
181 stars 155 forks source link

Use world's spherical_coordinates for GPS/magnetic reference #47

Closed Roboterbastler closed 3 years ago

Roboterbastler commented 6 years ago

This pull request tries to address #15 by @meyerj.

It modifies GPS and magnetic plugins to use the values from the spherical_coordinates element of the world SDF as default values for the latitude, longitude, altitude and heading references. The optional parameters can still overwrite this values. However it is important to notice, that the heading in the world SDF is specified counter-clockwise from east whereas the reference_heading parameter is specified is specified clockwise from north.

Roboterbastler commented 6 years ago

Let me know if you need more information or request any changes. I think this is a cool enhancement because the reference location for GNSS sensor simulation should be part of the world definition not of the sensor definition (that way you can spawn the same robot in different worlds with different simulated locations).

ingramator commented 3 years ago

Execellent PR, will be testing this shortly

Roboterbastler commented 3 years ago

@ingramator Have you found time to test it?

Martin-Oehler commented 3 years ago

Hello, thank you for your initiative to improve this plugin. The change seems reasonable and we would like to merge it. However, the commit looks like it will break the old behavior. Could you update your PR so by default, the old behavior is used (i.e. if no sperical coordinates are included in the SDF)?

ndwallace commented 3 years ago

@Roboterbastler I'll step in here for @ingramator and state that we have recently tested this PR, albeit on ROS Melodic and Gazebo 9.

It is worth noting that there is a slight difference in the Gazebo API for the physics::World object between Gazebo 7 and 9; the world->GetSphericalCoordinates() function has now been replaced with world->SphericalCoordinates(). This should be the only change necessary to update this PR for the melodic branch too.

After applying that change, the PR worked without issue. The ability to read the reference location from the world definition was a very welcome addition!

ingramator commented 3 years ago

It works very well we have been using it for a couple of months now and no noticeable issues! Makes managing multiple worlds much easier. Thanks for the PR! Happy to help if there are any more modifications you want to make.

StefanFabian commented 3 years ago

To elaborate on what Martin said, we do not want to introduce ABI changes or unexpected behavioral changes between versions. This PR doesn't modify ABI so that part is alright. However, it is unclear to us if existing world files that use this plugin will behave exactly the same as they did before. We usually solve behavioral changes with an optional parameter to switch to the new behavior if that is necessary here.

Roboterbastler commented 3 years ago

I took note of your remarks and feedback, thanks! I will see when I find time to work on this...

StefanFabian commented 3 years ago

Thank you again for the PR, I have merged #83 which extended this PR and addressed our concerns so this can be closed.