antonioginer / switchres

Modeline generation engine for emulation
44 stars 12 forks source link

Allow reloading of settings from ini and allow absolute path to ini file #88

Closed Kopert closed 3 years ago

Kopert commented 3 years ago

I'm currently working on a feature for RetroArch that allows for core-specific switchres.ini overrides. This new functionality will work by reloading the base switchres.ini file (to undo any overrides from a previous core) and then loading the values from the core-specific switchres.ini file on top of it, replacing the previous values.

Currently it is not possible/stable to deinit/reinit SwitchRes inside RetroArch once it is running, but it is simple to just change the settings as they take effect on the following resolution changes. On my tests I was able to have a core-specific user_mode that got reverted as soon as the content was closed.

This pull request is relative to changes to the SwitchRes library that would be necessary to make the rest of the system work on RetroArch. They are as follows:

If this pull request is accepted then I can pull the changes to the SwitchRes library into a fork of the https://github.com/alphanu1/MME_SR2 repository and make a pull request over there with the additional functionality to complete the new feature.

antonioginer commented 3 years ago

Hi Kopert, thanks for contributing.

I think your loop would be easier to read in this format (not tested):

    for (auto &display : swr->displays)
    {
        display->m_ds = swr->ds;
        display->parse_options();
    }

Since this feature, basically refreshing the display settings struct after display initialization, wasn't considered in the design, I can't forsee if there will be any conflicts or unexpected behaviour. Hopefully there isn't.

The use case where this will be problematic for sure is with more than one display when different settings are required by each one. Anyway, the wrapper itself isn't ready yet for this case. So I think it'd be better to simply target the current, unique, display:

MODULE_API void sr_load_ini(char* config) {
    swr->parse_config(config);
    swr->display()->m_ds = swr->ds;
    swr->display()->parse_options();
}

At least until we figure out the way the api should deal with multiple display instances.

Kopert commented 3 years ago

Hey @antonioginer thanks for reviewing my changes.

  for (auto &display : swr->displays)

This was my initial implementation of it, but since this code has to be merged into RetroArch's and they have a requirement for C89 compatibility, I decided to play it safe.

MODULE_API void sr_load_ini(char* config) {
  swr->parse_config(config);
  swr->display()->m_ds = swr->ds;
  swr->display()->parse_options();
}

I have pushed this change after confirming that it is still working as before.

As for your multi-display concerns, I don't have any suggestions on how to best handle that. I would assume that overrides should affect all displays managed by SwitchRes, but it would certainly be safer to just affect the current main display for now, specially since it is a change that should affect the following resolution changes that will happen on it.

Let me know if you need any other adjustments or testing done.

antonioginer commented 3 years ago

Probably we'll handle multi display by adding an api function that sets the target display for subsequent operations. This way we won't need to pass a display index in each api call, and we can isolate each display with its own different settings.