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

refactor to not use tf version 1 #561

Merged

Conversation

wjwwood
Copy link
Contributor

@wjwwood wjwwood commented Mar 3, 2017

I was going to convert this to tf2, but it turns out that it really only needs to convert some Euler angles to Quaternion, and @tfoote's recommendation was to just use bullet directly:

ros2#1 (comment)

So that's what's happening here instead.

@mikeferguson
Copy link
Contributor

Seems reasonable -- I presume you've actually loaded a map and tested this?

@mikeferguson mikeferguson self-assigned this Apr 16, 2017
@wjwwood
Copy link
Contributor Author

wjwwood commented Apr 17, 2017

I have used this patch in the ROS 2 demo work we've been doing. I haven't tried it in ROS 1 yet.

I just resolved the conflicts too.

@vrabaud
Copy link
Contributor

vrabaud commented Jul 17, 2017

I probably need to rebase by now but there is a full tf2 port of navigation since hydro: #531

@wjwwood
Copy link
Contributor Author

wjwwood commented Jul 17, 2017

@vrabaud that would be great to get in, but I avoided using tf2's internal transform in favor of bullets in this pr at @tfoote's recommendation. Your pr does the former I think:

https://github.com/ros-planning/navigation/pull/531/files#diff-131b6b4d9e11b01840556ffeeb9acfe0

So if we close this in favor of #531, then I think we should update it to fix image_loader and map_saver like this pr does.

@vrabaud
Copy link
Contributor

vrabaud commented Jul 17, 2017

Your PR is smaller so go with it, I'll adapt. Then again, there is still some tf1 in there you need to also fix in your current PR:
https://github.com/ros-planning/navigation/blob/kinetic-devel/map_server/src/map_saver.cpp#L35

@wjwwood
Copy link
Contributor Author

wjwwood commented Jul 17, 2017

@vrabaud that's true. I guess I missed it from my port in our ROS 2 demo, but I just used the tf2 version of that header:

https://github.com/ros2/navigation/blob/ros2/map_server/src/map_saver.cpp#L38

@vrabaud
Copy link
Contributor

vrabaud commented Jul 17, 2017

Still not working. From my experience when switching to tf2: just do a grep on "tf/" or "tf::". You still have a "tf::".
Please refer to: https://github.com/ros-planning/navigation/pull/531/files#diff-11fba1ba2b27983920dd7eab46a366dd

@wjwwood
Copy link
Contributor Author

wjwwood commented Jul 17, 2017

@vrabaud I was being lazy, and then remembered that pr testing is currently down. I'll fix it up asap.

Also we could roll this into your pr and close this one. Wouldn't matter to me.

@vrabaud
Copy link
Contributor

vrabaud commented Jul 17, 2017

Naah, go with this one. The smaller the better.

@vrabaud
Copy link
Contributor

vrabaud commented Jul 17, 2017

@vrabaud, that's not what she said !

@wjwwood
Copy link
Contributor Author

wjwwood commented Jul 17, 2017

Haha, ok I'll fix this one up asap.

@nuclearsandwich
Copy link

@ros-pull-request-builder retest this please.

@vrabaud
Copy link
Contributor

vrabaud commented Jul 17, 2017

LGTM, feel free to comment on #531

@wjwwood
Copy link
Contributor Author

wjwwood commented Jul 17, 2017

The only test failures was in the costmap tests, I'm thinking it might just be flakey. I'm going to rerun the tests to see.

@ros-pull-request-builder retest this please

@wjwwood
Copy link
Contributor Author

wjwwood commented Jul 18, 2017

Ok, CI and stuff looks good. I still haven't tried this locally with ROS 1 as a sanity check.

@vrabaud
Copy link
Contributor

vrabaud commented Jul 31, 2017

@wjwwood , I have rebased #458 and #531 and used your changes. But let's merge this PR first.

@wjwwood
Copy link
Contributor Author

wjwwood commented Jul 31, 2017

@vrabaud that's fine by me, but I'm not the maintainer. I'll let one of them decide on when/if to merge this.

@mikeferguson
Copy link
Contributor

Ok -- tested on live ROS1 system. Going to squash-merge for ease of pulling into lunar

@mikeferguson mikeferguson merged commit c118e8e into ros-planning:kinetic-devel Aug 1, 2017
mikeferguson pushed a commit that referenced this pull request Aug 1, 2017
gerkey pushed a commit to codebot/navigation that referenced this pull request Jan 19, 2018
johaq pushed a commit to CentralLabFacilities/navigation that referenced this pull request Mar 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants