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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

AutoConfig #3723

Merged
merged 22 commits into from Oct 15, 2019

Conversation

@eviltak
Copy link
Member

eviltak commented Aug 13, 2019

Motivation

The goal of FCs (as listed in #2668) was to allow the "engine and modules [to be] able to publish their configuration settings, as needed." This is currently achieved by creating a map of settings which can be added to and accessed at runtime (see #2757). However, after looking at #3258 and existing config classes, it looks like that settings will only ever be accessed dynamically at runtime -- there shouldn't ever be a need for settings to be published at runtime, since all the settings to be published should be known at the time of writing.

This is evident from the fact that all configs are currently stored as hardcoded classes, with concrete fields as the "settings". FlexibleConfigs, thus, might be a bit too flexible for our uses! This PR thus proposes a hybrid approach, one which addresses all the shortcomings of both the FC and the concrete class approaches while also in my opinion being the most suitable to our use case.

The new approach

The hybrid approach involves using concrete config classes (which are marked as such by inheriting the AutoConfig class) with public final fields that are of type Setting (the FC setting). Here is the AudioConfig as an actual, usable AutoConfig in action (no seriously, that is all that is required to publish an AutoConfig 馃槃):

public class AudioConfig extends AutoConfig {
public final Setting<Float> soundVolume =
setting(
type(Float.class),
defaultValue(1.0f),
// From AudioSettingsScreen
constraint(new NumberRangeConstraint<>(0.0f, 1.0f, true, true))
);
public final Setting<Float> musicVolume =
setting(
type(Float.class),
defaultValue(1.0f),
constraint(new NumberRangeConstraint<>(0.0f, 1.0f, true, true))
);
// TODO: Convert into Setting -- no uses yet
private boolean disableSound;
}

Notes

  • This PR only converts AudioConfig to an AutoConfig as a proof-of-concept -- other engine configs will be covered in future PRs
  • This PR removes FlexibleConfig and all related types with the intention of replacing them with AutoConfig, but it is possible for both to coexist if needed
  • AutoConfig UI will be very similar to FC UI and hence will be included in #3699
  • AutoConfigs are saved in /homeDir/configs under a subdirectory named after the module declaring them

The DSL

Rather than use the newly introduced FlexibleConfig.SettingEntry API, settings are constructed using a DSL of sorts, enumerated by the AutoConfig.setting static method and the SettingArgument class (and its static factory methods). Couple of examples are above in AudioConfig.

Another example using all setting parameters:

Setting<Double> setting = AutoConfig.setting(
type(Double.class),
defaultValue(defaultValue),
constraint(constraint),
name(name),
description(description)
);

All the methods used are static and stateless. They can also be statically imported to make setting declaration easy to read.

The DSL is also fully type-checked and compiler enforced; it is also possible to make some parameters compulsory (as is done for the type and defaultValue parameters) and the others optional. It is easy to employ this DSL in FCs too, if needed.

Advantages

Versus FCs

  • Each setting is now accessed via the field in the config class (an instance of which will be retrievable via the context); this can be enforced by the compiler and doesn't require a unique identifier.
  • The type of the setting is stored in the field type and enforced by the compiler, allowing accessors of the setting to not have to guess what the type of the setting could be.
  • Modules/code that require access to a config/setting must be able to access the corresponding config class, and therefore must have the config declaring module as a dependency.

Versus concrete classes (currently used)

  • Only those classes marked by the AutoConfig base type are treated as a config class and stored in a AutoConfigManager.
  • The AutoConfigManager then looks through the fields of each loaded Config and recognizes those fields with the type Setting as a setting in the config and handles the UI/serialization automagically.
  • Modules/classes can thus create configs as they please -- all they have to do is create a class that is marked as a AutoConfig and publish settings in its fields.
    This will work very much like ComponentSystem and ComponentSystemManager do -- to create a ComponentSystem, all a module does is create a class that inherits the ComponentSystem interface. All ComponentSystems in the ModuleEnvironment are loaded by the ComponentSystemManager, and are initialized/handled/called as needed.
  • All the Config class needs to do to publish a setting is to declare a public final field of type Setting and initialize it with the constraints and the defaults; no other methods needed.
  • Unlike the current hardcoded config classes, getters and setters for each setting aren't needed since the Setting interface encapsulates all of the responsibilities that the getters and setters currently fulfill via the getValue and setValue methods.
    This includes checking if the value is valid (via the SettingConstraint) and notifying subscribers of any changes.
  • The AutoConfig base type could also contain helpful utility methods like subscribeAll and unsubscribeAll, which subscribes and unsubscribes a listener to and from all settings in a config.

Future work

  • UI as part of #3699
  • Concept of global and per-save configs
@eviltak eviltak requested review from Cervator and vampcat Aug 13, 2019
@eviltak eviltak self-assigned this Aug 13, 2019
@GooeyHub

This comment has been minimized.

Copy link
Member

GooeyHub commented Aug 13, 2019

Hooray Jenkins reported success with all tests good!

2 similar comments
@GooeyHub

This comment has been minimized.

Copy link
Member

GooeyHub commented Aug 13, 2019

Hooray Jenkins reported success with all tests good!

@GooeyHub

This comment has been minimized.

Copy link
Member

GooeyHub commented Aug 14, 2019

Hooray Jenkins reported success with all tests good!

@eviltak eviltak marked this pull request as ready for review Aug 14, 2019
@GooeyHub

This comment has been minimized.

Copy link
Member

GooeyHub commented Aug 14, 2019

Hooray Jenkins reported success with all tests good!

1 similar comment
@GooeyHub

This comment has been minimized.

Copy link
Member

GooeyHub commented Aug 14, 2019

Hooray Jenkins reported success with all tests good!

@eviltak eviltak referenced this pull request Aug 15, 2019
1 of 7 tasks complete
@eviltak eviltak requested review from jellysnake and oniatus and removed request for Cervator Aug 17, 2019
@Cervator Cervator requested review from dave2s and immortius Aug 25, 2019
Copy link
Member

skaldarnar left a comment

I'm convinced by this changeset, some minor things that could be done/added/changed, but it could also be done in another iteration.
Thanks a lot @eviltak

Copy link
Contributor

Qwertygiy left a comment

Tested, played around with the values to explore different available options, and with the exception of the "description" key (which is intentionally pending for UI to deal with) everything functioned as expected.

@eviltak eviltak force-pushed the eviltak:autoconfig branch from 67b3a6a to 0f7288a Oct 15, 2019
@eviltak

This comment has been minimized.

Copy link
Member Author

eviltak commented Oct 15, 2019

Rebased, please re-fetch the branch.

@GooeyHub

This comment has been minimized.

Copy link
Member

GooeyHub commented Oct 15, 2019

Uh oh, something went wrong with the build. Need to check on that

@GooeyHub

This comment has been minimized.

Copy link
Member

GooeyHub commented Oct 15, 2019

Hooray Jenkins reported success with all tests good!

@skaldarnar skaldarnar merged commit 3690b3d into MovingBlocks:develop Oct 15, 2019
2 checks passed
2 checks passed
LGTM analysis: Java No new or fixed alerts
Details
default Build finished.
Details
@Cervator Cervator added this to the v2.3.0 milestone Oct 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.