kmatheussen / radium

A graphical music editor. A next generation tracker.
http://users.notam02.no/~kjetism/radium/
GNU General Public License v2.0
844 stars 36 forks source link

1/4 scale editor rendering on HiDPI screen #1112

Closed 0xc1c4da closed 6 years ago

0xc1c4da commented 6 years ago

Summary

The Editor GUI does not scale with the rest of Radium UI on HiDPI screen, however Radium runs well, great sound. The Editor UI refreshes and tracks well, it's just small.

1. Steps to reproduce the problem.

radium-hidpi-editor-error

I think / I'm pretty sure the bug happens when I have a HiDPI screen or when using Intel Graphics, seems like a scaling issue to me.

2. Which version of Radium are you using?

Radium 5.4.2
OpenGL vendor: "Intel Open Source Technology Center"
OpenGL renderer: "Mesa DRI Intel(R) HD Graphics 630 (Kaby Lake GT2) "
OpenGL version: "3.0 Mesa 17.3.7"
OpenGL flags: 107f
Qt version: "5.10.1: x86_64-little_endian-lp64"
C/C++ compiler version: 7.3.1 20180312
S7 version: 5.12 / 3-Oct-17

Do you know if an earlier version has worked? No

3. If relevant, which operating system did you run Radium on?

Linux 4.15.12-1-hardened #1 SMP PREEMPT Thu Mar 22 02:53:45 CET 2018 x86_64 GNU/Linux with KDE Plasma 5

kmatheussen commented 6 years ago

Does it help if you run

export QT_AUTO_SCREEN_SCALE_FACTOR=1

first?

kmatheussen commented 6 years ago

Please also try

export QT_AUTO_SCREEN_SCALE_FACTOR=0

if export QT_AUTO_SCREEN_SCALE_FACTOR=1 didn't make a difference. (not sure whether it should be 0 or 1)

0xc1c4da commented 6 years ago

Thanks for timely response! export QT_AUTO_SCREEN_SCALE_FACTOR=0 results in the same image as above: radium-hidpi-qt_auto_screen_scale_factor-0 Ignore the visual artifact in the black area, it's just krunner and spectacle foolery.

export QT_AUTO_SCREEN_SCALE_FACTOR=1 Results in the following: radium-hidpi-qt_auto_screen_scale_factor-1

0xc1c4da commented 6 years ago

Here is export QT_AUTO_SCREEN_SCALE_FACTOR=1 playing a demo song, rendering issue persist, even smaller than with export QT_AUTO_SCREEN_SCALE_FACTOR=0 and original problem.

radium-hidpi-qt_auto_screen_scale_factor-1-demo-song

kmatheussen commented 6 years ago

This is strange. Seems like one part of the program is scaled differently than the rest. I really don't want Qt or the OS to scale anything, since the graphics in Radium does (or at least should) scale. It's also difficult to test this since I don't own an HiDPI monitor.

Anyway, here's a couple more things to try:

export QT_AUTO_SCREEN_SCALE_FACTOR=auto
export QT_DEVICE_PIXEL_RATIO=auto
0xc1c4da commented 6 years ago

Same UI as with QT_AUTO_SCREEN_SCALE_FACTOR=1 Also noticed a deprecated warning.

[a@pod]: ~>$ export QT_AUTO_SCREEN_SCALE_FACTOR=auto
[a@pod]: ~>$ export QT_DEVICE_PIXEL_RATIO=auto
[a@pod]: ~>$ radium
JUCE v4.2.4
INIT4
INIT5
Warning: Warning: QT_DEVICE_PIXEL_RATIO is deprecated. Instead use:
   QT_AUTO_SCREEN_SCALE_FACTOR to enable platform plugin controlled per-screen factors.
   QT_SCREEN_SCALE_FACTORS to set per-screen factors.
   QT_SCALE_FACTOR to set the application global scale factor. ((null):0, (null))

Did a quick google, I noticed a resolution of GL Views being half-sized in pyqtgraph https://github.com/pyqtgraph/pyqtgraph/pull/516/files returning viewport width/height multiplied by screen devicePixelRatio, maybe it's related?

kmatheussen commented 6 years ago

Did a quick google, I noticed a resolution of fixing GL Views being half-sized in pyqtgraph https://github.com/pyqtgraph/pyqtgraph/pull/516/files returning viewport width/height multiplied by screen devicePixelRatio, maybe it's related?

Definitely seems related. I guess that's the solution. I'll try to make a patch now.

kmatheussen commented 6 years ago

Well, it's a little bit more work than I first thought. The patch below should make the editor use all space, but the content is not scaled the same way as the header. And I also don't know whether mouse coordinates are correct, or if those needs to be scaled as well.

[kjetil@localhost radium]$ git diff 
diff --git a/OpenGL/Widget.cpp b/OpenGL/Widget.cpp
index 8a3f6de..9168825 100644
--- a/OpenGL/Widget.cpp
+++ b/OpenGL/Widget.cpp
@@ -1462,8 +1462,8 @@ public:
     if (h<32)
       h = 32;

-    current_width = w;
-    current_height = h;
+    current_width = w * 2; // Just use a hardcoded value of 2 for now.
+    current_height = h * 2; // same here

     initEvent();

I'll have to think about what's best, to scale the editor parameters up, or to scale the editor down. Either require much bigger patches.

0xc1c4da commented 6 years ago

The mouse coordinates were correct, it's only the rendering that appears to be affected.

Haven't applied or tried patch, will attempt

0xc1c4da commented 6 years ago

This is what I see with the patch applied. It seems like the black has been replaced with the theme colour but the tracks themselves are in the bottom lower 1/4. I'm also noticing that the visual scrolling during playback is stuttering even with AA off (audio plays fine).

radium-hidpi-widget-patched

For what it's worth btw, Renoise doesn't scale at all (:

kmatheussen commented 6 years ago

Maybe one of those QT environment variables are still set? I think the editor should use all available space if you don't set any QT environment variables.

kmatheussen commented 6 years ago

The stuttering is not good though. I guess it's too many pixels. (Are you sure hardware acceleration is enabled?). I don't know if there is any solution to that problem though, except a faster GPU.

0xc1c4da commented 6 years ago

This was from a fresh zsh environment

[a@pod]: ~>$ echo $QT_AUTO_SCREEN_SCALE_FACTOR
0
[a@pod]: ~>$ echo $QT_DEVICE_PIXEL_RATIO

[a@pod]: ~>$ radium
JUCE v4.2.4
INIT4
INIT5
********* Has set startup rect 0, 0**********
0xc1c4da commented 6 years ago

It is drawing in the space correctly, I tried a demo song with a longer sequence and you can see the track can be rendered in the view. It seems the tracks themselves are independently sized.

radium-hidpi-widget-patched-longer-song

kmatheussen commented 6 years ago

Hmm, maybe this helps:

-    GE_set_height(h);
+    GE_set_height(h*2);
0xc1c4da commented 6 years ago

With

    current_width = w * 2;
    current_height = h * 2 ;

    initEvent();

    GE_set_height(h * 2);

radium-hidpi-ge_set_height

kmatheussen commented 6 years ago

This is definitely not the proper solution, but I think it might help:

diff --git a/Qt/Qt_EventReceiver.cpp b/Qt/Qt_EventReceiver.cpp
index cc054a9..04e0cdc 100755
--- a/Qt/Qt_EventReceiver.cpp
+++ b/Qt/Qt_EventReceiver.cpp
@@ -593,7 +593,7 @@ void EditorWidget::resizeEvent( QResizeEvent *qresizeevent){ // Only QT VISUAL!
 #endif

   if (this->window != NULL){
-    this->window->width=qresizeevent->size().width(); //this->get_editor_width();
+    this->window->width=2*qresizeevent->size().width(); //this->get_editor_width();
     this->window->height=qresizeevent->size().height(); //this->get_editor_height();
   }

The horizontal scroll bar for the editor will be screwed up though.

Think I'll try to make OpenGL scale the editor instead though. I don't know how to do that, but I guess it's a matter of finding the right parameter for a call to vl::Camera::setProjectionMatrix: http://visualizationlibrary.org/docs/2.0/html/classvl_1_1_camera.html#a595dba4f10a739d008cb487bc2008168 (replacing the call to setProjectionOrtho() in OpenGL/Widget.cpp)

0xc1c4da commented 6 years ago

Hmm, didn't appear to change anything visually on the horizontal scale. However some good news, for some reason I have no visual playback stuttering.

0xc1c4da commented 6 years ago

Getting closer! In addition the the above changes, I've tried replacing _rendering->camera()->setProjectionOrtho(-0.5f); with

    _rendering->camera()->setProjectionMatrix( vl::mat4::getOrtho(
                                            -0.5f * 0.5, 
                                             (_rendering->camera()->viewport()->width() + -0.5f) * 0.5, 
                                            -0.5f * 0.5, 
                                             (_rendering->camera()->viewport() ->height() + -0.5f) * 0.5, 
                                            -1.0, +1.0), vl::EProjectionMatrixType::PMT_OrthographicProjection );

( I have nfi what I'm doing) and got: radium-hidpi-orthoprojection and radium-hidpi-orthoprojection2

It seems to render correctly vertically and the rendering is horizontally seems to be in-line with instrument tracker headers. Although horizontal scrolling seems to be broken and more importantly the second half of the Editor view isn't being rendered into properly. I also cannot see the current position marker. Visual playback performance is buttery smooth

kmatheussen commented 6 years ago

Great! The parameters for setProjectionMatrix seems correct. But I wonder why the fonts are not scaled. To fix horizontal rendering, I hope this patch is enough:

diff --git a/OpenGL/GfxElements.cpp b/OpenGL/GfxElements.cpp
index 3762a4a..f4622fb 100644
--- a/OpenGL/GfxElements.cpp
+++ b/OpenGL/GfxElements.cpp
@@ -664,7 +664,8 @@ public:

     if (_scissor.get()==NULL){
       const SharedVariables *shared_variables = GE_get_shared_variables(painting_data);
-      _scissor = new vl::Scissor(shared_variables->wtracks_scissor_x1, 0, shared_variables->wtracks_scissor_x2 - shared_variables->wtracks_scissor_x1, g_height);
+      double scale_factor = 2.0;
+      _scissor = new vl::Scissor(shared_variables->wtracks_scissor_x1 * scale_factor, 0, shared_variables->wtracks_scissor_x2 - shared_variables->wtracks_scissor_x1, g_height);
     }
kmatheussen commented 6 years ago

And here's fix for font size:

diff --git a/OpenGL/TextBitmaps.hpp b/OpenGL/TextBitmaps.hpp
index 9e9f4d1..f46994a 100644
--- a/OpenGL/TextBitmaps.hpp
+++ b/OpenGL/TextBitmaps.hpp
@@ -187,8 +187,14 @@ static inline void set_halfsize_font(void){
   g_qfont_halfsize = g_qfont;
   g_imageholders_halfsize = g_imageholders;
 }
- 
-static inline void GE_set_new_font(const QFont &font){
+
+
+static const double g_scale_factor = 0.5;
+
+static inline void GE_set_new_font(const QFont &nonscaled_font){
+  QFont font(nonscaled_font);
+  font.setPointSize(font.pointSize() * g_scale_factor);
+  
   g_qfont = font;

   g_imageholders = add_new_font(font);
@@ -244,10 +250,10 @@ struct TextBitmaps{
       //float x2 = x + image->width()-1;
       //float y2 = y + image->height()-1;

-      points[c].push_back(vl::dvec2(x+holder.width,y-image->height()/2.0));//(y+y2)/2));
+      points[c].push_back(vl::dvec2(x+holder.width/g_scale_factor,y-image->height()/g_scale_factor/2.0));//(y+y2)/2));
     }

-    return x + holder.width;
+    return x + holder.width/g_scale_factor;
   }
kmatheussen commented 6 years ago

Sorry, g_scale_factor above should be 2.0 for you, not 0.5. (I scaled down while testing, so for me it's 0.5)

0xc1c4da commented 6 years ago

Font fix certainly helped, unclear _scissor patch did much? scrolling horizontally looks better. Also I noticed that I think its tracks vertically too high, and the end of a block its playing but disappears.

radium-hidpi-latestpatches

kmatheussen commented 6 years ago

Maybe this one works better for the horizontal problem:

diff --git a/OpenGL/GfxElements.cpp b/OpenGL/GfxElements.cpp
index 3762a4a..338aa6e 100644
--- a/OpenGL/GfxElements.cpp
+++ b/OpenGL/GfxElements.cpp
@@ -664,7 +664,8 @@ public:

     if (_scissor.get()==NULL){
       const SharedVariables *shared_variables = GE_get_shared_variables(painting_data);
-      _scissor = new vl::Scissor(shared_variables->wtracks_scissor_x1, 0, shared_variables->wtracks_scissor_x2 - shared_variables->wtracks_scissor_x1, g_height);
+      double scale_factor = 0.5;
+      _scissor = new vl::Scissor(shared_variables->wtracks_scissor_x1 * scale_factor, 0, (shared_variables->wtracks_scissor_x2 - shared_variables->wtracks_scissor_x1)*scale_factor, g_height);
     }

     return _scissor.get();
kmatheussen commented 6 years ago

(and again, change 0.5 to 2.0. Sorry)

0xc1c4da commented 6 years ago

That did the trick! Horizontal works great!

radium-hidpi-scissorworks The only issue I see now, is that it's rendering too high I think (notice you can't see the track position)

At the end of a sequence or scrolling up too much I see this radium-hidpi-cutoffvertical

kmatheussen commented 6 years ago

I don't know what's happening here. But maybe this will work:

diff --git a/OpenGL/GfxElements.cpp b/OpenGL/GfxElements.cpp
index 3762a4a..a31b530 100644
--- a/OpenGL/GfxElements.cpp
+++ b/OpenGL/GfxElements.cpp
@@ -681,7 +681,7 @@ public:
     return scale(y,
                  0,height,
                  height,0
-                 );
+                 ) + height;
   }
kmatheussen commented 6 years ago

Sorry:

  static float y(float y){
    float height = safe_volatile_float_read(&g_height);
    return scale(y,
                 0,height,
                 height,0
                 ) - height/2.0;
  }
kmatheussen commented 6 years ago
index 3762a4a..8b3a80e 100644
--- a/OpenGL/GfxElements.cpp
+++ b/OpenGL/GfxElements.cpp
@@ -681,7 +681,7 @@ public:
     return scale(y,
                  0,height,
                  height,0
-                 );
+                 ) - height/2.0;
   }
0xc1c4da commented 6 years ago

w00t it works! It all seems to be behaving (to the best of my knowledge, new to the program) thank-you so much, also are you a wizard? (for being able to remote code like that) :P

radium-hidpi-itworks

kmatheussen commented 6 years ago

Wow! Do you have a list of all the patches that are applied? (git diff for instance). I think I know approx. the changes, but it would be best if there wasn't any doubt. At least the changes to OpenGL/Widget.cpp would be good to have.

0xc1c4da commented 6 years ago

Yep! this is what I have, you'll notice Qt_EventReceiver.cpp isn't modified

diff --git a/OpenGL/GfxElements.cpp b/OpenGL/GfxElements.cpp
index 3762a4a..a616ff5 100644
--- a/OpenGL/GfxElements.cpp
+++ b/OpenGL/GfxElements.cpp
@@ -664,7 +664,10 @@ public:

     if (_scissor.get()==NULL){
       const SharedVariables *shared_variables = GE_get_shared_variables(painting_data);
-      _scissor = new vl::Scissor(shared_variables->wtracks_scissor_x1, 0, shared_variables->wtracks_scissor_x2 - shared_variables->wtracks_scissor_x1, g_height);
+      // _scissor = new vl::Scissor(shared_variables->wtracks_scissor_x1, 0, shared_variables->wtracks_scissor_x2 - shared_variables->wtracks_scissor_x1, g_height);
+      double scale_factor = 2.0;
+      // _scissor = new vl::Scissor(shared_variables->wtracks_scissor_x1 * scale_factor, 0, shared_variables->wtracks_scissor_x2 - shared_variables->wtracks_scissor_x1, g_height);
+          _scissor = new vl::Scissor(shared_variables->wtracks_scissor_x1 * scale_factor, 0, (shared_variables->wtracks_scissor_x2 - shared_variables->wtracks_scissor_x1)*scale_factor, g_height);
     }

     return _scissor.get();
@@ -681,7 +684,7 @@ public:
     return scale(y,
                  0,height,
                  height,0
-                 );
+                 ) - height/2.0;
   }

   vl::Transform *get_transform(T2_data *t2_data, bool &is_scroll_transform) const {
diff --git a/OpenGL/TextBitmaps.hpp b/OpenGL/TextBitmaps.hpp
index 9e9f4d1..dbc2e36 100644
--- a/OpenGL/TextBitmaps.hpp
+++ b/OpenGL/TextBitmaps.hpp
@@ -187,8 +187,13 @@ static inline void set_halfsize_font(void){
   g_qfont_halfsize = g_qfont;
   g_imageholders_halfsize = g_imageholders;
 }
- 
-static inline void GE_set_new_font(const QFont &font){
+
+static const double g_scale_factor = 2.0;
+
+static inline void GE_set_new_font(const QFont &nonscaled_font){
+  QFont font(nonscaled_font);
+  font.setPointSize(font.pointSize() * g_scale_factor);
+
   g_qfont = font;

   g_imageholders = add_new_font(font);
@@ -244,10 +249,12 @@ struct TextBitmaps{
       //float x2 = x + image->width()-1;
       //float y2 = y + image->height()-1;

-      points[c].push_back(vl::dvec2(x+holder.width,y-image->height()/2.0));//(y+y2)/2));
+      // points[c].push_back(vl::dvec2(x+holder.width,y-image->height()/2.0));//(y+y2)/2));
+      points[c].push_back(vl::dvec2(x+holder.width/g_scale_factor,y-image->height()/g_scale_factor/2.0));//(y+y2)/2));
     }

-    return x + holder.width;
+    // return x + holder.width;
+    return x + holder.width/g_scale_factor;
   }

   // called from Main thread
diff --git a/OpenGL/Widget.cpp b/OpenGL/Widget.cpp
index 8a3f6de..d220b9a 100644
--- a/OpenGL/Widget.cpp
+++ b/OpenGL/Widget.cpp
@@ -39,6 +39,8 @@ Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA. */
 #include <vlGraphics/OcclusionCullRenderer.hpp>
 #include <vlGraphics/Actor.hpp>

+#include <vlCore/Matrix4.hpp>
+

 #include <QGLWidget>
 #include <QTextEdit>
@@ -883,7 +885,16 @@ private:

       _rendering->camera()->viewport()->setWidth(current_width);
       _rendering->camera()->viewport()->setHeight(current_height);
-      _rendering->camera()->setProjectionOrtho(-0.5f);
+      // glOrtho( -width/2*zoom, width/2*zoom, -height/2*zoom, height/2*zoom, -1, 1 );
+    _rendering->camera()->setProjectionMatrix( vl::mat4::getOrtho(
+                                            -0.5f * 0.5, 
+                                             (_rendering->camera()->viewport()->width() + 0.5f) * 0.5, 
+                                            0.5f * 0.5, 
+                                             (_rendering->camera()->viewport() ->height() + 0.5f) * 0.5, 
+                                            -1.0, +1.0), vl::EProjectionMatrixType::PMT_OrthographicProjection );
+
+
+      // _rendering->camera()->setProjectionOrtho(-1.0f);

       _rendering->camera()->viewport()->setClearColor(get_vec4(t2_data->background_color));
       //glClear(GL_STENCIL_BUFFER_BIT|GL_ACCUM_BUFFER_BIT|GL_DEPTH_BUFFER_BIT|GL_COLOR_BUFFER_BIT); makes no difference.
@@ -1462,12 +1473,12 @@ public:
     if (h<32)
       h = 32;

-    current_width = w;
-    current_height = h;
+    current_width = w * 2;
+    current_height = h * 2;

     initEvent();

-    GE_set_height(h);
+    GE_set_height(h * 2);

     GFX_ScheduleEditorRedraw();
   }
kmatheussen commented 6 years ago

Thank you.

kmatheussen commented 6 years ago

In OpenGL/Widget.cpp, I wonder if you could try to replace

GE_set_height(h*2);

with

GE_set_height(h);

(the original code)

and in OpenGL/GfxElements.cpp, replace

     return scale(y,
                  0,height,
                  height,0
+                 ) - height/2.0;
   }

with

     return scale(y,
                  0,height,
                  height,0
                 );
   }

(also the original code)

Hopefully this will look the same. I think what happens now is that the program paints up / prepares twice as much graphics as needed, and just displays the middle of it. It could make a difference in rendering speed, although I'm not sure.

kmatheussen commented 6 years ago

Sorry, I should have looked more at the code before posting the last comment. The call to GE_set_height() doesn't change the rendering height, only the scissor mask and so forth. No need to test anything.

kmatheussen commented 6 years ago

I've commited code to support high DPI now. Would be great if you could test it. It's a little bit different from the diff above (since it didn't calculate scissor and mask values correctly), so I'm not 100% sure it works, but pretty sure.

0xc1c4da commented 6 years ago

can confirm fresh clone from d752b9964fe32e4d763f633cbf77d715d174779a has full HiDPI support!

kmatheussen commented 6 years ago

Great! I also tried it on a mac with retina display now, and it worked there as well. Guess it's safe to close.