-
-
Notifications
You must be signed in to change notification settings - Fork 682
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
Switch config system to Configurate #5010
base: master
Are you sure you want to change the base?
Conversation
Improve node ordering when updating configs
core/src/main/java/org/geysermc/geyser/configuration/GeyserConfig.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/geysermc/geyser/configuration/GeyserConfig.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/geysermc/geyser/configuration/GeyserConfig.java
Outdated
Show resolved
Hide resolved
@SuppressWarnings("BooleanMethodIsAlwaysInverted") | ||
private boolean loadConfig() { | ||
try { | ||
if (!getDataFolder().exists()) //noinspection ResultOfMethodCallIgnored | ||
getDataFolder().mkdir(); | ||
File configFile = FileUtils.fileOrCopiedFromResource(new File(getDataFolder(), "config.yml"), | ||
"config.yml", (x) -> x.replaceAll("generateduuid", UUID.randomUUID().toString()), this); | ||
this.geyserConfig = FileUtils.loadConfig(configFile, GeyserBungeeConfiguration.class); | ||
} catch (IOException ex) { | ||
geyserLogger.error(GeyserLocale.getLocaleStringLog("geyser.config.failed"), ex); | ||
ex.printStackTrace(); | ||
return false; | ||
} | ||
return true; | ||
this.geyserConfig = new ConfigLoader(this).createFolder().load(GeyserPluginConfig.class); | ||
return this.geyserConfig != null; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's worth it to make this a default method in GeyserBootstrap - it's currently duplicated in every bootstrap impl
...p/mod/src/main/java/org/geysermc/geyser/platform/mod/mixin/client/IntegratedServerMixin.java
Show resolved
Hide resolved
} | ||
|
||
private AdvancedConfig migrateToAdvancedConfig(File file, ConfigurationNode configRoot) throws IOException { | ||
Stream<NodePath> copyFromOldConfig = Stream.of("max-visible-custom-skulls", "custom-skull-render-distance", "scoreboard-packet-threshold", "mtu", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other options that may be worth moving:
- compression level
- forward-player-ping
- maybe debug-mode (and then with e.g. scoreboard/pack length warnings that are currently system properties being added there too?)
- Maybe as hidden options that are not present by default: cookies, packet limit, threads
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debug mode was intentionally kept in config.yml by me because we use it when debugging with users sometimes. Can be up for discussion though.
core/src/main/java/org/geysermc/geyser/configuration/ExcludePlatform.java
Show resolved
Hide resolved
core/src/main/java/org/geysermc/geyser/configuration/GeyserConfig.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
14 files left that are more closely related to config loading. can probably get to those on friday
default boolean disableRelocateCheck() { | ||
PlatformType platformType = GeyserImpl.getInstance().platformType(); | ||
return platformType == PlatformType.FABRIC || | ||
platformType == PlatformType.NEOFORGE || | ||
platformType == PlatformType.STANDALONE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment about using JIJ on modded, and not doing relocations on standalone?
|
||
// Sorry Konicai... | ||
forAllConfigs(type -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment can be removed now haha
core/src/main/java/org/geysermc/geyser/text/AsteriskSerializer.java
Outdated
Show resolved
Hide resolved
try { | ||
// Workaround for JsonAdapter not being allowed on methods | ||
ConfigurationOptions options = InterfaceDefaultOptions.addTo(ConfigurationOptions.defaults(), builder -> | ||
builder.addProcessor(AsteriskSerializer.Asterisk.class, String.class, AsteriskSerializer.CONFIGURATE_SERIALIZER)) | ||
.shouldCopyDefaults(false); | ||
|
||
ConfigurationNode configNode = CommentedConfigurationNode.root(options); | ||
configNode.set(geyser.config()); | ||
this.config = configNode.get(geyser.config().getClass()); | ||
|
||
ConfigurationNode advancedConfigNode = CommentedConfigurationNode.root(options); | ||
advancedConfigNode.set(geyser.config().advanced()); | ||
this.advancedConfig = advancedConfigNode.get(AdvancedConfig.class); | ||
} catch (SerializationException e) { | ||
if (geyser.config().debugMode()) { | ||
e.printStackTrace(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use configurate-gson
instead? Should only increase jar size by 6 classes at first glance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did attempt that once... I think I can try again.
private Path floodgateKeyPath; | ||
|
||
public void loadFloodgate(Path floodgateDataFolder) { | ||
floodgateKeyPath = FloodgateKeyLoader.getKeyPath(geyserConfig, floodgateDataFolder, dataFolder, geyserLogger); | ||
} | ||
|
||
@Override | ||
public Path getFloodgateKeyPath() { | ||
return floodgateKeyPath; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why isn't the FloodgateKeyPath just calling FloodgateKeyLoader, like other implementations? and why is testFloodgatePluginPresent the one responsible for calling this loadFloodgate method (seems like it was like that before tho)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as it works, I'm not too concerned, as it will be yeeted sometime in the short term.
Co-authored-by: Konicai <[email protected]>
// MinecraftAuth | ||
maven("https://maven.lenni0451.net/snapshots") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be removed again, we're back to release
The primary goal of this PR is to facilitate config changes required for the Floodgate merge, while also allowing for automatic config file generation and updates, as well as customizing configs depending on the features available to a platform.
We have also decided to remove usage of the Jackson library within Geyser. While it has its benefits over Gson, ultimately all platforms have Gson access (and our libraries utilize Gson alongside that) and Jackson is a hefty dependency to shade within Geyser.
User-facing changes
The config file is split into two files -
config.yml
andconfig-advanced.yml
(name subject to change to something that will always place it belowconfig.yml
alphabetically). The primary config will consist of values that a typical user needs to change for a typical setup or other basic config values. The advanced config will consist of values that we don't want most users to change, while still being accessible to tech-savvy users.The ultimate goal is to keep setup super-simple while still allowing Geyser to be customizable.
Metrics config options are now moved to platform global configs, if they currently exist.