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

Unified collision environment #54

Merged
merged 1 commit into from
Aug 21, 2019

Conversation

j-petit
Copy link
Contributor

@j-petit j-petit commented Aug 9, 2019

This PR adapts the visual tools for the unified collision environment as proposed here. It changes getCollisionWorld to the new getCollisionEnv function.

This does not build as the main unified PR has to be merged together with this.

@davetcoleman
Copy link
Member

Hmmm... this passes??

@BryceStevenWilley
Copy link

Seems like Travis tests the master branch of this repo everytime, not very useful. https://travis-ci.org/ros-planning/moveit_visual_tools/jobs/569762781#L220

@rhaschke
Copy link
Contributor

@BryceStevenWilley, can you elaborate on your previous comment, please? I don't get what is not very useful.

@BryceStevenWilley
Copy link

Sorry @rhaschke, I'm definitely not a Travis expert, but these changes depend on moveit/moveit#1584 being merged into master and should fail to build without them, yet Travis somehow passes. I saw the line in the Travis config that said

Testing branch 'master' of 'moveit_visual_tools' on ROS 'melodic'

and thought it was just pull the master branch, not this PR. I didn't look enough above to see that Travis is checking out the right branch (https://travis-ci.org/ros-planning/moveit_visual_tools/jobs/569762778#L178). So, I'm still not sure what's going on.

@rhaschke
Copy link
Contributor

I have the solution: Currently, both code changes are disabled.
So actually, this is an empty PR. I changed the title to WIP...

@rhaschke rhaschke changed the title Unified collision environment [WIP] Unified collision environment Aug 14, 2019
@davetcoleman
Copy link
Member

They are disabled at no fault of @j-petit 's, but probably of some lazy practices of mine when this was my graduate project. I think we can safely merge this and we can worry about collision checking of interactive marker collision checkers in the future.

Copy link

@BryceStevenWilley BryceStevenWilley left a comment

Choose a reason for hiding this comment

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

Ah, I see. Thanks for clearing that up Robert and Dave. I agree with Dave. Jens probably found these through a find-replace, not because it wouldn't compile after the unified change.

@j-petit
Copy link
Contributor Author

j-petit commented Aug 19, 2019

Ah, I see. Thanks for clearing that up Robert and Dave. I agree with Dave. Jens probably found these through a find-replace, not because it wouldn't compile after the unified change.

Yes, I did the changes through find-replace and didn't notice that this is disabled through the preprocessor. But still this should be replaced if #1584 gets merged right?

@davetcoleman
Copy link
Member

Right!

@BryceStevenWilley BryceStevenWilley merged commit af72ea8 into moveit:master Aug 21, 2019
@davetcoleman davetcoleman changed the title [WIP] Unified collision environment Unified collision environment Aug 21, 2019
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.

None yet

4 participants