Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add options API #1378

Merged
merged 7 commits into from
Feb 16, 2022
Merged

Add options API #1378

merged 7 commits into from
Feb 16, 2022

Conversation

originalfoo
Copy link
Member

@originalfoo originalfoo commented Feb 7, 2022

Fixes: #1376

Simple API for external mods to get TM:PE mod Options values. For example, if a mod needs to know which TMPE features are enabled, or get info on policy settings, icon theme, etc.

Vague example:

var mgr = OptionsManager.Instance;

if (mgr != null && mgr.OptionsAreSafeToQuery()) {
    if (mgr.TryGetOptionByName<bool>("turnOnRedEnabled", out bool turnOnRedEnabled) {
        if (turnOnRedEnabled) {
            // show some extra features in your traffic lights mod or whatever
        }
    }
}

Options are referenced by name, thus allowing us to alter names at later date whilst keeping API intact (a simple oldname -> newname lookup can be added on our side).

Options are accessed via reflection - while not super fast, it's reliable and allows us to easily handle possible future transition from option fields to properties, etc.

Thanks to @DaEgi01 for suggestion to use generics.

@originalfoo originalfoo added API API for external mods Settings Road config, mod options, config xml labels Feb 7, 2022
@originalfoo originalfoo added this to the 11.6.5 milestone Feb 7, 2022
@originalfoo originalfoo self-assigned this Feb 7, 2022
Copy link
Member

@krzychu124 krzychu124 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added some thoughts

TLM/TMPE.API/Manager/IOptionsManager.cs Outdated Show resolved Hide resolved
return true;
}
catch (Exception ex) {
ex.LogException();

// even though there was error, the options are now available for querying
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh .. are you sure about that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. If the load method fails, the options will just have default values but TM:PE will still be fully operable.

The goal was to prevent other mods reading options either prior to options being set, or during save/load, or from main menu.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But whats the point of playing with default options?
And what happens if I save the game with those defaults?
And what if another mod makes descisions based on that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If its fine for krzy its fine for me though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we shouldn't try to save anything if failed to load, that won't overwrite old data, so in case of a bug in load code it could be loaded after applying hotfix - IIRC game is cloning mod data container so even if you not longer have a mod, data will be migrated to new saves.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior to this PR, what would the difference be?

If the data fails to load, the LoadData() method will still return false just like it previously did, which results in loadingSucceeded = false at the call site - that currently has no effect on whether SaveData() gets invoked later.

I've created an issue to track the need to prevent data save should data fail to load.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is ok, you can merge :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TLM/TLM/Manager/Impl/OptionsManager.cs Outdated Show resolved Hide resolved
TLM/TLM/State/Options.cs Outdated Show resolved Hide resolved
@originalfoo
Copy link
Member Author

updated based on review feedback

return true;
}
catch (Exception ex) {
ex.LogException();

// even though there was error, the options are now available for querying
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But whats the point of playing with default options?
And what happens if I save the game with those defaults?
And what if another mod makes descisions based on that?

return true;
}
catch (Exception ex) {
ex.LogException();

// even though there was error, the options are now available for querying
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If its fine for krzy its fine for me though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API API for external mods Settings Road config, mod options, config xml
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API for external mods to query TM:PE mod options
3 participants