Skip to content

Unified collision environment#54

Merged
BryceStevenWilley merged 1 commit intomoveit:masterfrom
j-petit:unified_collision_env
Aug 21, 2019
Merged

Unified collision environment#54
BryceStevenWilley merged 1 commit intomoveit:masterfrom
j-petit:unified_collision_env

Conversation

@j-petit
Copy link
Copy Markdown
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
Copy Markdown
Member

Hmmm... this passes??

@BryceStevenWilley
Copy link
Copy Markdown

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
Copy Markdown
Contributor

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

@BryceStevenWilley
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
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.

4 participants