Almamu / linux-wallpaperengine

Wallpaper Engine backgrounds for Linux!
GNU General Public License v3.0
1.51k stars 56 forks source link

Fixed texture clamping caused by texture coordinates being outside of (0,1) interval #188

Closed Pasalc closed 7 months ago

Pasalc commented 8 months ago

Fix for issue #173.

Short description

Video gets clamped for 2659503088 wallpaper. Problem is caused by texture coordinates being out of boundaries, resulting in clamp.

Full description

Texture UV coordinates starts from 0 and ends at 1. We will change uend and see how it will affect video 2667198601. If we have:

float ustart = 0.0f;
float uend = 1.0f;
float vstart = 1.0f;
float vend = 0.0f;

we will get full size video: image

And if we have:

float ustart = 0.0f;
float uend = 0.5f;
float vstart = 1.0f;
float vend = 0.0f;

we will get only left halve of video: image

And if we try to render texture out of this boundries:

float ustart = 0.0f;
float uend = 1.5f;
float vstart = 1.0f;
float vend = 0.0f;

texture will be clamped to the edge (because _GL_CLAMP_TOEDGE parametr in CFBO.cpp is set): image

What caused problem

Previously, ustart/uend or vstart/vend could be less than 0 or bigger than 1 (for example ustart could be -0.16 and uend could be 1.15), resulting in texture clamping on left/right or top/bottom of window or screen(though I only had top/bottom clamp). By making them equal to 0 or 1 accordingly, we make sure that OpenGL renders whole texture with glDrawArrays and don't get out of boundaries so we have no clamping.

Before: image

After: image

Additional concerns

It probably shouldn't affect people from issue #81, but may be it's a good idea to ask if everything works fine for them.

Teddy-Kun commented 8 months ago

I can confirm this fixes the pixel artifacts, however I don't think this approach is always the best as this stretches the wallpaper to fit the size given by either the monitor or window. I believe that there could/should be a command line flag to change the behavior. On OLED screens I imagine it could be desirable to just leave the pixels outside completely black, whereas for my current personal wallpaper I would prefer cropping the sides to not stretch out the image. But to not undermine your work, this is 100% a lot better than before and the code definitely looks much cleaner and simpler to me!

Pasalc commented 8 months ago

Thank you for response! I would probably do as you suggested and add flags:

  1. fit the size image
  2. leave the pixels outside completely black image
  3. if flag was not provided: preserve things as they are now image

If i understand correctly, by

cropping the sides to not stretch out the image

you meant flag 2, but correct me if I got it wrong.

Pasalc commented 8 months ago

Added --clamp-strategy option, with stretch, border, repeat, clamp arguments. Can be shortend to -t and arguments to s, b, r, c. @Teddy-Kun for your case you should be able to write: ./linux-wallpaperengine <background_path/background_id> -t b and you will get black borders instead of clamp.

Not that important, but I also had to add in WallpaperEngine/Render/Drivers/Output/CGLFWWindowOutput.cpp #include <GL/glew.h> or else I would get compile error:

In file included from /home/kermit/Projects/fun/linux-wallpaperengine/src/WallpaperEngine/Render/Drivers/CVideoDriver.h:3,
                 from /home/kermit/Projects/fun/linux-wallpaperengine/src/WallpaperEngine/Render/Drivers/Output/CGLFWWindowOutput.h:3,
                 from /home/kermit/Projects/fun/linux-wallpaperengine/src/WallpaperEngine/Render/Drivers/Output/CGLFWWindowOutput.cpp:2:
/usr/include/GL/glew.h:85:2: error: #error gl.h included before glew.h
   85 | #error gl.h included before glew.h
      |  ^~~~~
/usr/include/GL/glew.h:97:2: error: #error glext.h included before glew.h
   97 | #error glext.h included before glew.h
      |  ^~~~~

Also repeat looks like this: image

Pasalc commented 8 months ago

Just realized that this issue might be related to #90 , so I would add another flag option and make zoom fill another option too. For now I just made it default. Looks fine to me, but I'm not sure it really works correctly, so more thought must be put into that. It looks like this for different window sizes(I don't know their exact size): image

image

image

image

So yeah, it's either zoomed to full width or to full height.

Pasalc commented 8 months ago

Overview

So I added 2 flags:

--scaling <mode>     Scaling mode for wallpaper. Can be stretch, fit, fill, default. Must be used before wallpaper provided.
                             Example: ./wallengine --scaling stretch --screen-root eDP-1 --bg 2667198601 --scaling fill --screen-root eDP-2 2667198602

--clamping <mode>    Clamping mode for all wallpapers. Can be clamp, border, repeat. Enables GL_CLAMP_TO_EDGE, GL_CLAMP_TO_BORDER, GL_REPEAT accordingly. Default is clamp.

Scaling mode should work with different screens, so you could for example set stretch for one wallpaper and fill for other. You must provide --scaling option before wallpaper was provided.

Clamping mode apply to all screens, so you can't set it for each screen individually.

Code

Added new class CWallpaperState files:

Added CWallpaperState as new member in CWallpaper class.

CWallpaperState saves last viewport, projection, vflip and UVs values. Now if nothing have changed(viewport, projection, vflip) then old UVs coordinates are used (or more simple: UVs are cached). If something have changed, then the value of m_textureUVsMode defines which scaling algorithm will be applied to recalculate UVs.

If any new scaling algorithm will be needed in the future, then it can be easily(I hope) added:

  1. Add new value to enum class TextureUVsScaling in CWallpaperState.h
  2. Provide new template specialization for your algorithm with template<> void CWallpaperState::updateTextureUVs<CWallpaperState::TextureUVsScaling::YOUR_ALGORITHM> in CWallpaperState.cpp
  3. Add new switch statement in CWallpaperState::updateState(...)
  4. Add new switch statement for case 't' in CApplicationContext.cpp

How Fit/Fill works

I think it's better to explain it with example with different sizes of wallpaper. Here we will only look on examples were only width is different, because the same logic applies to height.

Both equal Imagine you have screen with sizes 2000x2000 and wallpaper with sizes 2000x2000. Then, with default scale(the one that is used on main now) you will have left=0;right=2000. Just as it should be.

Width is smaller If screen sizes was 2000x2000 and wallpaper was 1000x2000, you would get left=-500; right=1500. So now you will get clamps, meaning wallpaper will zoom fit the screen.

1000x2000 drawio

Width is bigger If screen sizes was 2000x2000 and wallpaper was 3000x2000, you would get left=500; right=2500. So now you will see only part of wallpaper, meaning wallpaper will zoom fill the screen.

3000x2000 drawio(1)

The same principles applies if width is equal for screen and wallpaper, but the height differs.

Fit In code screen is viewport and wallpaper is projection. So, if we get either viewport.width==projection.width && viewport.height>projection.height or viewport.height==projection.height && viewport.width>projection.width we will get zoom fit when using code for default scale algorithm for findingUVs coordinates. To make one of this conditions true, we can use this code:

const float m1 = static_cast<float>(viewportWidth) / projectionWidth;
const float m2 = static_cast<float>(viewportHeight) / projectionHeight;
const float m = std::min(m1,m2);
projectionWidth*=m;
projectionHeight*=m;

Fit In code screen is viewport and wallpaper is projection. So, if condition is either viewport.width==projection.width && viewport.height<projection.height or viewport.height==projection.height && viewport.width<projection.width we will get zoom fit when using code for default scale algorithm for finding UVs coordinates. To make one of this conditions true, we can use same code as above, but max instead of min:

const float m1 = static_cast<float>(viewportWidth) / projectionWidth;
const float m2 = static_cast<float>(viewportHeight) / projectionHeight;
const float m = std::max(m1,m2);
projectionWidth*=m;
projectionHeight*=m;

Miscellaneous

Both --scaling and --clamping options are under case 't' in CApplicationContext.cpp, because there are a lot of letters already used for other options and I don't want to use even more if I can conserve.

Also added constexpr size_t customHash(const char* str) in CApplicationContext.cpp. Made it to have compile time hash for choosing options for scaling and clamping in switch statement. Could be done with simple if else statements, so maybe it's a bit of an overkill.

Almamu commented 7 months ago

The PR looks pretty good. There's slight adjustements I'd make to how scaling is calculated (mainly using templates to provide the different implementations), but I don't know when I'll have time to properly look at these, so accepting the PR as is. Thanks for your contribution and sorry for the delay in reviewing this!