LiangliangNan / Easy3D

A lightweight, easy-to-use, and efficient C++ library for processing and rendering 3D data
GNU General Public License v3.0
1.37k stars 245 forks source link

[BUG] - MultiViewer is broken on Linux with a hidpi monitor #179

Open r3dl3g opened 7 months ago

r3dl3g commented 7 months ago

Describe the bug On a hidpi monitor (were dpi_scaling is 2) the MultiViewer doesn't display as expected

Tell us how to reproduce the bug Just run the demo "Tutorial_205_MultiView" on a hidpi monitor.

Development/Running environment:

Additional context The dpi_scaling is unnecessary applied to the sub viewports.

Here's the diff for my patch to fix it:

diff --git a/easy3d/viewer/multi_viewer.cpp b/easy3d/viewer/multi_viewer.cpp
index 3116de7d..d7f943ba 100644
--- a/easy3d/viewer/multi_viewer.cpp
+++ b/easy3d/viewer/multi_viewer.cpp
@@ -222,14 +222,16 @@ namespace easy3d {

         for (int i = 0; i < num_rows_; ++i) {
             auto &row = views_[i];
-            const auto y = height() - (i + 1) * view_height_;
-            for (int j = 0; j < num_cols_; ++j)
+            const auto y = i * view_height_;
+            for (int j = 0; j < num_cols_; ++j) {
+                const auto x = j * view_width_;
                 row[j].viewport = ivec4(
-                        static_cast<int>(static_cast<float>(j * view_width_) * dpi_scaling()),
-                        static_cast<int>(static_cast<float>(y) * dpi_scaling()),
-                        static_cast<int>(static_cast<float>(view_width_) * dpi_scaling()),
-                        static_cast<int>(static_cast<float>(view_height_) * dpi_scaling())
+                        static_cast<int>(static_cast<float>(x)),
+                        static_cast<int>(static_cast<float>(y)),
+                        static_cast<int>(static_cast<float>(view_width_)),
+                        static_cast<int>(static_cast<float>(view_height_))
                 );
+            }
         }

         // ------------------------------------------------------------
LiangliangNan commented 7 months ago

Thanks for reporting the potential issue. I don't have such a device for debugging, which makes it tricky for me. Can you attach some snapshots?

LiangliangNan commented 7 months ago

By checking your code, I found that you have changed const auto y = height() - (i + 1) * view_height_; to const auto y = i * view_height_; This seems wrong because the OpenGL tradition is to have the origin of the coordinate system at the lower left corner. But the window system (based on GLFW) has that at the top left corner. I still believe the dpi_scaling should be applied, so I guess your change to the calculation of the Y coordinate is the source of the issue. Can you please check?

r3dl3g commented 7 months ago

Of course,

Without the patch: Without the patch

With the patch: With the patch

r3dl3g commented 7 months ago

By checking your code, I found that you have changed const auto y = height() - (i + 1) * view_height_; to const auto y = i * view_height_; This seems wrong because the OpenGL tradition is to have the origin of the coordinate system at the lower left corner. But the window system (based on GLFW) has that at the top left corner.

Ah, I didn't was aware of this OpenGL tradition. So sorry for this unnecessary change.

r3dl3g commented 7 months ago

So I changed this part back and that's the result: Bildschirmfoto vom 2024-03-07 11-11-34

And that's the diff now:

diff --git a/easy3d/viewer/multi_viewer.cpp b/easy3d/viewer/multi_viewer.cpp
index 3116de7d..1eeb59cf 100644
--- a/easy3d/viewer/multi_viewer.cpp
+++ b/easy3d/viewer/multi_viewer.cpp
@@ -223,13 +223,15 @@ namespace easy3d {
         for (int i = 0; i < num_rows_; ++i) {
             auto &row = views_[i];
             const auto y = height() - (i + 1) * view_height_;
-            for (int j = 0; j < num_cols_; ++j)
+            for (int j = 0; j < num_cols_; ++j) {
+                const auto x = j * view_width_;
                 row[j].viewport = ivec4(
-                        static_cast<int>(static_cast<float>(j * view_width_) * dpi_scaling()),
-                        static_cast<int>(static_cast<float>(y) * dpi_scaling()),
-                        static_cast<int>(static_cast<float>(view_width_) * dpi_scaling()),
-                        static_cast<int>(static_cast<float>(view_height_) * dpi_scaling())
+                        static_cast<int>(static_cast<float>(x)),
+                        static_cast<int>(static_cast<float>(y)),
+                        static_cast<int>(static_cast<float>(view_width_)),
+                        static_cast<int>(static_cast<float>(view_height_))
                 );
+            }
         }

         // ------------------------------------------------------------
r3dl3g commented 7 months ago

Btw thanks for this wonderful piece of code. I've been looking for a real high level 3D library for a long time, without the need for masses of glx boilerplate code and then luckily I found this one.

LiangliangNan commented 7 months ago

Thanks for your kind words! I believe the issue is in some other part(s) of the code. For now, you can use your workaround solution. I cannot debug it because I don't have such a device, but I will check with some friends if can further investigate this issue. Can you change, in the main.cpp file, from initialize(); to initialize(true); Then you will see a log file Tutorial_205_MultiView.log will be generated next to the executable. I would be grateful if you post the entire log here, in the hope that I can find some clue about the issue.

r3dl3g commented 7 months ago

Of course. Here there are:

Without patch: Tutorial_205_MultiView.log

With patch: Tutorial_205_MultiView-with_patch.log

As far as I can see, they are identical.

LiangliangNan commented 7 months ago

I don't have a Linux machine with a hidpi screen. But I've tried to fix this issue using a Mac (which has hidpi). The new code is in the 'hidpi' branch: https://github.com/LiangliangNan/Easy3D/tree/hidpi Can you check if it works and post a snapshot here?

r3dl3g commented 7 months ago

grafik

Sorry, no change on my machine with that fix.

LiangliangNan commented 7 months ago

Okay. Then please go with your changes. I will have to find a proper machine to further explore the issue.

r3dl3g commented 7 months ago

Just for completeness, I testet now also under windows10 on the same machine and got the same results, as under linux:

Original: MultiView_original

With Patch: MultiView_patched

r3dl3g commented 7 months ago

I forgot to mention a very small change to compile it with Visual Studio 2017:

diff --git a/easy3d/util/console_style.cpp b/easy3d/util/console_style.cpp
index d2d793ee..2a5632f3 100755
--- a/easy3d/util/console_style.cpp
+++ b/easy3d/util/console_style.cpp
@@ -30,6 +30,7 @@
 #include <vector>
 #include <cstring>
 #include <cstdlib>  // for std::getenv
+#include <string>

 namespace easy3d
 {

But that's does not affect this issue