MLG-Fortress / PopulationDensity

The now-official repository of the PopulationDensity Bukkit plugin
https://dev.bukkit.org/projects/population-density
10 stars 10 forks source link

Support for multiple managed worlds #78

Open QuantumToasted opened 5 years ago

QuantumToasted commented 5 years ago

As I mentioned in #77, I thought it would be nice for PopulationDensity to support multiple managed worlds as opposed to only a single world. I know that such a big undertaking would be a large amount of work and might require lots of testing and many sets of eyes to observe the changes to code to make sure nothing would break, and everything would work the right way.

I am opening this PR with the rough changes I have so far, with the hopes that we can collectively get the ball rolling for this type of support!

Notable code changes:

Do note that much of this code was written or hacked at in the wee morning hours as I was determined to create a working build before sleeping. I do apologies for any gross coding styles, and I greatly appreciate and welcome any changes to better suit your code style. I come from a C# background so I may have missed some common Java style choices.

The one caveat to all of this, however, is that it means that users will need to regenerate or modify their config.yml file to reflect this multiple-world change, otherwise their original managed world may not be detected. I'm sure that could simply be changed by adding a few lines of code to support either single or multiple worlds. It's up to you :)

There is a lot of changes here, so please do not hesitate to fix things or ask me why I did things the way I did. I'll answer to the best of my ability.

RoboMWM commented 5 years ago

Ooooh, you can submit PRs as drafts? Neato.

Gotta say, other than deprecating support for IE, GitHub's been getting better and better after the MS acquisition.

RoboMWM commented 5 years ago

I also come from (a little bit of) a C# background - I like newline braces. I think IntelliJ likes spaces and doesn't do well with tabs and I haven't bothered to figure out how to change it (and I do happen to like spaces better, though that's probably because of much work done in YAML before doing software development).

Remember to keep your PR diff as small as possible, which can be done by avoiding any changes such as any code cleanup or refactoring. If you think that needs to be done, do it in another PR. Other than that, I quickly scanned the diff and it isn't that bad - but obviously would need testing, and should default to a single world as per the original design of the plugin. Lastly, it would need to handle conversion of existing files.

QuantumToasted commented 5 years ago

Remember to keep your PR diff as small as possible, which can be done by avoiding any changes such as any code cleanup or refactoring. If you think that needs to be done, do it in another PR. Other than that, I quickly scanned the diff and it isn't that bad - but obviously would need testing, and should default to a single world as per the original design of the plugin. Lastly, it would need to handle conversion of existing files.

One of my plans will be to provide legacy support for the old ManagedWorldName config value by default, just creating a list with only that world. However a part of the reason I didn't include it directly was that I wanted to know - if a server owner specifies ManagedWorldName and ManagedWorldNames, which of the two should be used? Both? Just one? Should the worlds specified in both be combined?

As for migration of old region data files, that should hopefully be painless, but would need to make heavy assumptions about what world it was for. We'd need to either depend on the user having ManagedWorldName existing in the old config file, or using some other default world, which might be a headache or cause incorrect migrations. I'm open to suggestions of course as to the best way to accomplish that.

RoboMWM commented 5 years ago

We'd need to either depend on the user having ManagedWorldName existing in the old config file.

Yes, this would probably be the case. Check for this key, and migrate its value accordingly to the new List (and then clear it from the config via setting its value to null).

QuantumToasted commented 5 years ago

Yes, this would probably be the case. Check for this key, and migrate its value accordingly to the new List (and then clear it from the config via setting its value to null).

How would you like to handle, for some odd reason, the value not being set? Say, if someone updates to the newest version of the plugin and is aware of the change and removes the key to replace it with ManagedWorldNames? Should we take the first world from that list? Or just the first normal world?

RoboMWM commented 5 years ago

Say, if someone updates to the newest version of the plugin and is aware of the change and removes the key to replace it

If someone's doing this they know what they're doing then. This won't happen normally.

I don't see the issue though. If the key isn't there just skip over it?

QuantumToasted commented 5 years ago

If someone's doing this they know what they're doing then. This won't happen normally.

I don't see the issue though. If the key isn't there just skip over it?

I was referring to importing of /RegionData/* files to the new format. It was a concern over which world to assume was the old world when converting from x z to world,x z filename format. There could only be a handful of possibilities for this:

My concern was only really about the second scenario, about what world to assume the /RegionData/* files were for if for whatever reason ManagedWorldName didn't exist anymore. In the case the user changes or removes their config file but does not delete the RegionData folder.

RoboMWM commented 5 years ago

I'd just abort and disable then, since they performed a partial manual conversion. It's on them if they're trying to convert manually.

RoboMWM commented 5 years ago

Basically it makes no sense if the administrator is half-performing a manual conversion unless they've been instructed to do so. I only see two reasonable, non-theoretical situations occurring for existing users of the plugin:

In the case of the latter (besides it being very unlikely, and that the administrator performing this task most likely already has some competence), they should be already aware that it uses the default world - as this is what the plugin would do anyways if you purge the config that was setup for something other than the default world.

RoboMWM commented 5 years ago

In short, the only case that's worth spending time on is the former. The others are edge cases that we could address in another PR if we really want to engineer resilience into configuration - and even then, if the administrator deletes the only reference of clear data that tells us which world it was configured for, there really isn't much we can do, nor should be worried about... and we'd be just taking guesses at their motives (perhaps they're doing a world reset, etc.)

In other words, do the simplest, most effective thing; don't waste time on theoretical problems.

Spoonerj1 commented 5 years ago

looking forward to seeing multi-world support! :)

RoboMWM commented 4 years ago

Any updates on this @QuantumToasted ?

QuantumToasted commented 4 years ago

I've been really busy with other projects lately, and this, coupled with the fact that the reason I started this project (a personal MC server that I wanted multiple managed worlds on) no longer saw any use and was shut down, it sort of killed any motivation I had to continue on this. Someone else is welcome to pick things up where I left off, otherwise it may be a little while before I tackle the rest of this.

mpdk commented 4 years ago

This would be so cool. Multiworld support is the only thing that the plugin is missing. QuantumToasted you would be the hero of the year if you did this.