Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix spherical coordinates FromLocal calls #428

Merged
merged 10 commits into from
Feb 11, 2022
Merged

Fix spherical coordinates FromLocal calls #428

merged 10 commits into from
Feb 11, 2022

Conversation

caguero
Copy link
Contributor

@caguero caguero commented Feb 7, 2022

See issue #420 and pull request #421 where there's a detailed conversation.

As described in this pull request, the *FromLocal*() functions contain a bug that has been fixed in Ignition Math. This patch instantiates a new ignition::math::SphericalCoordinates member variable in the base scoring plugin to be used for any derived scoring plugins.

Note that even in the updated version we still can't use *FromLocal*() directly, as it contains the version with the issue to preserve old behavior. In order to use the corrected version we need to call PositionTransform() directly.

Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
@caguero
Copy link
Contributor Author

caguero commented Feb 7, 2022

@M1chaelM , test-gazebo11-noetic fails because the Docker image npslearninglab/watery_robots:noetic_current probably keeps an old Ignition Math version. Do you think you could generate a new image with the latest Ignition Math version (6.10)?

@j-herman
Copy link
Collaborator

j-herman commented Feb 8, 2022

@caguero I checked out the code on my local installation and all is working correctly in both the stationkeeping and wildlife transforms for noetic - just waiting on the docker fix. I am having trouble getting anything to run in the melodic docker image so will have to retest later.
I think we could use PositionTransform directly in the earlier versions as well, with 'GLOBAL' instead of 'LOCAL2', to keep the syntax consistent, but this should work so whatever you prefer works for me.

@caguero
Copy link
Contributor Author

caguero commented Feb 8, 2022

@caguero I checked out the code on my local installation and all is working correctly in both the stationkeeping and wildlife transforms for noetic - just waiting on the docker fix. I am having trouble getting anything to run in the melodic docker image so will have to retest later. I think we could use PositionTransform directly in the earlier versions as well, with 'GLOBAL' instead of 'LOCAL2', to keep the syntax consistent, but this should work so whatever you prefer works for me.

I just updated the code. Is that what you're thinking?

@j-herman
Copy link
Collaborator

j-herman commented Feb 9, 2022

Yes, that's what I was thinking, or we can even simplify further (stationkeeping example):

   ignition::math::Vector3d cartVec(this->goalX, this->goalY, xyz.Z());
   ignition::math::Vector3d scVec;

#if GAZEBO_MAJOR_VERSION >= 11
    auto in = ignition::math::SphericalCoordinates::CoordinateType::LOCAL2;
    auto out = ignition::math::SphericalCoordinates::CoordinateType::SPHERICAL;
#else
    auto in = gazebo::common::SphericalCoordinates::CoordinateType::GLOBAL;
    auto out = gazebo::common::SphericalCoordinates::CoordinateType::SPHERICAL;
#endif

  scVec = this->sc.PositionTransform(cartVec, in, out);
  scVec.X(IGN_RTOD(scVec.X()));
  scVec.Y(IGN_RTOD(scVec.Y()));

Maybe it would be easiest to use GLOBAL for all versions for now, until Michael has time to update the docker image?

Signed-off-by: Carlos Agüero <[email protected]>
@caguero
Copy link
Contributor Author

caguero commented Feb 9, 2022

0060993

I like your last suggestion. How about this? 0060993

Copy link
Collaborator

@j-herman j-herman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked both wildlife and stationkeeping tasks - spherical coordinates are correct in both. Good to merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants