-
Notifications
You must be signed in to change notification settings - Fork 194
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
doTransform non stamped msgs #452
doTransform non stamped msgs #452
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR is much larger than it has to be because you moved functions around. What was the rationale behind that? Reverting those changes would make reviewing more straightforward too.
tf2_geometry_msgs/include/tf2_geometry_msgs/tf2_geometry_msgs.hpp
Outdated
Show resolved
Hide resolved
I've applied your suggestion, hope it's fine now 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doTransform
methods introduced for the non-stamped message types look mostly fine to me. Two concerns though:
- There's still a lot of refactoring that adds noise to this PR. You should defer structural refactoring for a different PR. If you're fine with it, I can force push a commit that removes any unnecessary moving-around of the older methods. Please check 2050824 to see what I mean. That's the commit with least noise.
- I think unit tests for some of the methods are missing.
Okay, you are right. Do apply your path and then I will open next PR with refactor I've done before and finally we do new unit tests. I was wondering whether to add new tests here but most of existing are time related tests also the issues was not about this. To sum up if you agree with me please close the PR and do think about better unit tests. Thank you for your patience and help :) |
Co-authored-by: adamkrawczyk <adam-krawczyk@outlook.com> Co-authored-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu> Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>
Co-authored-by: adamkrawczyk <adam-krawczyk@outlook.com> Co-authored-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu> Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>
3b070dd
to
48a18fb
Compare
Alright, I've updated the header file in question to retain your changes but introduce less noise (74c7d82).
By this, I meant that you did not introduce unit tests for the
I don't think this PR needs to be closed; but yes, unit tests (not those that target the changes introduced in this PR) is part of a conversation outside of this PR's scope. By the way, having tested locally, |
Okay, I found the issue behind this build fails. Let me explain it: I did manual calculations
Which solves the issue. I will prepare a path for this - simple change val and also will add the same tests for other msg types (stamped) which fails too. I'm 99% sure about this but as always check me. Best |
I don't think this is necessary to add because the |
I've changed return val to 11. Also to prove that implementation is right I have applied orginal tests from geometry_msgs on other branch more-test-branch. Try them locally, all tests pass. |
I think the changes in d67d92f are appropriate, but I am still perplexed by how the @tfoote would you be able to shed some light about why - for the tests in FYI: the transform specified in the tests. Note that geometry2/tf2_geometry_msgs/test/test_tf2_geometry_msgs.cpp Lines 292 to 302 in 8b12844
|
Any updates, @tfoote ? |
Yes, that's a common confusion. If you are asking for the transform from frame A to frame B it's the inverse of the transform which will take data in frame A and express it in frame B. That's why we use target and source frame nomenclature for where the data is coming from and going to. It starts in the source frame and ends in the target frame. And no one externally needs to know about the structure or order of the transforms except the implementors. |
Thank you for your response. Can it be merged now? @aprotyas |
@tfoote Can you merge and close this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks fine, just two small nits to take care of. Once those are done I'll run CI on the whole thing.
tf2_geometry_msgs/include/tf2_geometry_msgs/tf2_geometry_msgs.hpp
Outdated
Show resolved
Hide resolved
tf2_geometry_msgs/include/tf2_geometry_msgs/tf2_geometry_msgs.hpp
Outdated
Show resolved
Hide resolved
All right, CI is all green here. Thanks for the contribution, merging. |
Response to:
#337
I didn't implement
toMsg
andfromMsg
forPoseWithCovariance
because there is no tf2 equivalent to geometry_msgs PoseWithCovariance