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

Work in progress using the new resource retriever apis #1262

Open
wants to merge 8 commits into
base: rolling
Choose a base branch
from

Conversation

mjcarroll
Copy link
Member

No description provided.

@wjwwood
Copy link
Member

wjwwood commented Dec 17, 2024

Pushed an update that does some clean up and renames the message/service package into rviz_resource_interfaces, as well as adds an etag field and uses it to invalidate the cache. Working on finishing the cleanup, deduplicating with robot_link.cpp and updating the matching example service that works with this.

@asherikov
Copy link

In the context of related discussion in ros/robot_state_publisher#144, I think it would make sense to define the service in a package that is not related to rviz, e.g., meshes can be queried by a node that performs collision checking.

@mjcarroll
Copy link
Member Author

While the service is defined in this repository, it doesn't have any dependency on rviz, so it shouldn't incur any extra cost to use it in other packages.

@wjwwood
Copy link
Member

wjwwood commented Feb 19, 2025

@asherikov, as @mjcarroll said you should be able to use this new package without any issue even if it's in the rviz repository and the name implies a connection to rviz. I spoke to @mjcarroll about it, and we agreed that we'd like to see it used in rviz as-is in this pull request before recommending it be used outside of this context and/or moving it to one of the standard message repositories (e.g. visualization_msgs or something like that) or to it's own repository under a more general name.

@wjwwood wjwwood marked this pull request as ready for review February 19, 2025 00:56
@wjwwood
Copy link
Member

wjwwood commented Feb 19, 2025

Ok, I think this is ready for review. It depends on ros/resource_retriever#103 which is under review now.

@wjwwood wjwwood requested a review from ahcorde February 19, 2025 00:56
…riever.cpp

Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@ahcorde
Copy link
Contributor

ahcorde commented Feb 19, 2025

Pulls: ros/resource_retriever#103, #1262
Gist: https://gist.githubusercontent.com/ahcorde/13502377fe7128ba0a2d910a7c9dfd91/raw/2b32e8eeb4f5c28e7b0afbbdb6ea9f15771e6234/ros2.repos
BUILD args: --packages-above-and-dependencies resource_retriever
TEST args: --packages-above resource_retriever
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15221

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

return nullptr;
}
Ogre::MeshSerializer ser;
Ogre::DataStreamPtr stream(new Ogre::MemoryDataStream(*res->data.data(), res->data.size()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Ogre::DataStreamPtr stream(new Ogre::MemoryDataStream(*res->data.data(), res->data.size()));
Ogre::DataStreamPtr stream(new Ogre::MemoryDataStream(
const_cast<void*>(reinterpret_cast<void const *>(res->data.data())), res->data.size()));

@@ -72,7 +73,7 @@ TEST_F(MeshLoaderTestFixture, throws_reasonable_exception_for_missing_files) {
std::string mesh_path = "package://rviz_rendering/ogre_media/MISSING.mesh";
testing::internal::CaptureStderr();

auto mesh = rviz_rendering::loadMeshFromResource(mesh_path);
auto mesh = rviz_rendering::loadMeshFromResource(&this->retriever_, mesh_path);

std::string output = testing::internal::GetCapturedStderr();
ASSERT_THAT(output, HasSubstr("Error retrieving file"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ASSERT_THAT(output, HasSubstr("Error retrieving file"));
ASSERT_THAT(mesh, nullptr);

@@ -226,6 +228,13 @@ RobotLink::RobotLink(
color_material_ =
rviz_rendering::MaterialManager::createMaterialWithLighting(color_material_name);

resource_retriever::RetrieverVec plugins;
plugins.push_back(std::make_shared<RosResourceRetriever>(context_->getRosNodeAbstraction()));
Copy link
Contributor

Choose a reason for hiding this comment

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

other issue is happening here:

C++ exception with description "ROS node abstraction interface not valid" thrown in the test body

@mjcarroll mjcarroll self-assigned this Feb 27, 2025
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