The Wayback Machine - https://web.archive.org/web/20220925225449/https://github.com/PaperMC/Paper/pull/7609
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

Convert to configurate for paper.yml #7609

Merged
merged 1 commit into from Jun 9, 2022

Conversation

Machine-Maker
Copy link
Member

@Machine-Maker Machine-Maker commented Mar 15, 2022

Key Points

  • This does away with the traditional way (started by spigot) of reading configs in favor of using configurate.
  • Splits up the config into multiple files, a global file, a world-defaults file, and a file for each world that users can place overrides.
  • The current plan, is to have 1 patch at the beginning that new options are added to, and then those options are "implemented" in their own patch. This means the GlobalConfiguration and WorldConfiguration classes should only ever be modified inside one patch.

New directory structure

root/
	config/
		paper-global.yml
		paper-world-defaults.yml
	world/
		level.dat
		...
		paper-world.yml
	world_nether/
		level.dat
		paper-world.yml

TODO

  • Configurate 4.2 (full YAML comment support)
  • (depends on ^) Add @Setting to all options.
  • remove PaperConfig & PaperWorldConfig
  • edit patches that add config options to make them use the new config objects
  • write migrations from current format to new one
  • leave backup of legacy paper.yml for reference (moved to config/legacy-backup/paper.yml)
@jacksyyy
Copy link

jacksyyy commented Mar 15, 2022

I think this is very good idea :shipit: 👍

My only issue is the per world config file in the world folder. is this not confusing for the admin? How will this work with #7594?

This seems like it has pro of portable world. but if everything is one levelstem with vanilla this becomes very messy.

here is my idea for better

root/
	config/
		paper-global.yml
		paper-world-defaults.yml
                worlds/
                    paper-world.yml
                    paper-world_the_end.yml
                    paper-world_nether.yml

it is just idea. I think it work for the server admin and #7594 more.

@electronicboy
Copy link
Member

electronicboy commented Mar 15, 2022

There are pros and cons to both approaches, we somewhat figured that config files in the worlds seemed to be the best solution, especially in terms of things like minigame servers which carry worlds around, etc, it's much easier for them if the world is bundled with the config

the levelstem issue is complex, but, individual dimensions will still need their own level.dat, so it's not like everything is gonna be shared across stuff, just there are many complexities there as the entire systems both go against one another, data packs expect to be global state yet CB stuff expects some level of isolation

that file structure would also have to look more akin to

root/config/worlds/minecraft/world/paper-world.yml
root/config/worlds/minecraft/world_the_end/paper-world.yml
root/config/worlds/minecraft/world_nether/paper-world.yml
root/config/worlds/mymagicalpack/magical_world/paper-world.yml
etc

which is also somewhat ugly and doesn't really solve much vs keeping them with the world data

@jacksyyy
Copy link

jacksyyy commented Mar 15, 2022

it does not have to be like that, you can even use full namespace

root/config/worlds/minecraft:world.yml
root/config/worlds/minecraft:world_the_end.yml
root/config/worlds/minecraft:world_nether.yml
root/config/worlds/mymagicalpack:magical_world.yml
etc

I think this general idea looks much better. but : in filename is not working well probabably.
so maybe replace with - or other better char not valid as namespacedkey.

This is just my idea. it is your project. I like it!

@electronicboy
Copy link
Member

electronicboy commented Mar 15, 2022

: in filenames is not supported, - is a valid seperator elsewhere and that form of substitution just often creates its own weird compat issues in niche cases which people often forget about and then spend 2 weeks debugging

@jacksyyy
Copy link

jacksyyy commented Mar 15, 2022

I mention this in my comment. even with replace with another char it is still to me more user friendly. but i respect whatever final decision. i just bringing more ideas hopefully.

@PhilCCMG
Copy link

PhilCCMG commented Mar 16, 2022

This is a good move providing everything migrates fully and automatically (as it seems to be so far) and the old paper.yml has a huge notice saying configs have moved to avoid confusion.

Regarding location, I personally think something like
/root/world/paper.yml, /root/world_nether/paper.yml etc would be best, as it means optimised and configured worlds can easily be transferred between people in archive form.

@Machine-Maker Machine-Maker added the for: future Issue scheduled for resolution at some point in the future. label Mar 16, 2022
@Machine-Maker
Copy link
Member Author

Machine-Maker commented May 8, 2022

Updated for latest paper config changes

@Machine-Maker Machine-Maker force-pushed the feature/configurate branch 3 times, most recently from c289312 to 8cb05eb Compare May 30, 2022
@Machine-Maker Machine-Maker changed the base branch from master to dev/1.19 Jun 9, 2022
Copy link
Member

@kennytv kennytv left a comment

Also, making configurable messages use the minimessage format would be good to have, there won't be a better time to do that than now

patches/api/0002-Build-system-changes.patch Outdated Show resolved Hide resolved
+ private static final Map<String, Command> COMMANDS = new HashMap<>();
+ private static boolean metricsStarted = false;
+ static {
+ COMMANDS.put("paper", new PaperCommand("paper"));
+ COMMANDS.put("mspt", new MSPTCommand("mspt"));
+ }
+
+ public static void registerCommands(final MinecraftServer server) {
+ COMMANDS.forEach((s, command) -> {
+ server.server.getCommandMap().register(s, "Paper", command);
+ });
+
+ if (!metricsStarted) {
+ Metrics.PaperMetrics.startMetrics();
+ metricsStarted = true;
+ }
Copy link
Member

@kennytv kennytv Jun 9, 2022

Choose a reason for hiding this comment

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

Move commands and registration somewhere else

patches/server/0004-Paper-config-files.patch Show resolved Hide resolved
@kennytv kennytv removed the for: future Issue scheduled for resolution at some point in the future. label Jun 9, 2022
@kennytv kennytv marked this pull request as ready for review Jun 9, 2022
@kennytv kennytv merged commit 172d260 into PaperMC:dev/1.19 Jun 9, 2022
1 check passed
@Roan-V
Copy link

Roan-V commented Jun 10, 2022

Are there plans to/would it be possible to also move bukkit.yml and spigot.yml into root/config/?

@electronicboy
Copy link
Member

electronicboy commented Jun 10, 2022

atm, no; There is some plans to merge them all into a single file post hardfork

@Roan-V
Copy link

Roan-V commented Jun 13, 2022

Doesn't that basically defeat the point of having a config folder?
A config folder is nice, but not if it's inconsistent and a bunch of other files are somewhere else.

plans to ... post hardfork

Also doesn't say much sadly, a hard fork has always been this nebulous thing in the future

@Machine-Maker
Copy link
Member Author

Machine-Maker commented Jun 14, 2022

A config folder is nice, but not if it's inconsistent and a bunch of other files are somewhere else.

It leaves open the opportunity to have additional files if the need arises. Like what if there is a new config thing that ends up being super big. It might warrant having its own file. If we already have a directory, it makes that super easy.

Also we already have 2 files, the world defaults and the global. So those are best not in the root of the server

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants