Create a settings window

[Baekalfen]

It would be nice to have a settings window to change keybindings, ROM directory, screen recording settings etc.

[krs013]

I just want to jot down some thoughts on this...

I see a setting objects in PyBoy acting as both a namespace/object where all configurable variables are stored, and also as a GUI for adjusting those variables. Since the internals are still under development and it would be bad form to tightly couple this one object to all of the various modules, the settings module should have minimal hardcoded awareness of the values it's storing. Instead, on startup, modules will hook into the settings module and instantiate their variables (in their subdivided namespaces), inputting a default value that comes from their source file and receiving an actual value that could come from one of several places with different priorities. From lowest to highest:

• Default values, set by the modules themselves on startup
• Stored values from a settings.conf or similar file
• Command-line arguments, also parsed in the settings module
• Object instance arguments, in case a single process has multiple pyboy instances for look-ahead or AI

The settings module has both global and instance-level behavior, so either all instances of the object have to have some global awareness, or a single global instance makes namespaces for different instances of pyboy that register with it. Either way, pyboyb's submodules will probably need something passed to them so they either know which settings instance is their own, or have a name or reference to the pyboy instance for looking up in a global version.

When registering their variables, modules should also leave a callback that can be used to update the values they've received (they keep local copies so they don't have to look it up constantly, but maybe this isn't necessary and they can look up every time). If the callback does not exist or raises an exception, the settings window can display some sort of warning that pyboy will need to restart to apply the changes.

Anyway, if I were to write it today, that's probably how I would go about it, but I want to finish sound first and this would be a lot of effort so I thought I'd throw this up for some discussion before committing to the design. Thoughts?

[krs013]

Okay, so @kr1tzy and I have been talking about how to approach this for a while, and it's been a very good discussion. I think we've worked out a plan for this that will work quite well for now and for the future. We wrote up our ideas together, which I'll paste in here. Hopefully it's clear enough that it gives a solid idea of how it works, and @Baekalfen perhaps you can ask questions or offer feedback if there's anything you don't feel sure about. After that, @kr1tzy can get started on implementing it when he's got time.

PyBoy Settings Specification

PyBoy’s configurable options (key bindings, window coloring, speed, etc.) are either hardcoded and left to recompilation or flow through several layers of constructors after being passed into PyBoy via CLI. There are many parts of PyBoy that we’ve wanted to configure, but every change requires edits in several places to accommodate new parameters. We’ve also wanted to add a settings window to allow for this configuration, but without changes to how settings are handled, it would increase the complexity and coupling of PyBoy even more.
This document describes a new model of how configurable values can be stored, loaded, and updated in a way that allows for more configuration and a settings window, but reduces the amount of work needed to add any particular feature. Instead of passing values as parameters through constructors, it stores them in a single object that allows modules to retrieve values from a config file, command line arguments, or PyBoy’s constructor parameters during initialization, and allows for the values to be changed in a settings GUI when appropriate.

Summary

• Add a settings module that provides a Settings object and a GUI
• Add support for a config file and a template in the repository
• Change modules in PyBoy so configurable values use the Settings object
• (optional) Create a TextGUI plugin/window to serve as the interface for the settings GUI and the memory window, so SDL2 code is separate. (This probably should be a separate feature!)

How It Works

• On startup, options from CLI arguments, config files, and the pyboy.PyBoy() constructor are assembled into a dict mapping setting names to values. At this time, the names are just strings, not linked to the modules.
• Modules in PyBoy with configurable values receive a reference to the settings object and register their settings in __init__(), providing the class __name__, variable name, a default value, and maybe some other information.
• The settings object looks up the name (name plus the variable name) in its dict. If a matching value is found, that value is returned, otherwise the provided default value is used. The Settings object keeps a registry of the Setting objects.
• If the Settings GUI is opened, all registered settings are displayed and can be changed. If a setting is changed that cannot be changed at runtime, the GUI offers to save the changes to the config file and restart PyBoy.

Implementation

• Most of the new code will be in a top level settings module in pyboy that contains...
  • The generic Setting class, which can be used for custom settings
  • Type-specific classes that extend Setting (IntSetting, EnumSetting, StringSetting, BoolSetting, FloatSetting)
  • A Settings class that provides methods for the modules to use for registering settings (Settings.int(), Settings.enum(), Settings.custom(), etc.)
  • A SettingsGUI class that opens a dialog and allows for these settings to all be viewed and manipulated
    • This can contain SDL2 code for now but we should make an issue asking for a window class for better decoupling for the future.
• In __init__, PyBoy will create and store a Settings object and pass it to any child objects that need it. This object will be used to create all the configurable values in the various modules in PyBoy. This must be done in the modules’ __init__ methods, not later.
  • The modules will provide a default value and an optional callback for updating the value. If the callback is not present, it implies a restart is required to change the value. There will also be arguments for providing help text and things like that.
• Since Settings are strings in CLI options and the config file, all Settings need methods to translate to and from strings.
  • The type-specific *Setting classes can provide these themselves, but custom Setting instances made with the generic class will need to provide these (e.g., a color_palette setting can read in the 12/16 bytes of hexadecimal values and print them back out, or handle whatever format we choose for that).
  • The methods that read in the strings should also do the range validation and raise an error if the value is invalid. These functions should also accept the final type as well without problems (they’re idempotent).
• After these values are set, they should not be changed except by the Settings GUI (or perhaps an API that the settings module can provide), which ensures that the callbacks are correctly used and changes are optionally saved to the config file.
  • If a setting has a value that the module itself should need to change, like the speed limit or window scale, then the module should treat the Setting as a default value, and have a separate variable for the runtime value, and a setter callback that updates both.
• The configuration file (“pyboy.conf” by default) will store any non-default values for these settings persistently and be loaded automatically.
  • PyBoy will search for a file with this name along a standard search path (e.g., current directory, $HOME, $HOME/.config, etc.) and can also have a path provided manually for specific situations in the PyBoy constructor and __main__ CLI args.
  • The .ini format will be used, and the Python standard library configparser module, but data will be read in as strings; type conversion will be left to the methods we write.
  • A default config “pyboy_defaults.conf” can be added to the repository, with comments describing how to use it. The settings module will need a special command to generate this file, and this could be made into a git commit hook.
• The names of the settings can be assembled by combining the module’s __name__ with the setting’s name; similar to how the loggers do it.
  • This enables names to only have to be unique per class. As an example, a respective value would be pyboy.mb.lcd.Renderer.limitsprites.
  • The .ini format can divide the entries into sections, so we should be able to have sections [pyboy.mb.lcd.Renderer] with simpler entries for tidiness.
• Settings can be set in __main__.py’s CLI arguments, but with the format -o pyboy.mb.lcd.Renderer.limitsprits=False so the argparser doesn’t have to get any fancy interpretation logic. Any -o options will be passed to the settings module as strings or (string, value) tuples and unpacked there.
  • Note that this will change the syntax for some existing arguments, but it will also declutter those options a lot. Arguments that only make sense for an individual runtime can stay as they are.
  • Since __main__.py invokes the PyBoy constructor, CLI -o arguments are basically the same as constructor arguments. The settings module does not need to have any awareness of argparser or sys.argv.
• All the values collected from the config file, command line arguments, and constructor arguments will be collected before they can be matched to actual Settings, since the names of these variables aren’t known until after the arguments are passed to PyBoy.
  • Constructor arguments (including command line arguments passed through the constructor by __main__.py) are given higher precedence than config file arguments, which can be discarded if there’s a name collision.
  • Constructor arguments are passed in a parameter called options, no **kwargs shenanigans. They can be either strings or typed data, as the parser functions must accept the typed data.
  • After PyBoy has finished initializing all of its modules, it will call settings.finalize() or something like that, which will check that all provided options have been mapped to Settings, and print warnings for anything left over. After this, any attempts to create new settings objects will result in an error.

[Baekalfen]

Great work both of you! Looks like a solid foundation for the task.

Modules in PyBoy with configurable values receive a reference to the settings object

Is settings a singleton object, that lives for each process? Wouldn't it be easier to simple import said object instead of passing it down from the PyBoy constructor? Or am I misunderstanding it?

If a setting is changed that cannot be changed at runtime, the GUI offers to save the changes to the config file and restart PyBoy

For simplicity, we could always just default to save the settings, save state, restart PyBoy and load state again. If our save/load state works as intended, this would fix practically all cases? Then for example key mappings only need to be taken care of in the constructor for the window.

We can always make it better down the line.

Type-specific classes that extend Setting (IntSetting, EnumSetting, StringSetting, BoolSetting, FloatSetting)

Would it be any help to use .json instead of .ini for the settings? The JSON parser built into Python could take care of a lot of the type casting. It would also allow us to make nested dictionaries for settings that belong to specific modules.

The modules will provide a default value and an optional callback for updating the value.

Great idea with the optional callbacks

Note that this will change the syntax for some existing arguments

Remember not to break too much of existing user's experience and documentation.

After PyBoy has finished initializing all of its modules, it will call settings.finalize() or something like that, which will check that all provided options have been mapped to Settings, and print warnings for anything left over. After this, any attempts to create new settings objects will result in an error.

Is this needed?

Lastly, we should probably already start thinking about adding a version number, and possibly a simple settings migrator.

[krs013]

Is settings a singleton object, that lives for each process? Wouldn't it be easier to simple import said object instead of passing it down from the PyBoy constructor? Or am I misunderstanding it?

That was how I thought to do it at first as well, but then if you want to have multiple instances of pyboy that behave differently, it gets complicated. I couldn't think of a good way to have multiple ones without passing them explicitly, but the other option would be to only use a settings object for the interactive side of pyboy, and keep the core stuff out of it so headless instances just don't need it. I'd be very interested in opinions on this if you think it would be better. It makes sense, in a lot of ways, from a user perspective, so it warrants discussion.

For simplicity, we could always just default to save the settings, save state, restart PyBoy and load state again. If our save/load state works as intended, this would fix practically all cases? Then for example key mappings only need to be taken care of in the constructor for the window.

That's an interesting idea. I would think allowing for on-the-fly changes is still a good idea for simple settings but you're right that that could be left out for the first run to get the critical parts working if loading state can make it smooth.

Would it be any help to use .json instead of .ini for the settings? The JSON parser built into Python could take care of a lot of the type casting. It would also allow us to make nested dictionaries for settings that belong to specific modules.

I don't think so. .json is nice but it's better suited for data, while .ini is better for settings. Rather than nested dictionaries for settings, we can just use explicit names and [sections]. configparser (for .ini) is also built into Python. It also can do the type casting, but since we have to accommodate settings coming in from various places (config file, command line, constructor args), it seems best to me to have that logic all in one place to make the behavior more consistent.

Remember not to break too much of existing user's experience and documentation.

We can definitely preserve as many of the existing arguments as necessary, but I figure we can talk about which should stay and which can change to exclusively -o forms. --loadstate [LOADSTATE] is definitely worth keeping. --scale SCALE probably is not, because most users will prefer to set that once in a config file. Most of them are somewhere in between, as I see it. I'd aim to not have more than 8 custom flags if we can help it. But yes, I do remember, which is why I included that note 🙂

After this, any attempts to create new settings objects will result in an error.

Is this needed?

Raising an error, or the whole settings.finalize()? Raising an error isn't strictly necessary, I just added that because I thought it would be good hygiene to make sure that it was clear what was wrong if someone were to try to add a setting down the line in the wrong place. Likewise, doing the finalize() and emitting warnings is there so the user gets a signal if one of their -o's are not recognized. But also that was in part a relic of when I had thought to have multiple instances of global settings objects in one process, so it was useful to have the object available only while it's needed, and then another could take its place later and there wouldn't be any confusion (but if a plugin creates an instance that breaks, which led me to the conclusion at the top that there was no good way to make it work with globals). So then, could it be better to allow for a later lazy binding of settings, perhaps for a module that only will be constructed if it's needed, like the ImageRecorder has sometimes been? Maybe yes, but then I see us dealing with errors where a user enters an invalid value for some lazy binding setting, doing a lot with the emulator, and then crashing suddenly when they finally call upon that value and losing progress. It seems better to me to make sure that error happens at startup.

Lastly, we should probably already start thinking about adding a version number, and possibly a simple settings migrator.

Do you mean to create a numbered branch again, or just to plan to increment when this merges to master? What would the settings migrator do?

Anyway, thanks for the responses! I know it's a lot, and it's taken a long while to get here as well, haha. Hopefully it gets the job done!

[Baekalfen]

I'd be very interested in opinions on this if you think it would be better

I think that sounds like a more convenient way of handling it on the programming side. I kind of imagined, that the settings were loaded from the config file only on start-up and when opening the settings window, we load the file and save to it. This makes it a lot easier to code, and it also fits with the save-state/restart/load-state in regards to configs.

In regards to conflicting configs, we can just let the newest instance override the settings object. It's already a bit of a weird use-case.

allowing for on-the-fly changes is still a good idea

Definitely. I know it's tempting to do the "final" version first, but I think it's better to take it in increments, so it won't be an enormous change at once. We can always improve later. And I don't think this will code us into a corner.

.ini is better for settings

How are we going to store the settings? I have imagined something like this in .json, but I can't imagine it in .ini:

{
  "version": 1
  "main" : {
    "scale" : {
      "type": "int",
      "value": "3"
      },
      ...
  },
  "window" : {
    "BUTTON_A": {
      "type": "enum_key",
      "value": "A"
    }
  }
}

I figure we can talk about which should stay

Yes, let's do that when we get started. Also, we can deprecate some settings and keep them, while hiding them in the documentation.

the user gets a signal if one of their -o's are not recognized

Makes sense. Just thought it sounded like more work to break if somebody creates a new settings object etc. But validating the options with warnings make sense.

if someone were to try to add a setting down the line in the wrong place

Where would settings come from down the line? Bad Python code?

What would the settings migrator do

When we eventually change the format or the available settings, we want to automatically migrate to the new format. Say that an int becomes a float or a string becomes an enum.

[krs013]

Ah, there's a crucial point here that I think I didn't make well enough in the first place: the config file only stores the names and values (as strings) with no extra information like types or defaults or version. All that information is in the code of the modules where the values are used, and then stored at runtime in the settings object. As long as the various settings can parse the strings used to encode their values and return the correct type, there won't be any need for an upgrader or meta data (in fact, if we make an update that changes the type for a certain value, we can make the upgrade in the setting parser functions so it's backwards compatible during the transition.

So a .ini with settings would look something like this:

[pyboy]
pyboy.speed = 1
pyboy.window_title = PyBoy

[pyboy.core]
mb.sound_enabled = False
mb.bootrom_enabled = True
sound.device_num = 0
lcd.renderer.color_palette = 0xFFFFFF,0x999999,0x555555,0x000000

[pyboy.plugins]
screen_recorder.path = ~/pyboy/recordings

...

Obviously that's not everything, and we can decide to have more sections (e.g., a depth of three so pyboy.core.sound and pyboy.core.lcd are separate) and I'm not sure that's exactly how the names would work out when there are multiple classes per file like lcd.renderer, but it's the general idea. The key is that the values are as simple and human readable as possible. When the config file is read, pyboy already knows what types to look for, and in the case of complex settings like lcd.renderer.color_palette, there will just be a method passed in that can turn the string "0xFFFFFF,0x999999,0x555555,0x000000" into the list of numbers that is required.

I think that sounds like a more convenient way of handling it on the programming side.

So does this mean you like the idea of only having a settings object for the singleton GUI instance, and tell headless copies to do something else, or what "that" do you mean?

In regards to conflicting configs, we can just let the newest instance override the settings object. It's already a bit of a weird use-case.

This becomes a problem if you have plugins that create their own instances, or if you have a bunch of instances that are owned by different threads running concurrently like we do with the automated tests.

[Baekalfen]

there won't be any need for an upgrader or meta data (in fact, if we make an update that changes the type for a certain value, we can make the upgrade in the setting parser functions so it's backwards compatible during the transition

How are you able to detect if you're reading a settings file in an old format, if there's neither a version number nor a datatype?

So does this mean you like the idea of only having a settings object for the singleton GUI instance

I like the idea of being able to import the active settings-object directly in each file it's needed (like from django.conf import settings). I don't think corner-cases of people changing the settings in run-time and from different processes are important to handle. I'm certain people will accept that settings are loaded at launch, and that's it. If there's a need to make it complicated or smarter later, we can extend the behavior.

singleton GUI instance

Have you decided on how to access the settings GUI?

This becomes a problem if you have plugins that create their own instances, or if you have a bunch of instances that are owned by different threads running concurrently like we do with the automated tests.

Plugins that create their own instances of PyBoy or the settings object? Either way, we can just say it's not the way to do it, and it's unsupported.

if you have a bunch of instances that are owned by different threads running concurrently like we do with the automated tests

That is a limitation. We could probably make a trick to keep track of the PyBoy instances?

[krs013]

Hey, sorry it took me so long to get back to this! I got distracted with holidays and forgot it was my turn to reply.

On the version detection, I don't think that's something we should worry about. None of the data in the settings file is critical or hard to reproduce--it's just trivial settings. Bundling backwards compatibility like we do for the save states would be more work than it's worth, especially if it limits readability. If an update requires that a user tweak their preferences file, then we don't care. It won't happen that often anyway.

So let's consider whether a single settings object along with a singleton GUI instance of PyBoy is the way to go... it would mean that there are some settings that wouldn't make sense, like a setting for which window plugin to use, or really anything that we'd need to access from non-primary instances. That might not be a bad thing, though: it would force us to treat this feature as a helpful thing for users running the main emulator rather than feature creeping all the way up to some global data structure that would impose extra restrictions on us.

Maybe we could have some kind of flag that goes to pyboy.PyBoy() that indicates if it's a GUI instance or a daemon instance (which could also replace the --window thing we've been doing), and only GUI instances make a settings object and a window and polls for sdl2 events. We could even have it so only primary/GUI instances can run plugins at all, so the daemon instances can be a bit quicker and not burdened with features that don't make sense. That said... it's not how I would probably choose to design something like this, since it would kindof be like running two code bases that are folded into one, which doesn't seem like best practices. Perhaps instead we could separate it between core and GUI and the GUI is what handles the settings, and the core continues to pass arguments like before, and we provide a way to make a core instance that isn't a GUI instance, and main.py just makes a GUI that creates a core instance inside of it, if that makes sense...

Oh, and for what it's worth, if the setting object is a singleton object, then we can do from pyboy import settings or something like that, which is pretty slick. And I guess if we want to be a little less strict, we could let child/core instances use the global settings and just trust that any values that get read multiple times are probably not supposed to be for singleton GUI things. So there wouldn't be a setting that says "make a GUI" because then all the threads make a GUI, but there could be a setting that says "run at CGB mode" and it makes sense for all of the threads/instances to do that. Hmm that might be the simplest solution, actually.

Sorry that's a little all over the place. It's been a while since I thought about this. But I think we're moving in the right direction still.

[Baekalfen]

If an update requires that a user tweak their preferences file, then we don't care

I don't agree on this premise. We could easily have to change formats to something that might break a users config, and you'll have to fix it themselves. And ignoring it, will either make the config parser crash, or ignore their settings ― both of which are frustrating. By adding a simple version counter, we can prevent a lot of issues in the future.

We should go for what makes it easy for us to program against. I doubt we are going to see a lot of people trying to change the settings depending on each instance of PyBoy. And if they do, they should fall back to make separate processes without shared memory.