Skip to content

Conversation

@QuantumToasted
Copy link

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:

  • PopulationDensity.ManagedWorld has become PopulationDensity.ManagedWorlds and is now of type List<World>. I did my best to ensure any references or checks for ManagedWorld would now perform the proper reference or check inside ManagedWorlds (that is, to ensure that the player does not do something in one managed world that would affect another). Testing will be needed to ensure this is the case.
  • RegionCoordinates now also contains a world property/field such that methods which require it can now be linked to the world they're for. It prevents needing to pass Worlds into every method, but if you do not like this, it can be removed. I also modified the toString(), public RegionCoordinates(String string), and equals() methods to better accomodate and reflect these changes.
  • A few small code changes here and there were made to better communicate that there can be multiple managed worlds. Some console logging makes note to add world info during region scans.
  • DataStore has a small change, namely that region data files now are in the format world,x z instead of the previous x z format, to ensure regions are loaded for the right world. (See changes to RegionCoordinates for more info).
  • Many others I might have missed

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.

Very rough work, a fair amount of replacing here and there, but at the very least, it does not cause console errors or warnings save for server slowdowns or long periods of inactivity due to resource scans.
@RoboMWM
Copy link
Member

RoboMWM commented Aug 30, 2019

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
Copy link
Member

RoboMWM commented Aug 30, 2019

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
Copy link
Author

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.

QuantumToasted and others added 7 commits August 31, 2019 12:53
I have not bothered to dig through the source code for Bukkit/Spigot, so I am unaware if == is equivalent to .equals() in this case.
Tried CTRL+SHIFT+F in IDEA, let me know if there are any i missed.
For some reason, IDEA's building tools let this slip by, and caused it to build successfully but fail to compile. I don't get it.
@RoboMWM
Copy link
Member

RoboMWM commented Aug 31, 2019

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).

Hopefully for the last damn time.
I don't remember who originally wrote this line of code, if it wasn't me, I just saw it while i was fixing imports so I thought to be consistent.
@QuantumToasted
Copy link
Author

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
Copy link
Member

RoboMWM commented Aug 31, 2019

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
Copy link
Author

QuantumToasted commented Aug 31, 2019

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:

  • The user doesn't change their config file at all. ManagedWorldName is used.
  • The user changes their config file, setting ManagedWorldNames, deleting ManagedWorldName, or even just deleting the config file altogether to let the plugin re-generate.

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
Copy link
Member

RoboMWM commented Aug 31, 2019

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
Copy link
Member

RoboMWM commented Sep 8, 2019

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:

  • They simply update the jar as per usual. Config contains all keys of the prior version.
  • They delete the config file to regenerate a fresh config.

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
Copy link
Member

RoboMWM commented Sep 8, 2019

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
Copy link

looking forward to seeing multi-world support! :)

@RoboMWM
Copy link
Member

RoboMWM commented Nov 17, 2019

Any updates on this @QuantumToasted ?

@QuantumToasted
Copy link
Author

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
Copy link

mpdk commented Dec 20, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants