kbeckmann / game-and-watch-retro-go

Emulator collection for Nintendo® Game & Watch™
GNU General Public License v2.0
440 stars 133 forks source link

Remember the last selected game in the menu after reset #101

Closed kbeckmann closed 3 years ago

kbeckmann commented 3 years ago

Store in the config area

rzinurov commented 3 years ago

As far as I can tell, in the code there was an intention to remember the last selected item on the tab, and the cursor position is stored under str_buffer key in odroid settings. I've tried debugging the code to the best of my skills and all I could figure out was that the value of str_buffer is different when odroid settings are being saved right before the game is launched and when the tab inits after I exit the game. Since the key of the cursor mismatches it always falls back to zero. The cursor value persists when switching between the tabs in the menu, but it's being read incorrectly when launching a game and exiting it.

str_buffer value before the game starts:

Screenshot 2021-07-11 at 23 13 52

str_buffer value after the game exits:

Screenshot 2021-07-11 at 23 14 12

As you can see in the value, the value was saved for nes but the init tab value was read for gb. I'll try debugging this further.

kbeckmann commented 3 years ago

The reason it doesn't have any effect is because of the stub implementations of odroid_settings_int32_get / odroid_settings_int32_set.

The reason they aren't properly implemented is because i didn't want to bloat the config struct with dynamic data.

I say we add two fields in persistent_config_t in https://github.com/kbeckmann/game-and-watch-retro-go/blob/main/Core/Src/porting/odroid_settings.c

To keep things simple, we can have a uint8_t array of size 4 with the tab name (this comes from the extensions in Core/Src/retro-go/gui.c, so gb, nes etc.

The second field can be a uint16_t with the cursor.

kbeckmann commented 3 years ago

You'll also have to expose the get/set methods for these two fields. You can do something similar to:

uint16_t odroid_settings_MainMenuTimeoutS_get()
{
    return persistent_config_ram.main_menu_timeout_s;
}
void odroid_settings_MainMenuTimeoutS_set(uint16_t value)
{
    persistent_config_ram.main_menu_timeout_s = value;
}

Don't forget to update the header file too (it will build even if you forget).

rzinurov commented 3 years ago

@kbeckmann oh, that explains it 😄 Did you have any specific issues with odroid_settings_int32 get/set or are you mostly concerned by the memory footprint? I don't have any problem with custom attributes in settings, but I was wondering whether supporting dynamic data in the config will lead to easier maintenance and readability of the code. For example, the issue with resetting color palette https://github.com/kbeckmann/game-and-watch-retro-go/issues/102 might also be caused by this, because it seems to be working for gb emulator (first "tab"), but it resets for the nes (second tab), and if that's the case we'd need another custom attribute for it.

kbeckmann commented 3 years ago

The main issue is that we don't have a file system. That means that we need to allocate space statically. Having a struct with fixed entries makes this easy.

We could e.g. reserve 4096 bytes for a json-blob and have dynamic data, but then it might or might not be enough etc.

rzinurov commented 3 years ago

Can we consider "whitelisting" keys and validating values of dynamic attributes? I can put together a list of all commonly used key/value pairs if you'd like.

kbeckmann commented 3 years ago

Yeah that'll work. Then the API will stay the same and we can reuse the old code, nice.

kbeckmann commented 3 years ago

Another middle-ground is otherwise to have an enum for the allowed variables. That way we won't have to store the strings and do strcmp and so on. We could then use the enum as an index to an array of integers.

typedef enum {
  CONFIG_KEY_A = 0,
  CONFIG_KEY_B,
  CONFIG_KEY_C,
  CONFIG_KEY_COUNT
} config_key_t;

typedef struct persistent_config {
  ...
  int32_t values[CONFIG_KEY_COUNT];
} persistent_config_t;

int32_t odroid_settings_int32_get(config_key_t key, int32_t default_value)
{
  if (key < CONFIG_KEY_COUNT) {
    return values[key];
  } else {
    return default_value;
  }
}
kbeckmann commented 3 years ago

Urgh, now that I think about it, the default_value has to be handled in a different way or scrapped completely.

rzinurov commented 3 years ago

I couldn't get anywhere with the dynamic approach, but I think I've managed to get it done with two static variables: https://github.com/kbeckmann/game-and-watch-retro-go/pull/108