Shader Core/Game Presets are being saved as default

[ghost]

First and foremost consider this:

• Only RetroArch bugs should be filed here. Not core bugs or game bugs
• This is not a forum or a help section, this is strictly developer oriented

Description

When loading a core with shader core/game presets saved, after closing this core the preset is being saves as default in retroarch.cfg "video_shader =". This causes issue when loading another core that does not have its own shader presets as its very possible the shaders presets will not be compatible.

Expected behavior

When loading core with its own presets, this should not be saved in retroarch as general or main shader preset.

Actual behavior

Last loaded shader preset(or the 1st loaded in my test) is being saved as default shader in retroarch.cfg. clearing up video_shader ="" does not help coz it gets filled the next time a shader preset is loaded. already cleared and/or deleted shaders/retroarch.glslp and it still behaves the same.

Only tested with glsl atm.

Steps to reproduce the bug

1. Load core, load some shaders and save core or game preset.
2. exit core. exit retroarch. 2a. remove shaders/retroarch.gslsp (or equivalent if using cg shader) 2b. edit retroarch.cfg and clean video_shader = ""
3. run RA, load the core with presets. normally this should load its own preset.
4. exit core. launch another core without its own preset.
5. new core would take the last preset used-which is if incompatible is disastrous 💃

Bisect Results

[Try to bisect and tell us when this started happening]

Version/Commit

You can find this information under Information/System Information

• RetroArch: 1.6.7 git 35c4770c9

Environment information

• OS: Solus 3 Kernel 4.12
• Compiler: GCC 6.4.0 / clang 4.0.1

[andres-asm]

Yeah this has been a bug forever, basically retroarch uses the last configured shader "current shader storage"

I haven't found a way to fix this.

[sergiobenrocha2]

See #3097

[Oibaf66]

It would be sufficient to make the code not to save and load the last configured shader.

I don't see any reason to apply a global shader configuration in retroarch.

If you want retroarch to automatically load a shader configuration, you can use the shader saving function for cores and games.

[orbea]

@retro-wertz Was this fixed?

https://github.com/libretro/RetroArch/pull/6779#issuecomment-388697899

[LazyBumHorse]

This is fixed, yes, but it's still pretty annoying.

While loading a preset does not set the video_shader setting, pressing "apply" or "load shader preset" will. So whenever you want to tweak your existing presets, you end up resetting the global shader setting as well!

I think we should really remove the video_shader setting altogether* and instead add an explicit "save as global \<current shader language> shader" option. It can write to retroarch.<cg/glsl/slang>p like it already does and then use that as a fallback preset automatically.**

I can't tell you how many times the global shader setting was set when I didn't want it to. I ended up having to add presets for every single core, some of them simply to disable the shaders via manually setting shaders = "0".

* The setting isn't well thought out anyways, because it only saves a default preset for one shader language, so if you use different video drivers, you're basically out of luck.

* One issue with that is that since these `retroarch.pwere already written at some point, people might be confused why they suddenly got some default shaders and might not know how to disable them, so maybe it's not the worst idea to rename them to something likedefault.*p` - and then having people confused because their default shader is gone :) This also ties into another problem: I believe there's currently no way to remove presets from the menu (is there?), which might be a bigger problem for systems like android where you cannot access the shader directory. I'll put "allow saving presets with 0 passes" on my TODO to combat this.

[hizzlekizzle]

ok, so your proposal is:

• [ ] remove video_shader = "" config switch. (I actually use this setting with --append-config on one of my setups, but I'm okay with seeing it go)

• [ ] rename retroarch.p to default.p to make its purpose more intuitive. (would the term 'global' make more sense?). With video_shader gone, where will this load from (i.e., where will it look for default.*p)?

• [ ] Have an explicit "save default/global shader" option alongside the existing core/game options.

• [ ] Allow saving 0-pass presets (would this include a "remove global/core/game shader preset" option that does essentially this?)

• [ ] Add a check for other shader languages when saving presets?

I think we could simplify the menu quite a bit to just have "save preset" with a "pull-down menu" for global, core, game, and 'save as...'. And the same thing for "remove preset".

On the topic of streamlining the shader menu: would it make sense to hide the individual passes behind the "advanced options" toggle? That is, only expose the presets by default (and possibly show the name of the current-loaded preset somewhere)?

[LazyBumHorse]

[1] Ah, I didn't think about it being used as a parameter via --append-config. Instead of setting video_shader = "", could you not effectively replace that with video_shader_enable = false in your case? Or would that then also be saved to the default config? Not sure how --append-config works.

I was thinking a bit about reporpusing the setting (e.g. only reading, never writing), but there are a lot of drawbacks and ultimately it made me think that a direct command-line option to set a global shader override (ignoring any preset) would be much better.

[2] I think "global" is an even better name! Currently the default retroarch.*p loads from the base shader directory, we could put the renamed version into the "presets" subfolder were all the core/folder/game presets go so it's a bit more consistent.

[4] There's a difference between removing a preset and creating one with 0 passes. So say you have a global preset but you create a 0 pass preset for one core, I think you see what I mean, the core preset negates the effect of the global one. (I believe the priority goes like this: global < core < folder < game). So ideally both should be an option and 0 pass should be trivial to patch in, I could probably have already created a pull request by the time I wrote this very sentence which I intentionally made a bit longer than necessary so you don't underestimate the time it takes to create a pull request IthinkyougettheideaIneedtostopnow.

[5] Add a check for other shader languages when saving presets?

What do you mean by that? I don't understand...

I think we could simplify the menu quite a bit to just have "save preset" with a "pull-down menu" for global, core, game, and 'save as...'. And the same thing for "remove preset".

Yeah, that sounds like a good idea. Would be nice if the remove options are grayed out if there's nothing to remove, if that is an option.

On the topic of streamlining the shader menu: would it make sense to hide the individual passes behind the "advanced options" toggle?

Oof, I don't think so, but I personally also dislike how many important settings get hidden by this toggle in general, so don't trust me on that. In my opinion, being able to edit passes and create your own presets is one of the main features of the shader menu, so it shouldn't get tucked away. The only thing I can definitely say is that "Apply Changes" needs to move directly to the shader passes are, because you need to press it regularly. Ideally "Apply Changes" would be automatic, but that's another feature request :)

[hizzlekizzle]

1.) I use it to have a shader of a specific type loaded at launch because it sometimes gets weird with context stuff using Cg/GLSL via GL driver. Having a CLI option to load a shader would completely replace it, and it's a pretty common request, too.

2.) sounds good

4.) ah, yeah, i see what you mean

5.) this is really only a problem with Cg and GL. If you save a core/game preset in Cg and then save a GLSL one, it'll always load whichever one takes precedence (used to be Cg, might be GLSL now?) and there's no way to get rid of the old, conflicting one (that is, saving a new core/game preset won't overwrite it and there's currently no way to delete them without doing it in the filesystem manually). Personally, I think we should stop supporting Cg on GL except on PS3, as it's high time we got away from that dead format.

While I agree that tinkering with shader passes is an important use for the menu, I think the majority of people just load presets and then piddle around with parameters.

Re: advanced options, yeah, the first thing I usually do with an installation is 'show advanced options' ON, but it's been an ongoing complaint from users that we show them too much by default :/

[LazyBumHorse]

Number 5 is a hard one which I'm still thinking about for #8910.

It currently goes through cpg, glslp and slangp in order and takes the first it finds. There's multiple problems with that.

1. If we use vulkan for example, there might be a glslp which will then be loaded by the video driver's init(), which then fails because the type is not supported when there was a slangp all along!

2. The problem you described. Basically being "stuck" on one shader language when multiple are supported.

Number 1 can be solved with a proper compatibility check and possibly improved priorization (working on it, just slooowly). Number 2 is exactly caused by any kind of automatic priorization, because there are 2 options, you want 1, but the system says "nope". Having a way to remove presets for different types would at least give some control over this.

A more general solution might be to be able to, somehow, magically, and manually define priorization from the menu, more generally for each core and most generally for each combination of core and video driver, simple :)

[LazyBumHorse]

The insane priorization idea could actually be done by saving the type history for each preset into a file. E.g. you save a Snes9x.cgp core preset, then you save a Snes9x.glslp core preset, now there's a Snes9x.p file (or whatever extension) which contains "cgp" and "glslp" in the next line, so you know the last choice was glslp, so whenever you have multiple choices for a compatible shader preset, you go through the candidates in reverse order.

Not saying it's a good idea though.

[hizzlekizzle]

A more general solution might be to be able to, somehow, magically, and manually define priorization from the menu, more generally for each core and most generally for each combination of core and video driver, simple :)

Since every video driver -> shader language is 1:1 except GL (i.e., Cg and GLSL), I would think we can make it prioritize the current driver's shader type as long as we're willing to make a choice on Cg being demoted.

[ghost]

At some point we'll be adding HLSL to the d3d drivers as well. So they would have that as well as slang.

[LazyBumHorse]

--setshader or --set-shader?

I got the CLI option working yesterday, though I got lost in the rabbit hole that is configuration.c. (It's apparently hard to control which settings get read and written, then there's also overrides and it doesn't seem safe to reuse the video_shader setting even just internally, so instead of dealing with that I'll probably just put it into global_get_ptr().)

So the video_shader setting is removed, but removing/refactoring the internal settings->paths.path_shader is a bit more work. There's this bit where it's used as a pass-through from menu_shader.c to retroarch.c: https://github.com/libretro/RetroArch/blob/b69ab4b178acf705005c6985c9edc6c2c38f062f/menu/menu_shader.c#L163-L167 https://github.com/libretro/RetroArch/blob/b69ab4b178acf705005c6985c9edc6c2c38f062f/retroarch.c#L2859-L2870 What this does is remembering the shader preset you manually applied and loading that the next time you initialize a core (even if it's a different one) in the case when there is no core/folder/game preset. So it's basically a temporary global preset with lowest priority, only reset by restarting RetroArch.

I think one of the main reason this was done is so that the shader is kept when switching between windowed and fullscreen mode. But the shader also carries over to other cores, compatible or not. There's also no way to tell RetroArch to stop remembering that shader, so if you don't have a preset in a core/folder/game, you are stuck with the one you chose.

This should probably be changed so that when the content closes, the shader is cleared. edit Actually, it should also have the highest priority, so that switching fullscreen/windowed doesn't cause it to reset to a preset.

[hizzlekizzle]

--set-shader gets my vote