Sponge: WorldProperties - GeneratorModifiers not Saving

[Mohron]

I am currently running...

• SpongeForge version: 1.10.2-2281-5.2.0-BETA-2597
• Java version: 8 update 144
• Operating System: Windows 10
• Plugins/Mods:

@Plugin(id = "worldpropertiestest")
public class WorldPropertiesTest {

    @Inject
    private Logger logger;

    @Listener
    public void onConstructWorldProperties(ConstructWorldPropertiesEvent event) {
        if (!event.getWorldProperties().isInitialized()) {
            Collection<WorldGeneratorModifier> modifiers = event.getWorldProperties().getGeneratorModifiers();
            modifiers.add(WorldGeneratorModifiers.VOID);
            event.getWorldProperties().setGeneratorModifiers(modifiers);
            logger.info("{} set to use the \"sponge:void\" world generator modifier.", event.getWorldProperties().getWorldName());
        }
    }
}

What is the expected result?
When adding the sponge:void modifier during ConstructWorldPropertiesEvent, the modifer should be saved to the sponge/worlds/.../.../world/world.conf file.

What is the current result?
The modifier is only applied at runtime and is lost after a restart and by design, is not applied again (!event.getWorldProperties().isInitialized()).

[Mohron]

I've tested this issue with SF 1.12.2-2529-7.0.0-BETA-2730 and the world gen modifier still disappears after a restart.

[dualspiral]

I hesitate to call this a bug in the strictest sense (well, this one actually is, but the fix wouldn't do anything straight off the bat), because this fires due to how the system is designed, but it's not documented anywhere.

It's to do with the world config files defaulting to enabled=false. When the world info is generated, the world config is generated using the defaults, and Mohron here sets the modifiers correctly. I might then set the PvP using the API, and all is rosy...

...until the server restarts.

What's happening is that the config files are not being saved when the config is not enabled. With this issue, @Mohron is always going to get a config that isn't enabled because it's just been generated. If someone then enables PvP through the API, that sticks until the server restarts, and plugin authors that set the PvP state get bugs telling them it doesn't work.

This is linked to an issue where setting properties on the WorldProperties object doesn't stick because per-world configs are not enabled by default - and that breaks the expectation of the API.

I question the value of enabling/disabling world/dim configs - it would be better to allow for overrides and allow it to fall back to the parent level if it doesn't exist - but I know that's not easy to do that with the current setup.

[Mohron]

@dualspiral if I was able to enable the config via the WorldProperties object I'd have no issue doing so when using the ConstructWorldPropertiesEvent to make changes to a newly constructed world but that's not currently possible.

Is there some method I can access through the API to turn on the world/dim config required to make this work?

[dualspiral]

No, there is not at the moment.

[Mohron]

Same goes for the World object but I think it would be more appropriate to exist on WorldProperties since you set the values there.

[ryantheleach]

It's tough, because whilst I like it being a config option, having this moved to the NBT files that follow the world would be more 'correct' and suddenly enabling overrides for configs isn't a sane option, because it could change things the server admin is unaware of.

This needs to be in the NBT files, or moved to the global config with options per world.

Of course, you could always argue:

"by design, is not applied again (!event.getWorldProperties().isInitialized())." is the incorrect approach to take. If you are applying WorldModifiers programmatically, rather then setting the configuration file programmatically, why should they persist? Well, probably because you are calling a setter on something that's mostly persisted to level.dat's . So it's largely expected.

[ryantheleach]

I've set this to input wanted, @Zidane Can a snap decision be made as to which direction this ticket should head, so it's not paralyzed by decision making, and remove the tag once someone has made a decision?

[dualspiral]

I'm not sure what you're getting at.

If you are applying WorldModifiers programmatically, rather then setting the configuration file programmatically, why should they persist? Well, probably because you are calling a setter on something that's mostly persisted to level.dat's

If you take a look at the SC code, that method that sets the modifiers is _meant_ to set the config file programmatically - as far as I can see, the config is designed to act as persistence. The problem is, if config-enabled is set to false for the world, as it is when a world is newly created, then it _doesn't_ persist. There is nothing that @Mohron can do about it.

The way the world/dim/global configs are designed makes this an incredibly difficult question to answer. Unless we added the ability to have a "default" value for every key, this may require some rethink of the config system or enabling the config by default, but solving this will fix more than just this issue, it'll fix the problem with setting PvP programmatically too, for example, which also tries to save this to the config...

[ryantheleach]

"Well, probably because you are calling a setter on something that's mostly persisted to level.dat's . So it's largely expected."

I was playing devils advocate (Of course, you could always argue:), and still ended up agreeing that it should be persisted. It was confusing, sorry.

The problem is, if config-enabled is set to false for the world, as it is when a world is newly created, then it doesn't persist. There is nothing that @Mohron can do about it.

Sure, so my devils advocate argument was that you should always set it, if it doesn't persist. If your plugin isn't loading WGen is going to fail anyway potentially. (assuming it's the same plugin managing world gen, as supplying it(bad assumption))

This needs to be in the NBT files, or moved to the global config with options per world.

Was the main point I was debating, if the world configs are going to be disabled by default, and only used for overriding override-able sections, it simplifies the mental model for the user.

In my mind these are the only 2 fair options, NBT or global config.

world and dimension files are only enabled if there are overrides, therefore the use of those configs is overrides.

Having things that affect the world in there, that don't have globally inherited settings is wrong (currently) as it confuses the mental model. You could always sub-section it, and have an always active section of the config, but that makes the config more confusing.

So I was proposing that we persist the world gen information where the rest of world gen info is currently persisted, the NBT. If this is a bad idea, (it removes control to the user unless using a plugin) Then I would settle for a global configuration option.

The reason I asked for input is this issue stalled, and there wasn't really a clear answer. Zidane, because Zidane has in the past been the apparent code owner of multi-world code.

[gabizou]

Personally, the config system is whacked out to where there's a lot of duplication and ambiguity about where config values are being pulled from.

Personally I prefer the world config being only populated if enabled and if values are different from the global config. This way, you can clearly see what is different, and whether the config is enabled or not. Though, I don't know if this is possible with configurate, might have to prod @kashike @zml2008 etc.

[dualspiral]

It is, it would just require some custom logic in a custom object mapper, I think. I’ve done something similar in Nucleus where a config option isn’t generated unless the option is not the default or already defined. You could do something to link that with a parent config, or route read calls through some method that finds the first place the option is defined.

The other option is to add a “default” value to each and handle that in the object mapper, but that could be hacky.

[ryantheleach]

https://github.com/SpongePowered/SpongeCommon/issues/1764

So the setting is read from the per world file fine (regardless of config-enabled), it's just the persistence that's off.

I suspect this came from a hacky fix to make some of the world config properties NOT inherited or triggered by config-enabled rather then fixing it properly, or re-arranging the config.

So the fastest fix, would be to persist changes no matter if the config is enabled or not.

The re-arrangement can always come later.

This needs to be in the NBT files, or moved to the global config with options per world.

After finding out that many of the settings in the config file are always read, and not toggled by config-enabled, by design, then moving this to the NBT or global config would potentially fix it for this one issue, but wouldn't fix it for the other values that are always enabled.

[OstenTV]

I'm having problems with Nucleus not saving the gamemode of a world (Multi-world thingy). They say it's because of this problem. Plz fix :)

[bloodmc]

This has been resolved with Luck's PR https://github.com/SpongePowered/SpongeCommon/pull/1957