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.
|
CI:
|
wjwwood
added
enhancement
There was a problem hiding this comment.
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.
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
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.
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:
|
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.rvizwill 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.