ramapcsx2 / gbs-control

GNU General Public License v3.0
793 stars 111 forks source link

Fix output resolution shown after loading and saving custom presets #438

Closed nyanpasu64 closed 1 year ago

nyanpasu64 commented 1 year ago

(This writeup may not be 100% accurate. I'm creating this PR weeks after my initial commit, and discovered numerous errors in my original commit message, so I had to reword those messages. There may be errors in this writeup as well.)

Initialize GBS_PRESET_CUSTOM in setOutModeHdBypass

I found that GBS_PRESET_CUSTOM was left uninitialized (keeps old value) when switching into bypass mode. This means that switching from a fixed resolution to Bypass left it at 0, and switching from a custom preset to Bypass left it at 1. This may or may not affect auto gain behavior; I have not checked, but this inconsistent behavior is worth fixing anyway.

To avoid issues, make setOutModeHdBypass() set GBS_PRESET_CUSTOM to 0. But don't do this if applyPresets() is loading a custom preset, has set GBS_PRESET_CUSTOM to 1, and is calling setOutModeHdBypass(regsInitialized=true) to apply the saved "bypass" preset.

(This really belongs in a separate PR, but I got a merge conflict trying to remove this commit from this PR, and I'm worried about introducing new bugs when disentangling the code.)

Fix output resolution shown after loading and saving custom presets

Change the code to set rto->isCustomPreset = GBS_PRESET_CUSTOM::read(), and not set rto->presetID to 9.

Why was this done? Loading a custom preset writes the "effective preset ID" to the GBS_PRESET_ID register. Previously it would set rto->presetID to 9 rather than GBS_PRESET_ID, so the web UI (updateWebSocketData()) can highlight the "load preset" button to indicate a preset was loaded.

Saving the preset again (in the HTTP "/slot/save" handler) writes rto->presetID (=9) to SlotMeta::presetID. This causes the web UI to show the preset slot's output resolution as "Custom", which is not useful.

Fix

We cannot make "/slot/save" read the GBS_PRESET_ID register rather than rto->presetID, because reading registers over I2C requires calling yield(), and the current (terrible) AsyncWebServer does not allow calling yield() from HTTP callbacks.

Instead we make rto->presetID always reflect GBS_PRESET_ID (and never be set to 9). "/slot/save" reads only rto->presetID when writing the resolution metadata of a saved preset.

How do we fix the web UI? We cannot make updateWebSocketData() highlight "load preset" if uopt->presetPreference == OutputCustomized. This is because clicking a custom slot in the web UI (HTTP /slot/set) sets uopt->presetPreference = OutputCustomized, but doesn't call applyPresets() until you click "load preset" (user command '3'). Prior to actually loading the custom preset, if we're on a fixed resolution and GBS_PRESET_CUSTOM is 0, we need to highlight the fixed resolution's button (and ignore uopt->presetPreference == OutputCustomized).

To fix the web UI, we create a new variable rto->isCustomPreset mirroring GBS_PRESET_CUSTOM. Then updateWebSocketData() reads rto->isCustomPreset to decide whether to highlight "load preset" in the web UI.

Future work

In the future, we could also change the WebSockets protocol to independently transmit the output resolution (and whether it's customized), and the current preset slot if uopt->presetPreference == OutputCustomized.

If different input resolutions are mapped to different output resolutions (for example mapping 480i input to 480p output, and 480p input to Bypass), the web UI still only shows the output resolution for the most recently saved input resolution, not the current one. But this is a smaller problem than before.