-
Notifications
You must be signed in to change notification settings - Fork 211
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
Restore icons throughout RViz2 #235
Conversation
- This is necessary to be able to find icons (name=path)
- Otherwise using the default config can't load classes anymore
- this does not functionally reenable different cursors
By catching the exception rviz will start with a visibly broken config so that the user can fix the config manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is one good change to make first, but otherwise lgtm.
The breaking change for the names of the displays is frustrating, but necessary without changing how the system works. We could add some code to help the migration, which simply maps all calls to load rviz/*
to the new equivalent, warnings users as they go. New saves could save in the new format, so at best old configs will work with a warning and newly saving that config will fix the problem. I won't require that for this pr, but it would be nice if it were. At the very least, we should open an issue clearly describing this breaking change what could be done to alleviate it.
RVIZ_COMMON_LOG_ERROR("Invalid or unsupported URL: " + url.toStdString()); | ||
} | ||
return file_path; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you're doing this manually with ament_index_cpp
and not with resource_retriever
?
For example, this code uses resource_retriever
to do basically the same thing:
rviz/rviz_rendering/src/rviz_rendering/mesh_loader.cpp
Lines 84 to 96 in 2223027
resource_retriever::MemoryResource getResource(const std::string & resource_path) | |
{ | |
resource_retriever::Retriever retriever; | |
resource_retriever::MemoryResource res; | |
try { | |
res = retriever.get(resource_path); | |
} catch (resource_retriever::Exception & e) { | |
RVIZ_RENDERING_LOG_ERROR(e.what()); | |
return resource_retriever::MemoryResource(); | |
} | |
return res; | |
} |
That's in service of loading mesh files so they can be interpreted by assimp or the STL parser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason was that I didn't see that QPixmap
offered a possibility to load data in the form of a uchar
array, which is what we're given by resource_retriever
. In addition, I thought that it would then be necessary to distinguish between file formats. Turns out, neither is the case, so I switched to using the resource_retriever.
It would be nice though if the resource_retriever
had a method to just give us the full path to the resource.
Regarding the breaking change in the config, I added a quick fix which converts all names that can occur when building the current ros2 master from source, warning the user (on the console) to save their config. If you had something more sophisticated in mind (e.g. when using configs from the old RViz), we'd have to work with a map and I'd rather open an issue and close it with a later PR as you suggest. CI: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm me now, thanks for fixing it up
I made an issue to remind me to remove that "correction" code at some point: #239
Fixes #56.
The package and path for a icon are derived from the class id of the plugin. This requires some renaming of classIds in the
plugin_description.xml
.As a consequence there is a breaking change: an already existing
~/.rviz2/default.rviz
will not load correctly. We fixed the crash so a user is faced with an empty rviz window and needs to add all panels and displays manually and save the config afterwards.