LCD grid shader #369

DestinyXZ9 · 2024-03-27T04:51:41Z

This is a LCD grid shader based on LCD1x.
The parameters are "based" on my GBA SP with horizontal lines darker than the vertical ones.

fleroviux · 2024-03-30T18:23:41Z

Thanks! I noticed that there currently are graphical artifacts when using the LCD grid effect alongside LCD ghosting or xBRZ filtering. I think the issue with LCD ghosting is related to how we are currently inverting the vertical UV coordinate for the final render to the screen (default FBO). I am not sure anymore why that is the case, but probably the shader pipeline could be simplified if that issue would be solved outside the shader code.

Maybe @GranMinigun can give some input on this PR as they recently did a rewrite of the rendering loop and I'm not super familiar with the code anymore.

LCD grid + LCD ghosting

LCD grid + xBRZ filter

(in reality the artifacts are flickering and not consistent from frame to frame)

GranMinigun · 2024-03-31T10:08:51Z

I'll do a review later today.

GranMinigun

I see this is your first contribution. Welcome to the world of open-source development. I've left some detailed comments to hopefully help you get acclimatized quicker in case of future contributions to this or other projects.

I'm not super familiar with the code anymore.

Demand documentation before merging stuff from random people. :-P But yes, I'm not entirely happy with the loop myself. It's functional tho... or was...

GranMinigun · 2024-03-31T16:05:04Z

src/platform/core/src/device/ogl_video_device.cpp

@@ -202,6 +203,16 @@ void OGLVideoDevice::CreateShaderPrograms() {
    }
  }

+  //LCD grid pass
+  if(config->video.lcd_grid) {


You're pushing the shader pass at the end of the chain. This breaks certain assumptions in the current code regarding when each pass happens, which leads to artefacts when ghosting effect or xBRZ are enabled, and also renders bilinear sharp shader as regular bilinear.

Can you please describe the expected output of this shader, and expected interactions with other present shaders? Right now, my understanding is this should be used as a final pass with integer scaling enabled, although it theoretically can be overlaid on top of any output.

Also, please adjust formatting: add a space to the comment line, so it reads // LCD...; remove the empty line after if(success) {; and leave only one empty line before UpdateShaderUniforms(); call.

Hello, sorry for the late answer.
The objective of the shader is to draw borders on each of the "GBA pixels".
In theory it can be applied with any scaling, but the pixels may not match the grid.
The "best setup" in theory is integer scaling ON and nearest filter.
If I undestand the problem. It's needed to change the order of the shaders.
I tried to make "a correction" like in xbrz with ghosting, but it didn't work, xbrz is disabled and If I turn off lcd grid, the image is flipped.
This is harder than I imagined. hehehehe.

And thank you for all your help.

Honestly, I'd probably just slap it alongside the rest of output shaders and revert back to its original name (so lcd1x instead of "LCD grid"). By default, nearest interpolation is performed, so the user only has to turn on integer scaling. Then you don't have to worry about breaking anything.

Adding more passes will sooner or later necessitate a more advanced rendering loop, at which point it'll probably make sense to adopt RetroArch's Slang shader format and just leave passes configuration to the user, or something like that. I'd certainly prefer to leave the current simple and curated experience with a minimal set of shaders arranged in a specific way ~~because it's less work~~.

GranMinigun · 2024-03-31T16:16:37Z

src/platform/core/src/device/shader/lcd_grid.glsl.hpp

@@ -0,0 +1,50 @@
+/*
+ * Copyright (C) 2024 fleroviux


Who is the author of the shader? What is the licence of the shader? Can you please provide a link to the source from which you've ported the shader, so I could suggest how to properly write the header? Authorship and licensing shouldn't be arbitrarily changed, this can be illegal depending on the wording of the original licence. See, e.g., how xBRZ and sharp bilinear shader headers look like.

https://github.com/libretro/glsl-shaders/blob/master/handheld/shaders/lcd1x.glsl
https://github.com/jdgleaver/drastic_ds_shaders/blob/master/lcd1x.dsd
I based the port in these sources.
I already had a header similar to drastic, but it seems I forgot to commit.

Yeah, copying either one (and, at your option, adding a line that you've ported it) should suffice.

(I don't understand why it was necessary to apply GPLv2+ to the public domain code, but oh well.)

GranMinigun · 2024-03-31T16:19:56Z

src/platform/core/src/device/shader/lcd_grid.glsl.hpp

+
+#pragma once
+
+#include "device/shader/common.glsl.hpp"


This header contains common vertex shader sources for shaders that do not perform anything special at the vertex stage. You're not using them here (assigning a custom shader source to lcd_grid_vert), so this include should be removed.

fleroviux · 2024-03-31T17:19:02Z

If this source is to be trusted, the original code is by Gigaherz and public domain. Nonetheless it would indeed be nice to add attribution in the header comment at least.

Demand documentation before merging stuff from random people. :-P But yes, I'm not entirely happy with the loop myself.

Very fair ^^ But also my comment wasn't meant to criticize your changes, in case it came off that way. It's more that I got confused by the vertical UV flip and why that is necessary for final output only, before I even got to look at the xBRZ issue and I figured your input on this PR would be valuable anyways. Doesn't help when I've last touched the relevant code two years ago myself.

GranMinigun · 2024-04-03T16:45:07Z

But also my comment wasn't meant to criticize your changes, in case it came off that way.

Don't worry about it.

I got confused by the vertical UV flip and why that is necessary for final output only

I mean, if GBA and OpenGL assume different origin points, then it has to happen somewhere. Right now the flip is performed in the output pass because it's the only one that's guaranteed to be present, but it's possible to do that before the post-processing chain (either by writing scanlines in reverse, or as a separate shader program, possibly even from raw GBA framebuffer and palettes).

The bug you see here is due to LCD grid pass happening after the output pass, which leads to copying the wrong frame for the ghosting effect. As for xBRZ, it seems to be reading hell knows what for the same reason.

fleroviux · 2024-04-03T18:34:06Z

I mean, if GBA and OpenGL assume different origin points, then it has to happen somewhere.

Yeah, the bit that was missing on me was that not only UVs are considered bottom-left origin in OpenGL, but also glTexImage2D and friends expect the bottom-most scanline to be at the start of the buffer when I thought the top-most scanline would go first.

but it's possible to do that before the post-processing chain (either by writing scanlines in reverse, or as a separate shader program, possibly even from raw GBA framebuffer and palettes).

Writing scanlines in reverse seems reasonable, maybe with an option to let the video backend decend whether the core should flip the scanlines or not.

DestinyXZ9 · 2024-04-10T05:41:21Z

Ready, all the necessary corrections for the lcd1x shader have been made. Thanks for all your help. @fleroviux @GranMinigun

GranMinigun

Almost there, just some formatting nits (which probably should be handled by clang-format... something to look into?).

Do you know how to perform rebases and edit and squash commits? If not, what tools are you using? Plain Git through terminal, or some sort of editor?

GranMinigun · 2024-04-10T07:41:15Z

src/platform/core/src/device/ogl_video_device.cpp

+    case Video::Filter::Lcd1x:{
+      auto [success, program] = CompileProgram(lcd1x_vert, lcd1x_frag);
+      if (success) {


Suggested change

case Video::Filter::Lcd1x:{

auto [success, program] = CompileProgram(lcd1x_vert, lcd1x_frag);

if (success) {

case Video::Filter::Lcd1x: {

auto [success, program] = CompileProgram(lcd1x_vert, lcd1x_frag);

if(success) {

The space after if above was my mistake that I noticed only now (oops), you can fix that too.

I am using visual studio code to modify the code and plain git through terminal. I don't know how to perform rebases and edit and squash commits. I made the corrections and then i just made an add .
commit -m
and git push.

GranMinigun · 2024-04-10T07:43:28Z

src/platform/core/src/config.cpp

+        { "xbrz",    Video::Filter::xBRZ    },
+        { "lcd1x",    Video::Filter::Lcd1x    },
+


Please adjust spaces inside brackets and remove trailing comma and newline:

Suggested change

{ "xbrz", Video::Filter::xBRZ },

{ "lcd1x", Video::Filter::Lcd1x },

{ "xbrz", Video::Filter::xBRZ },

{ "lcd1x", Video::Filter::Lcd1x }

GranMinigun · 2024-04-10T07:45:17Z

src/platform/core/src/device/ogl_video_device.cpp

@@ -365,4 +374,4 @@ void OGLVideoDevice::Draw(u32* buffer) {
  }
 }

-} // namespace nba
+} // namespace nba


I see you're a Windows user. /s

Sources should have a newline at the end, this change should be omitted.

GranMinigun · 2024-04-10T07:46:27Z

src/platform/core/src/device/shader/lcd1x.glsl.hpp

+ *
+ * Original code by Gigaherz, released into the public domain
+ *
+ * Ported to Nano Boy Advance format by Destiny


NanoBoyAdvance, no spaces.

GranMinigun · 2024-04-10T07:56:26Z

src/platform/qt/src/widget/main_window.cpp

@@ -177,7 +177,8 @@ void MainWindow::CreateVideoMenu(QMenu* parent) {
    { "Nearest", nba::PlatformConfig::Video::Filter::Nearest },
    { "Linear",  nba::PlatformConfig::Video::Filter::Linear  },
    { "Sharp",   nba::PlatformConfig::Video::Filter::Sharp   },
-    { "xBRZ",    nba::PlatformConfig::Video::Filter::xBRZ    }
+    { "xBRZ",    nba::PlatformConfig::Video::Filter::xBRZ    },
+    { "Lcd1x",    nba::PlatformConfig::Video::Filter::Lcd1x    }


Spaces:

Suggested change

{ "Lcd1x", nba::PlatformConfig::Video::Filter::Lcd1x }

{ "Lcd1x", nba::PlatformConfig::Video::Filter::Lcd1x }

fleroviux · 2024-04-10T08:48:37Z

Almost there, just some formatting nits (which probably should be handled by clang-format... something to look into?).

Do you know how to perform rebases and edit and squash commits? If not, what tools are you using? Plain Git through terminal, or some sort of editor?

@GranMinigun I am not a huge fan of auto-formatters, because sometimes I think it's better for readability to make case-by-case decisions. But I see the usability for working with multiple contributors, so if we can fit the current code style well enough into a clang-format preset, then I'd be open to it. I guess the very least would be to document the current code standard.

GranMinigun · 2024-04-10T09:35:48Z

Formatters don't prevent you from making manual adjustments, and clang-format can work with just the modified lines, so pretty columns aren't going anywhere. It is pretty flexible, although I'm not sure it allows to configure lack of space after keywords like if (by the way, what's up with that? If it's for uniformity with CMake, then if() there is a command/"function", unlike in C++). Edit: yes, that's possible.

fleroviux · 2024-04-10T09:50:14Z

(by the way, what's up with that? If it's for uniformity with CMake, then if() there is a command/"function", unlike in C++).

There's no rationale behind it. It's just a personal preference of mine.

GranMinigun · 2024-04-12T12:39:25Z

IMPORTANT: I did the whole write without realising that the editor invoked by Git for editing commits might be something else than GNU nano in your case. I assume it's MSYS2 environment, so the instructions should apply, but if at any moment you're not sure what's going on - please tell so. nano is keyboard-driven, controls are listed at the bottom of its window, the important ones are Ctrl+O to initiate saving, and Ctrl+X to exit (listed as ^O and ^X, respectively; M- stands for Meta, or Alt). Also, Git's log and diff commands can be navigated with arrow keys and Page Up/Down, Q to Quit when you're done with reviewing. Again, when in doubt - ask.

Sorry it's taking so long for relatively simple changes. If all goes well, this should be the final round. Wall of text incoming.

I've noticed that, while you did add loading of lcd1x setting, you missed the saving part further down the file. I've prepared a patch that addresses this issue (and also makes final formatting adjustments, just so I don't feel terrible for torturing you any more with that). Download and extract the patch, then execute: git apply path/to/file.patch.

formatting.zip

What this does is applies the changes from the file as if you've done them yourself. You can execute git diff and compare the output with contents of the patch file, it's plain text. It'll show that lcd1x.glsl.hpp was replaced entirely. That's because I've changed line endings format from Windows-style CRLF to UNIX-style LF, which is historically the preferred format for sources.

Now, instead of creating a new commit, you can make these changes part of the last commit. To do so, stage the changes as usual via git add command, then type git commit --amend. This will let you change the commit message, if you want, and then will apply the changes on top of whatever the commit already had. Verify with git diff HEAD^, this will show what changes the last commit contains.

Rebasing. The thing that's important to understand about Git is that commits, in essence, are just a series of patches applied on top of each other. What rebasing does is takes a range of commits and moves them in history, changing what's their parent commit (base) is. Doing that is usually needed when the branch you want to merge your changes to has advanced forward after you've started working on your branch - which is exactly the case here.

First, pull the latest changes from the master branch, then switch back to your branch:

git checkout master
git pull
git checkout shaders_branch

We'll do something slightly different, but just so you know: if all you need is to update your branch, then most of the time executing git rebase target_branch is enough. Sometimes you might need to resolve conflicts, if the history contains changes around the same lines that you've also edited. Git will tell you where the conflicts are, and mark them inside the files. All you need to do in that case is look for those markers (<<<<<<<, =======, and >>>>>>>), edit those segments, stage and verify the changes (git add, git diff --staged), then go on with the rebase until it is done (git rebase --continue). If you ever feel like you're not sure what you're doing, then the rebase can be aborted by typing git rebase --abort. Better ask for help then.

Fortunately, your branch can be rebased cleanly, so no conflict resolution is needed. But we need to squash the changes from multiple commits into a single one, so that the history looks nice and clean, is easier to review and bisect, and just takes up less space. Meet interactive rebase: git rebase -i master.

You'll be greeted with a list of commits, with commands and hashes prepended to them, and a description of what commands do below. The commits are ordered from the earliest on top to the latest on bottom. You can change the commands, reorder the commits, edit their messages, or even outright remove them. We'll just change the commands, from pick (which takes and applies the commit as is):

pick 77626b46 LCD grid shader based on lcd1x
pick dd8a6f3e Include lcd1x shader on ogl_video_device
pick 2d700a05 Correction
pick 667afd2e format corrections

To reword for the first one and fixup for the rest, like so (using shorthands here):

r 77626b46 LCD grid shader based on lcd1x
f dd8a6f3e Include lcd1x shader on ogl_video_device
f 2d700a05 Correction
f 667afd2e format corrections

fixup command merges with the previous commit, similar to amend, while reword lets you adjust commit message. For the commit message, change it to: Platform: Core: Add lcd1x shader

Verify that everything is correct one more time: git log should show your now single commit on top of master, git diff HEAD^ should contain all the changes you intended to make. All good? Then, finally, rewrite the history in the remote GitHub repository: git push -f your_remote_name (or, if you do pushes without specifying a remote, just git push -f), - careful with that though, there's no way back after a force push.

DestinyXZ9 · 2024-04-18T06:21:55Z

It's not a problem, I am learning a lot of git. I think that I did all the steps correctly, but I didn't notice when the spaces in lcd1x file appeared again. I need to correct this problem and then rebase again?

GranMinigun · 2024-04-18T09:48:52Z

No need for changes. When I said "no spaces", I only meant that NanoBoyAdvance is written in one word. The changes came from the patch file you've applied.

But yes, because there was another commit added, you now need to synchronize your fork (GitHub will offer you to do so when you pick master branch), then pull, do a simple rebase (without -i), force push.

(Not all PRs you work on need to be squashed, by the way. Keeping isolated changes in multiple commits is a good idea, for example. But in this specific case, it was better to consolidate everything in a single commit.)

Tested on my side, all looks good. Will give a check mark after the rebase. (And I have to think about the renderer...)

fleroviux · 2024-04-18T15:12:08Z

(And I have to think about the renderer...)

@GranMinigun What do you think about adopting something like librashader? I haven't looked deeply at it myself yet, but I think it got adopted by ares recently.

GranMinigun · 2024-04-18T15:50:07Z

It's huge, almost 20 MiB on my system. No opinion otherwise, as I didn't have any experience with it. With a UI to let users customize their shader chain, this or a small custom solution similar to raslafx - maybe?

...Although, that might affect adoption of sRGB -> Linear -> sRGB pipeline. Ugh.

fleroviux · 2024-04-18T21:08:54Z

It's huge, almost 20 MiB on my system.

Oh wow, that's huge. I'd guess a lot of that is because it support a lot of different backends like DX11, DX12, Vulkan, WebGPU, Metal, OpenGL...

I think for our usecase we'd probably only need OpenGL 3.3 core and maybe a Metal backend. Metal because OpenGL is deprecated on macOS. But that would likely require shader translation, or shaders that are written to compile both as GLSL and MSL (maybe with preprocessor magic). I'm not biased towards a third-party or custom solution.

fleroviux · 2024-04-20T20:19:04Z

LGTM! Thanks @DestinyXZ9 and @GranMinigun.

DestinyXZ9 · 2024-05-03T21:22:02Z

Thank you @fleroviux and @GranMinigun for all your help and patience.

GranMinigun suggested changes Mar 31, 2024

View reviewed changes

GranMinigun suggested changes Apr 10, 2024

View reviewed changes

DestinyXZ9 force-pushed the shaders_branch branch from 667afd2 to a9de5e6 Compare April 18, 2024 06:14

Platform: Core: Add lcd1x shader

c42da68

DestinyXZ9 force-pushed the shaders_branch branch from a9de5e6 to c42da68 Compare April 19, 2024 03:34

GranMinigun approved these changes Apr 19, 2024

View reviewed changes

fleroviux merged commit 60fdedc into nba-emu:master Apr 20, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LCD grid shader #369

LCD grid shader #369

DestinyXZ9 commented Mar 27, 2024

fleroviux commented Mar 30, 2024 •

edited

Loading

GranMinigun commented Mar 31, 2024

GranMinigun left a comment

GranMinigun Mar 31, 2024

DestinyXZ9 Apr 5, 2024

DestinyXZ9 Apr 5, 2024

GranMinigun Apr 7, 2024

GranMinigun Mar 31, 2024

DestinyXZ9 Apr 5, 2024

GranMinigun Apr 7, 2024

GranMinigun Mar 31, 2024

fleroviux commented Mar 31, 2024 •

edited

Loading

GranMinigun commented Apr 3, 2024

fleroviux commented Apr 3, 2024

DestinyXZ9 commented Apr 10, 2024

GranMinigun left a comment

GranMinigun Apr 10, 2024

DestinyXZ9 Apr 12, 2024

GranMinigun Apr 10, 2024

GranMinigun Apr 10, 2024

GranMinigun Apr 10, 2024

GranMinigun Apr 10, 2024

fleroviux commented Apr 10, 2024

GranMinigun commented Apr 10, 2024 •

edited

Loading

fleroviux commented Apr 10, 2024

GranMinigun commented Apr 12, 2024 •

edited

Loading

DestinyXZ9 commented Apr 18, 2024

GranMinigun commented Apr 18, 2024

fleroviux commented Apr 18, 2024 •

edited

Loading

GranMinigun commented Apr 18, 2024

fleroviux commented Apr 18, 2024 •

edited

Loading

fleroviux commented Apr 20, 2024

DestinyXZ9 commented May 3, 2024

		{ "xbrz", Video::Filter::xBRZ },
		{ "lcd1x", Video::Filter::Lcd1x },

	{ "Lcd1x", nba::PlatformConfig::Video::Filter::Lcd1x }
	{ "Lcd1x", nba::PlatformConfig::Video::Filter::Lcd1x }


		#pragma once

		#include "device/shader/common.glsl.hpp"

LCD grid shader #369

LCD grid shader #369

Conversation

DestinyXZ9 commented Mar 27, 2024

fleroviux commented Mar 30, 2024 • edited Loading

GranMinigun commented Mar 31, 2024

GranMinigun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fleroviux commented Mar 31, 2024 • edited Loading

GranMinigun commented Apr 3, 2024

fleroviux commented Apr 3, 2024

DestinyXZ9 commented Apr 10, 2024

GranMinigun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fleroviux commented Apr 10, 2024

GranMinigun commented Apr 10, 2024 • edited Loading

fleroviux commented Apr 10, 2024

GranMinigun commented Apr 12, 2024 • edited Loading

DestinyXZ9 commented Apr 18, 2024

GranMinigun commented Apr 18, 2024

fleroviux commented Apr 18, 2024 • edited Loading

GranMinigun commented Apr 18, 2024

fleroviux commented Apr 18, 2024 • edited Loading

fleroviux commented Apr 20, 2024

DestinyXZ9 commented May 3, 2024

fleroviux commented Mar 30, 2024 •

edited

Loading

fleroviux commented Mar 31, 2024 •

edited

Loading

GranMinigun commented Apr 10, 2024 •

edited

Loading

GranMinigun commented Apr 12, 2024 •

edited

Loading

fleroviux commented Apr 18, 2024 •

edited

Loading

fleroviux commented Apr 18, 2024 •

edited

Loading