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

Changed include paths in utils to reflect the correct paths and replaced SimpleResourceLocator with TesseractSupportResourceLocator. #30

Conversation

alliethebanana
Copy link

Had opened an issue since I couldn't figure out the solution for this, but by going through the backlog of releases of tesseract I managed to figure that SimpleResourceLocator has been since replaced by TesseractSupportResourceLocator.

This merge request does not cover fixes to the tests folder.

Copy link
Contributor

@Briancbn Briancbn left a comment

Choose a reason for hiding this comment

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

Not sure why the rosdep is failing in the pipeline tho.

@@ -1,5 +1,7 @@
cmake_minimum_required(VERSION 3.5.0)

set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not needed I believe.

Copy link
Author

Choose a reason for hiding this comment

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

Ah this was something I left over by accident while debugging, I'll remove it :)


ROSResourceLocator::ROSResourceLocator() : SimpleResourceLocator(::tesseract_rosutils::locateResource) {}
ROSResourceLocator::ROSResourceLocator() : tesseract_common::TesseractSupportResourceLocator() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work out of the box with ROS2?

Copy link
Author

Choose a reason for hiding this comment

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

It seeming more like it doesn't on my side. The package builds, but whenever I try to use it to initialize an environment it crashes my node.

Do you have any idea what the SimpleResourceLocator got replaced with in the Tesseract repo?

Copy link
Contributor

Choose a reason for hiding this comment

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

This was removed to support serialization of the environment. There is now two available one used by testing and examples found here and there is one in tesseract_ros which could be used as an example if one does not already exist in this repository.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

I was trying to implement something like this yesterday, but I was a bit confused on how to do it. This cleared it up quite a bit, thanks for the help :)).

I have implemented the changes and it seems to build without a problem.

Copy link
Author

Choose a reason for hiding this comment

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

Though, it fails to parse the URDF on the examples, not sure if this relates to the patch
image

Failure is also seen while parsing the URDF file on my own planning task
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, this error occurred because the path sent to parseURDFFile still started with package:// which should have been replace by your resource loader. I would use a debugger and see if you resource locator is working as expected.

Copy link
Author

Choose a reason for hiding this comment

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

I managed to load the environment correctly. The merged code for the locator is working as expected, there were just some problems also on my URDF side :).

@@ -20,6 +20,8 @@
<depend>tesseract_motion_planners</depend>
<depend>tesseract_process_managers</depend>
<depend>tesseract_srdf</depend>
<depend>tesseract_command_language</depend>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is common_language used there? Since not the serialization dependencies are from tesseract_common

Copy link
Author

Choose a reason for hiding this comment

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

Hmmm, not sure. Can't remember if I needed this to be able to build the package, I'll try removing it and get back at you.

@alliethebanana
Copy link
Author

I have also applied fixes to the examples provided, since it seems the methods in the response msg have been changed.

From response.results->as to response.problem->results->as.

Let me know if this is the intended usage.

@Levi-Armstrong
Copy link
Contributor

@GuiAivero if you rebase on this PR will the CI pass? If so,I will merge both.

@alliethebanana
Copy link
Author

@GuiAivero if you rebase on this PR will the CI pass? If so,I will merge both.

Done. Fingers crossed that it passes

@Briancbn
Copy link
Contributor

Briancbn commented Jul 6, 2022

It seems that the .repos files is not updated. CI is still using the 0.7.0 version of tesseract. Changing the dependencies.repos repo versions to match that of ros1 dependencies.rosinstall should fix the foxy CI build. For the format check, add in the suggested changes should pass.

@alliethebanana
Copy link
Author

It seems that the .repos files is not updated. CI is still using the 0.7.0 version of tesseract. Changing the dependencies.repos repo versions to match that of ros1 dependencies.rosinstall should fix the foxy CI build. For the format check, add in the suggested changes should pass.

Done! Also added tesseract-qt as a dependency since it was part of the ROS1 dependencies, not sure if it is meant to be included.

@Briancbn
Copy link
Contributor

Briancbn commented Jul 6, 2022

Please also remember to fix the lint errors reported in the CI as such. Briancbn@1136183. I can confirm the new CI will pass after the changes https://github.com/Briancbn/tesseract_ros2/runs/7211700850?check_suite_focus=true, the newly added qt package is also causing rosdep dependencies issue. I think should remove for now.

@marrts marrts mentioned this pull request Jul 27, 2022
@rjoomen
Copy link
Contributor

rjoomen commented Feb 28, 2023

I suppose this PR can be closed now, since #35 has been merged.

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.

5 participants