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

Resize RenderWidget properly when using different DPI scaling. #1263

Merged
merged 7 commits into from
Aug 27, 2019

Conversation

rlober
Copy link
Contributor

@rlober rlober commented Jun 19, 2018

See #1262 for details.

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Sorry for the long delay. rviz didn't have an active maintainer for a while...
I need to find a high-dpi display to test this 😉, but otherwise looks reasonable.

src/rviz/ogre_helpers/render_widget.cpp Show resolved Hide resolved
@atyshka
Copy link

atyshka commented Jun 9, 2019

+1 on this. I have a HiDPI mac and have this issue with RViz2 in ROS. It would be great if this was merged and added to the ros 2 repo also.

Copy link

@sippeyxp sippeyxp left a comment

Choose a reason for hiding this comment

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

I have a similar issue as #1262 on a Linux(debian 9). This fix is able to fix the issue if pixel_ratio_ is kept as double.

@rlober rlober closed this Jul 9, 2019
@rlober rlober deleted the kinetic-devel branch July 9, 2019 12:47
@rlober rlober restored the kinetic-devel branch July 9, 2019 12:49
@rlober rlober reopened this Jul 9, 2019
@rlober
Copy link
Contributor Author

rlober commented Jul 11, 2019

So all the requested changes have been made and everything seems to work as expected. I can't help the ABI change without making a global variable.

@atyshka
Copy link

atyshka commented Jul 18, 2019

Any progress on merging this?

@tompe17
Copy link

tompe17 commented Jul 25, 2019

I just manually applied tese patches. I really need them... So a merge would be very good.

But I noticed when using these patches together with jsk-visualization and overlay-menu that the menu was not placed in the middle. The rest of the things seems to work.

Copy link
Contributor Author

@rlober rlober left a comment

Choose a reason for hiding this comment

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

These changes should be reflected in the latest commits.

Copy link
Contributor Author

@rlober rlober left a comment

Choose a reason for hiding this comment

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

@rhaschke Sorry to leave these changes dangling like this. I have made all of the necessary changes in the recent commits but I do not know how to close out these changes you have requested me to review and I feel like it's the last thing keeping the PR from being merged?

@rhaschke
Copy link
Contributor

I would really like to validate those changes before merging. Could anybody (maybe @sippeyxp) hint me at instructions on how to set up a high-dpi monitor on Linux (Ubuntu) to
a) reproduce the issue and
b) validate that this PR fixes it.

@tompe17, could you please elaborate more on the issue described in #1263 (comment)?

@rlober
Copy link
Contributor Author

rlober commented Aug 16, 2019

@rhaschke what kind of monitors do you have access to?

@rhaschke
Copy link
Contributor

I just have a laptop (1920x1080) with an external monitor (1920x1200). But I guess, by configuring it somehow, I can pretend that the monitor is high-dpi (resulting in huge fonts / gui elements on that screen), right? So my questions is: How do I configure it like this?

@atyshka
Copy link

atyshka commented Aug 16, 2019

Should be able to do it in Ubuntu display settings... unless it knows the resolution and only offers scaling for hidpi. In that case maybe you could try a vm with custom display settings?

@rhaschke
Copy link
Contributor

I don't get high-dpi display settings offered.

@VictorLamoine
Copy link
Contributor

@rhaschke https://askubuntu.com/a/960041/73160 does this help ?

@sippeyxp
Copy link

Verified the patch fixed issue in my build (lunar). Builds fine and looks perfectly on 3840x2160 15" display.

Not sure why the cont. integration is not happy.

@rlober
Copy link
Contributor Author

rlober commented Aug 19, 2019

@rhaschke

I don't know if you can trick the high DPI checking in QT without having the physical pixels. Let me know if @VictorLamoine 's link works for you.

@sippeyxp

Thanks for checking. The CI is unhappy due to the change to the header render_widget.h (see here) causing an ABI incompatibility with librviz (see here) meaning that people linking to this will have to recompile for it to work.

@tompe17
Copy link

tompe17 commented Aug 19, 2019

The issue I saw was that an overlay menu that is centered on low resolution devices seemed to be centered in the top left 1/4 of the high resolution screen. But thing still works so do not let this block the patch. The alternative is that it does not work at all.

@rhaschke
Copy link
Contributor

I managed to have a custom pixel ratio via:
QT_SCALE_FACTOR=1.5 rviz

@rhaschke rhaschke merged commit 75c4918 into ros-visualization:kinetic-devel Aug 27, 2019
rhaschke pushed a commit to rhaschke/rviz that referenced this pull request Aug 27, 2019
This was referenced Aug 31, 2019
@rlober
Copy link
Contributor Author

rlober commented Nov 6, 2019

Can this be merged into melodic-devel also ?

@rhaschke
Copy link
Contributor

rhaschke commented Nov 6, 2019

It is already forward-ported (1342d2e) and released into Melodic. Are there any open issues in Melodic?

@Tacoma
Copy link

Tacoma commented Apr 30, 2020

Hi, I hope this reaches the right people :D
I noticed that the QApplication::sync() that was added in line 84 of the render_widget.cpp causes the VisualizationFrame to be initialized twice. I wonder if this is expected behaviour and why this sync() call is needed.
I noticed the problem because I advertise a service in a custom rviz display plugin. This fails when starting rviz with a config including this plugin.
Is it save for me to remove the QApplication::sync() call in my rviz version? I'm fine with building rviz myself.

I printed the stack-trace every time my plugin is initialized to find the problem:

[ INFO] Initializing preview service.
[ INFO] waitForService: Service [/save_preview] has not been advertised, waiting...
 0# TextureDisplay::onInitialize()
 1# rviz::Display::initialize(rviz::DisplayContext*) in librviz.so
 2# rviz::DisplayGroup::load(rviz::Config const&) in librviz.so
 3# rviz::VisualizationManager::load(rviz::Config const&) in librviz.so
 4# rviz::VisualizationFrame::load(rviz::Config const&) in librviz.so
 5# rviz::VisualizationFrame::loadDisplayConfig(QString const&) in librviz.so
 6# rviz::VisualizationFrame::initialize(QString const&) in librviz.so
 7# RvizPluginFactory::create(QString const&, QUrl const&, QStringList const&, QStringList const&) const
 8# 0x00007F219C593861 in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
 9# 0x00007F219D5A51D4 in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
10# 0x00007F219D5A66BA in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
11# 0x00007F219D154FF8 in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
12# 0x00007F219D15A2F4 in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
13# 0x00007F219D62733C in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
14# 0x00007F219D630830 in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
15# 0x00007F219D63087C in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
16# 0x00007F219D6D496E in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
17# QObject::event(QEvent*) in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
18# QApplicationPrivate::notify_helper(QObject*, QEvent*) in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
19# QApplication::notify(QObject*, QEvent*) in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
20# QCoreApplication::notifyInternal2(QObject*, QEvent*) in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
21# QTimerInfoList::activateTimers() in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
22# 0x00007F219AC8B391 in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
23# g_main_context_dispatch in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
24# 0x00007F218F1DD650 in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
25# g_main_context_iteration in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
26# QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
27# QGuiApplication::sync() in /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5
28# rviz::RenderWidget::RenderWidget(rviz::RenderSystem*, QWidget*) in librviz.so
29# rviz::QtOgreRenderWindow::QtOgreRenderWindow(QWidget*) in librviz.so
30# rviz::RenderPanel::RenderPanel(QWidget*) in librviz.so
31# rviz::VisualizationFrame::initialize(QString const&) in librviz.so
32# RvizPluginFactory::create(QString const&, QUrl const&, QStringList const&, QStringList const&) const
33# 0x00007F219C593861 in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
34# 0x00007F219D5A51D4 in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
35# 0x00007F219D5A66BA in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
36# 0x00007F219D154FF8 in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
37# 0x00007F219D15A2F4 in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
38# 0x00007F219D7B3258 in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
39# 0x00007F219D27D98E in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
40# 0x00007F219D29681A in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
41# 0x00007F219D2A5DAC in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
42# 0x00007F219D124CA6 in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
43# 0x00007F219D1246F1 in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
44# 0x00007F219D3AC813 in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
45# 0x00007F219D3B824A in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
46# 0x00007F219D3B838E in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
47# 0x00007F219DFB9C3F in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
48# 0x00007F219CAEB568 in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
49# 0x00007F219C8A399D in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
[ INFO] service advertised.
...
[ INFO] Initializing preview service.
 0# TextureDisplay::onInitialize()
 1# rviz::Display::initialize(rviz::DisplayContext*) in librviz.so
 2# rviz::DisplayGroup::load(rviz::Config const&) in librviz.so
 3# rviz::VisualizationManager::load(rviz::Config const&) in librviz.so
 4# rviz::VisualizationFrame::load(rviz::Config const&) in librviz.so
 5# rviz::VisualizationFrame::loadDisplayConfig(QString const&) in librviz.so
 6# rviz::VisualizationFrame::initialize(QString const&) in librviz.so
 7# RvizPluginFactory::create(QString const&, QUrl const&, QStringList const&, QStringList const&) const
 8# 0x00007F219C593861 in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
 9# 0x00007F219D5A51D4 in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
10# 0x00007F219D5A66BA in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
11# 0x00007F219D154FF8 in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
12# 0x00007F219D15A2F4 in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
13# 0x00007F219D7B3258 in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
14# 0x00007F219D27D98E in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
15# 0x00007F219D29681A in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
16# 0x00007F219D2A5DAC in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
17# 0x00007F219D124CA6 in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
18# 0x00007F219D1246F1 in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
19# 0x00007F219D3AC813 in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
20# 0x00007F219D3B824A in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
21# 0x00007F219D3B838E in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
22# 0x00007F219DFB9C3F in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
23# 0x00007F219CAEB568 in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
24# 0x00007F219C8A399D in /usr/lib/x86_64-linux-gnu/libQt5WebKit.so.5
[ERROR] Tried to advertise a service that is already advertised in this node [/save_preview]

@rhaschke
Copy link
Contributor

Sorry @Tacoma, but I can't reproduce your issue. For me the stack trace looks like this:

#0  0x00007fff98ae80e1 in rviz::GridDisplay::onInitialize() (this=0x55555648cdc0) at /vol/sandbox/rviz/src/rviz/src/rviz/default_plugin/grid_display.cpp:115
#1  0x00007ffff7a6c48d in rviz::Display::initialize(rviz::DisplayContext*) (this=this@entry=0x55555648cdc0, context=<optimized out>) at /vol/sandbox/rviz/src/rviz/src/rviz/display.cpp:95
#2  0x00007ffff7a83d01 in rviz::DisplayGroup::load(rviz::Config const&) (this=0x555556087380, config=...) at /vol/sandbox/rviz/src/rviz/src/rviz/display_group.cpp:109
#3  0x00007ffff7b48a70 in rviz::VisualizationManager::load(rviz::Config const&) (this=this@entry=0x555556660ba0, config=...) at /vol/sandbox/rviz/src/rviz/src/rviz/visualization_manager.cpp:510
#4  0x00007ffff7b441bb in rviz::VisualizationFrame::load(rviz::Config const&) (this=0x55555586bad0, config=...) at /vol/sandbox/rviz/src/rviz/src/rviz/visualization_frame.cpp:850
#5  0x00007ffff7b44f9f in rviz::VisualizationFrame::loadDisplayConfigHelper(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (this=this@entry=0x55555586bad0, full_path="/tmp/empty.rviz") at /vol/sandbox/rviz/src/rviz/src/rviz/visualization_frame.cpp:765
#6  0x00007ffff7b4514c in rviz::VisualizationFrame::loadDisplayConfig(QString const&) (this=this@entry=0x55555586bad0, qpath=...) at /vol/sandbox/rviz/src/rviz/src/rviz/visualization_frame.cpp:736
#7  0x00007ffff7b45b5b in rviz::VisualizationFrame::initialize(QString const&) (this=this@entry=0x55555586bad0, display_config_file=...) at /vol/sandbox/rviz/src/rviz/src/rviz/visualization_frame.cpp:353
#8  0x00007ffff7b4f99e in rviz::VisualizerApp::init(int, char**) (this=0x7fffffffd430, argc=<optimized out>, argv=<optimized out>) at /vol/sandbox/rviz/src/rviz/src/rviz/visualizer_app.cpp:212
#9  0x0000555555554ec1 in main(int, char**) (argc=3, argv=0x7fffffffd598) at /vol/sandbox/rviz/src/rviz/src/rviz/main.cpp:40

Looks like you are creating a new VisualizationFrame in a plugin?
In any case, please open a new issue and refer to related ones if you see fit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants