usage for rviz_common::VisualizationManager function

[JaehyunShim]

Hi,

There has been a change in the input arguments for the rviz_common::VisualizationManager function (rviz::VisualizationManager in ROS1) and now the following three have to be passed over to the function when called.

ros_integration::RosNodeAbstractionIface::WeakPtr
rclcpp::Clock::SharedPtr
WindowManagerInterface* 

so I did as follows.

(... for rviz_render_panel_ )
auto rviz_ros_node = std::make_shared<rviz_common::ros_integration::RosNodeAbstraction>("rviz");
rviz_common::WindowManagerInterface * wm;
auto clock = rviz_ros_node->get_raw_node()->get_clock();
rviz_manager_ = new rviz_common::VisualizationManager(rviz_render_panel_, rviz_ros_node, wm, clock);

but it fails when

rviz_render_panel_->initialize(rviz_manager_);
rviz_manager_->initialize();

The relevant tutorial for that function hasn't been written for ROS2 yet. Any advice about what to pass over to the function when using it?

Thanks in advance,
Jaehyun

[mjeronimo]

@JaehyunShim I took a look for similar code in rviz and found this: https://github.com/ros2/rviz/blob/ros2/rviz_common/src/rviz_common/visualization_frame.cpp#L325-L350

I suggest that you review that code for differences. Otherwise, you can submit a test case that can be used to reproduce the issue.

[JaehyunShim]

@mjeronimo Thank you for your comment.

I copypasted the code in this guy's comment and tested it but it didn't work. The program dies when running the function (setEnable) I mentioned in this comment.

If visualization_manager is going to be added to Rviz2 for the next distro anyway (as you can see it here) and there are already use cases of the feature (to be migrated to ROS2) out there, I think verifying that it works without issues this time is not a bad idea.

Thank you for your help in advance!
Jaehyun

[JaehyunShim]

@mjeronimo ping

[mjeronimo]

@JaehyunShim I've used the code as a starting point and have reproduced the failure (I'm getting a double free error). I need some time to debug to see if I can get to the root cause. Output should be a running example.

[JaehyunShim]

@mjeronimo Thanks! I will wait for your next comment then

[JaehyunShim]

@mjeronimo Kind reminder~.

Also, I would like to ask if your team can reconsider blooming in foxy the latest version of this repository which is with the visualization_frame.hpp and visualization_manager.hpp files. It takes quite a lot of time to compile the entire rviz every build. I believe the Moveit working group will also need it for their setup assistant. If you release it in the next distro, we have to wait for not only rviz, but also other dependencies, and considering G distro is not an LTS version, I am worried many distributors might even skip the non-LTS version.

[mjeronimo]

@JaehyunShim Here's the code I've been working on: https://github.com/mjeronimo/rviz_embed_test. I haven't been able to get it working yet and could use some help debugging what's going on.

Regarding releasing the latest code for Foxy: we should be able to do that. @jacobperron any issues with this?

[MartiBolet]

Hey @JaehyunShim @mjeronimo ,
I managed to get a working implementation of a custom Qt5 Application with a RenderPanel. I had to create a function in the source code of rviz2 and maybe it's not the best option but it works. I have the solution in this repo. Any feedback is apreciated.

[JaehyunShim]

@MartiBolet Awesome! I will test it and let you know if it works now. It will be even more helpful if you send a pull request to https://github.com/ros-visualization/visualization_tutorials/tree/ros2/librviz_tutorial from your repo, then other users in the community can also see and use your code.

[JaehyunShim]

@MartiBolet

OK, so I read your code and it seems what you did is

1. instantiate a frame (which is a QMainWindow actually)
2. get the render panel from that frame
3. add that render panel to your QMainWindow

but my question here is about how to make a frame after initializing and using a manager & renderpanel.

[MartiBolet]

Hey @JaehyunShim ,
Yes that's what it does. Since the error is the instantiation of the VisualizationManager, when there is a window event and tries to resize it, from this comment, I tried to instantiate the VisualizationManager in the same way the rviz2 app does it. However, I couldn't get it work, so the way I found was retrieving the RenderPanel from the VisualizationFrame as a last resort, so yes, I didn't solve that problem i just found a workaround to make a functional version.

[JaehyunShim]

@MartiBolet Yeah, I will wait for help from @mjeronimo a bit more. Thanks very much though!

[mjeronimo]

@MartiBolet Thanks for letting us know about your work! I'll have a look at the repo you've provided.

[jacobperron]

Regarding releasing the latest code for Foxy: we should be able to do that. @jacobperron any issues with this?

I'm assuming we're talking about code on the ros2 branch. We should make sure we maintain API (and preferably ABI) compatibility for any changes being backported to the foxy branch.

[JaehyunShim]

@jacobperron We are not talking about backporting but git pulling from the ros2 branch to the foxy branch and releasing it so users can use the newly added features in the foxy distro as well. Of course, API compatibility should be considered in this case as well though.

[mjeronimo]

@MartiBolet @JaehyunShim I did some more digging and found that the code was crashing in Ogre here:

     void X11EGLWindow::windowMovedOrResized()
    {
        if (mClosed || !mWindow)
        {
            return;
        }

        NativeDisplayType mNativeDisplay = mGLSupport->getNativeDisplay();
        XWindowAttributes windowAttrib;

        if (mIsTopLevel && !mIsFullScreen)
        {
            Window parent, root, *children;
            uint nChildren;

            XQueryTree((Display*)mNativeDisplay, (Window)mWindow, &root, &parent, &children, &nChildren);

            if (children)
            {
                XFree(children);
            }

It was crashing because the XQueryTree call failed with a BadWindow return value. The code doesn't check for an error code from XQueryTree and then attempts to free 'children', which is an uninitialized local variable that holds a random value.

However, this was just a clue and didn't provide the root cause for the mWindow value resulting in a BadWindow error. I had another look at the VisualizationFrame code in rviz that the myviz code is emulating. In the initialize() method there are several calls to app->processEvents: https://github.com/ros2/rviz/blob/ros2/rviz_common/src/rviz_common/visualization_frame.cpp#L246-#L375

I included some of those calls and the code now works. I believe the reason is that processing the message allows the window(s) to be properly initialized. Once initialized in this way, there is no BadWindow error.

I've updated the code here: https://github.com/mjeronimo/rviz_embed_test. I've borrowed some from @MartiBolet's code (thanks again for the help). Hopefully this is a good enough starting point for what you need to do. I've tried it with Rolling, but haven't tried with Foxy yet. Perhaps you could give it a try.

[JaehyunShim]

@mjeronimo Thanks a lot. One more thing, the code is actually from https://github.com/ros-visualization/visualization_tutorials/tree/ros2/librviz_tutorial. If you send a pull request to this repository, it will be reviewed & maintained & updated by the community. I will test the code, see how different your code is from the one from ROS1 and add another comment later on.

[MartiBolet]

Thank you @mjeronimo,
I tested your repo and the RenderPanel is created and you can visualize in the MainWindow but I can't change the view, I can't drag the mouse or scroll and zoom in or zoom out. Did this happens to you or it's just me?

[JaehyunShim]

@MartiBolet

That's because he didn't add ViewController. Actually, I was thinking, using a pre-made frame is not a bad idea for such reasons. Users can take that easy option if he only adds the function with a RenderPanel type return to the VisualizationFrame class as you did.

(Update)
I am reading rviz_common::RenderPanel code and maybe I am wrong about this point

That's because he didn't add ViewController.

[JaehyunShim]

Hi @mjeronimo

I would like to ask how it is going with the conversation you had with @jacobperron about the release of the latest rviz_common source code for Foxy? 😃

Thank you for your help in advance!
Jaehyun

[mjeronimo]

@JaehyunShim We can backport relevant PRs to Foxy, but at the moment, we have a lot going on with the upcoming ROS 2 Galactic release. It'll take a bit of time to get to this. Thanks for your patience.

[JaehyunShim]

@mjeronimo

Hi, It's been more than a month since your response. I would like to ask how it is going with the release work. As you can see in this issue thread, I don't think I am the only person who is waiting for it.

Thanks

[mjeronimo]

@JaehyunShim Thanks for the reminder. I've submitted a backport PR here: #709. I've successfully tested this change on Foxy with my rviz_embed_test.

[mjeronimo]

@JaehyunShim Change integrated into Foxy branch and bloom released: ros/rosdistro#29963.

[JaehyunShim]

@mjeronimo Thank you very much. I will test it when it is available and let you know if it works without issues.

[mjeronimo]

@JaehyunShim You're welcome. Closing this item.

[JaehyunShim]

@mjeronimo

Hi, I tested rviz_common and you seem to have not added the "rviz_common/visualization_frame.hpp" file which is mentioned in our comments above. I also checked the foxy branch and it is not on the list. Could I get help with it?

Thanks,
Jaehyun

[JaehyunShim]

@mjeronimo

FYI, resource_retriever in the foxy branch will also be updated as retriever.hpp is used in the code in rviz_common. Please let me know if there is anything I can help.

Regards,
Jaehyun

[mjeronimo]

@JaehyunShim I'm currently on vacation through July 21st. Feel free to submit PRs for any missing files and I'll review when I get a chance.

[JaehyunShim]

@mjeronimo Hi thank you very much for your reply. PR is just the code in the ros2 branch. Could you merge it to the foxy branch as it has not hit EOL yet?

[jacobperron]

@JaehyunShim Feel free to open a PR backporting the changes to the foxy and/or galactic branches and we can review it.

[JaehyunShim]

@jacobperron

Thank you very much for your support.

1. I can see the galactic branch is up to date.
2. Foxy currently needs the same support (syncing with the ros2 branch).
3. I do not understand what you mean by sending a PR. It only has the dependency problem (https://github.com/ros/resource_retriever) as it already works in galatic. It only needs bloom release for the dependency (resource_retriever) and merging the ros2 branch of rviz_common to foxy branch.

It can wait if there is other work in priority but I am asking if the Foxy version can also get continuous support as the support for Foxy is until May 2023 (https://docs.ros.org/en/foxy/Releases.html) and it is currently the only existing LTS version.

Thanks,
Jaehyun

[clalancette]

• Foxy currently needs the same support (syncing with the ros2 branch).

The thing is, we do not really "sync" between the ros2 branch and the older branches. We carefully backport each change to older versions, being careful of API and ABI concerns. So it needs someone to actively push it forward.

It can wait if there is other work in priority but I am asking if the Foxy version can also get continuous support as the support for Foxy is until May 2023 (https://docs.ros.org/en/foxy/Releases.html) and it is currently the only existing LTS version.

We do support Foxy, but we rely on the community to put the work into the parts that they are interested in. In this case, you are specifically interested in the Foxy backport of this, so we are asking for your support in creating a PR that backports the functionality from the ros2 branch to Foxy.

[JaehyunShim]

@clalancette

Thank you very much for your explanation! That explanation is very helpful and clear. If that is the case, I would like to ask the following questions.

1. No backporting for the dependencies too? While more features from ROS1 rviz were added to ROS2 rviz_common, there were changes made also in its dependencies (ex, link). CI will crash if you merge rviz branches without updating the dependencies.

2. If 1) is possible, the below combination should work since it was working when I downloaded the source code on 23 April and tested.
   rviz for e19a075 commit
   resource_retriever for 791becb15061bfb064512a17ddbd341eeb14cef7 commit
   Let me go to my office and double-check it works.

[JaehyunShim]

Hi @clalancette

OK! I tested 2) and it perfectly worked without any issues. You mentioned

being careful of API and ABI concerns

but could I ask about it in more detail? Other than potential CI crashes, is there anything else that has to be considered? (ex, do you have sort of a policy that only certain features can be provided in old distros you can encourage users to use newer distros?)

Thanks,

[clalancette]

(ex, do you have sort of a policy that only certain features can be provided in old distros you can encourage users to use newer distros?)

No, we can backport features to older distributions if it makes sense, but...

but could I ask about it in more detail? Other than potential CI crashes, is there anything else that has to be considered?

We guarantee API and ABI stability within a ROS distribution. That is, once we do the release, we make sure not break either the programming API, and we make sure not to break the binary ABI (which is the layout of the code and data in memory). So sometimes when we do backports, we have to make certain changes to the code to make sure it keeps the API and the ABI. That's why we have to evaluate each code change that gets backported individually.