Reworking Worlds management (introducing GameWorld & GameMode)

[Poslovitch]

Related to #378.

Introducing the rework

Although the current "Worlds management" is working fine, it suffers a few downsides:

• it doesn't support multiple schems per world;
• the IslandsManager manages Islands from all Worlds, which I think can be counter-intuitive;
• Addons that create Worlds are in no way distinguished from addons that don't.

What is the purpose of this rework ?

Instead of having Worlds as parameters for most of the methods in IslandsManager, I wanted to make us "select the World we want to interact with first". I don't know how to clearly express myself, but here's a "comparison" between the current API and the one I'm working one.

Current API:

Island island = plugin.getIslands().getIslandAt(World, Location);

Which is like:
from all the islands that are currently managed, get the one from this world at this location.

New API (pseudo-code):

Island island = plugin.getWorlds().getGameWorld(World).getIslands().getIslandAt(Location);

Well...

In facts, when I've started this rework almost a month ago, it seemed to be the best idea ever. But now, I'm unsure.
Is this all worth this big change?

I definitely need some feedback about this one.

[BONNe]

I would like to be able to specify multiple shems in each world, so I am for this change.
Also it looks much better when islands are separated by world and not crammed all in one manager.

The only thing that I do not like, is method plugin.getWorlds(), as it returns WorldManager, not worlds, so it is counter-intuitive.
And downside is to "fix" all other addons with new behavior.

[Poslovitch]

I think this is a bit "too much" there. How could we make this API call less big?

[BONNe]

One way is to create new method in BentoBox plugin
plugin.getGameWorld(World) { return this.worldManager.getGameWorld(World); }

Another good nanosecond optimization is to create new variable in this method
if (!e.getEntityType().equals(EntityType.ENDER_DRAGON))
{
World world = e.getEntity().getWorld();
if (plugin.getGameWorld(world).map(gameWorld -> gameWorld.getSettings().isDragonSpawn()).orElse(false))) { // do staff }

[Poslovitch]

I'm not sure if the plugin.getGameWorld(World) { return this.worldManager.getGameWorld(World); } method would be a good idea.

In regards of the variable - that's a no, I think. That'd definitely make the condition look "lighter" but it'll mainly create this unnecessary variable at each call.

[BONNe]

So internal variable is an optimization, as it avoid multiple calls for .getEntity().getWorld()
Its small, but in some cases a necessary optimization, to avoid unnecessary method calls.

So it improves performance over memory usage.

[Poslovitch]

This is not an optimization. getEntity().getWorld() are just getters that return a value that is part of the object - there is no calculations done there.
What I could do, however, is providing a method to directly access the GameSettings of a GameWorld...

[tastybento]

@Poslovitch Thanks for this. It takes a big man to be subjective and say "I'm not sure about this". I'm on holiday now so I'll take a look at this and give you my thoughts. Thanks again!

[tastybento]

@Poslovitch Thanks again for the work.

Just looking this, although you mentioned the current API is:

Island island = plugin.getIslands().getIslandAt(World, Location);

that is not actually correct - the current API is:

Optional<Island> island = getIslandAt(Location location);

This is because Location already contains the World so it's not required to have it as a parameter. Unfortunately, the new API now requires the world parameter no matter what:

Island island = plugin.getWorlds().getGameWorld(World).getIslands().getIslandAt(Location);

Also, the code above will require a null check on "getGameWold(World)" and getIslandAt(Location) because there may not be a game world in world, e.g., it could be another world on the server.and there may not be an island here. So that's two checks. With the current API, the returning Optional can be used quickly and easily for any world.

So, in regards to making the API cleaner, this is not a step in the right direction and the current API is actually about as small as you can get I think...

Just stepping back a bit - having a GameWorld class is not bad, but I don't think we have to have to put an IslandManager in each one. Indeed, keeping the IslandManager (by the way, it could be renamed to be WorldManager if you like) global makes sense because any Location given is a unique keys to identifying islands. Throughout the code, often we have queries for locations, flags or worlds and we just want to ask "is there an island there?" or "is there a setting for this world" and get a result. This can be done easily with a global Island/World Manager.

Where a GameWorld class might be useful is to clean up the world, worldSettings and schematics registration in an add-on. Right now, an add-on registers a world with BentoBox using this method:

plugin.registerWorld(world, worldSettings) (Example here)

And then is registers schematics with BentoBox using:

plugin.getSchemsManager().loadIslands(world); (Example here)

One thing I do like about the new GameWorld class is that you can register both the world and schems in one go and also it forces an add-on that makes worlds to supply schems, which it must do otherwise no islands can ever be made. So, perhaps we can do something around that.

In terms of enabling multiple schems, please have a look at #412. This uses the current API and just expands the current approach. I wrote it to see if there were any blockers or optimizations that the current API was limiting. I thought I might hit one, but in the end it went pretty much straightforward. The current UI is just text based (no GUI), but both the /island create and /island reset commands allow the addition of a schem name to them and they check the permission.

[Poslovitch]

Okay, thanks for the reviews @tastybento and @BONNe.

I get the point with this API not making things simpler. That's what I was afraid of.
However, I like the idea of the GameWorld class too. I'll go ahead and make another branch and PR.

We can consider this PR as declined.