Migrate map display #267
Migrate map display #267
Conversation
|
CI:
|
Martin-Idel-SI
force-pushed
the
feature/migrate_map_display
branch
from
241be68
to
692ed4d
Compare
|
CI after rebase:
|
|
I added a pr to add |
Martin-Idel-SI
force-pushed
the
feature/migrate_map_display
branch
2 times, most recently
from
09890ab
to
bfa76f5
Compare
|
Then let's see, whether this works (we tested locally that it actually works, this is just CI):
Sadly, there is still an error on the |
Martin-Idel-SI
force-pushed
the
feature/migrate_map_display
branch
from
bfa76f5
to
ae01f11
Compare
|
@wjwwood to look into the issue with the service definitions in |
|
From a quick look it seems that the parser requires the |
anhosi
force-pushed
the
feature/migrate_map_display
branch
from
ae01f11
to
49d89d9
Compare
|
@wjwwood I rebased this branch but will not start a new CI as it cannot succeed due to the |
|
@anhosi You can use https://github.com/mikaelarguedas/navigation_msgs branch |
|
New CI (restricted to rviz) including
|
wjwwood
added
in progress
|
I'm gonna place this in progress until CI is fixed up. |
|
@wjwwood reminder to self merge ros2/ros2#500 with this. |
|
Headsup: ros-planning/navigation_msgs#5 has been merged and the branch deleted so next round of CI should use the |
- stl_loader tests (see rviz_rendering) - Delete tests for pose, map, image superseded by visual tests
Martin-Idel-SI
force-pushed
the
feature/migrate_map_display
branch
from
bc153d5
to
91d49ce
Compare
|
Then let's give it a try with the new CI:
@wjwwood We added the last commit (91d49ce) only the make the tests pass. This commit resolves an undetected merge error from last night and will be published as a pr of its own. The last commit will cause a conflict once the other pr is merged. Simply remove the last commit from this pr before merging will work |
|
Next try with OSX:
|
wjwwood
added
in review
|
I resolved the conflict, running CI again on Linux as a tripwire for issues with the conflict resolution: @Martin-Idel-SI Please review my merge commit e6f038a to make sure I didn't screw anything up logically. |
| class Swatch | ||
| { | ||
| public: | ||
| RVIZ_DEFAULT_PLUGINS_PUBLIC Swatch( |
There was a problem hiding this comment.
nitpick: macro and class name should be on two separate lines
There was a problem hiding this comment.
I'll fix that together with #297 .
| return palette_builder->setColorForIllegalPositiveValues(0, 255, 0) | ||
| ->setRedYellowColorsForIllegalNegativeValues() | ||
| ->setColorForLegalNegativeValueMinusOne(0x70, 0x89, 0x86) | ||
| ->buildPalette(); |
There was a problem hiding this comment.
This indentation is strange to me. I would have expected it to either be aligned with the -> on the first line or only have one extra indention (two spaces) from the return.
I guess this is what uncrustify did?
There was a problem hiding this comment.
Yes, I would have preferred the alignment with ->, but it wouldn't let me do it.
|
@wjwwood Looks good. Map works as expected. Visual test runs, all other tests run, too. |
Closes #99
This PR migrates the Map display to ROS2. The old map display could subscribe to OccupancyGridUpdate messages, too. This functionality has also been ported, but it is commented out due to the fact that the message types are not available in ROS2, yet. Once they become available, it should be easy to enable updating a map again.
Furthermore, this PR
rviztest folder that are now superseded by automated visual tests (including all python tests for maps). More tests can probably be deleted, but we have to have a look whether all functionality has been ported. This will be done in follow-up pull requests.msg_conversions.hppfunctions and uses them when possible.rviz_rendering.