World.equals not working properly

I'm running multiverse and have 1 additional test world running.
I have a HashMap<Location, Circuit> that maps chips to their sign locations. These locations can be on any world and the map is populated on server boot after all other plugins have loaded. The plugin creates the Location objects from scratch:

Code:

World w = rc.getServer().getWorld(worldName);
Location l = new Location(w, coords.get(0), coords.get(1), coords.get(2));

When I later try to get the circuit using Block.getLocation() like so:

Code:

Circuit c = this.activationLookupMap.get(activationBlock.getLocation());

It will always return null when the location object's world is not the main world (ie not world 0, the test world). I checked and realized that the Block.getLocation().getWorld() and the world instance in the map were not the same object, they had different hash codes but their names were the same.
getServer().getWorld(worldName) does work with the map.

So basically, it seems that rc.getServer().getWorld() returns a different object than Block.getLocation().getWorld() for the same world.

I'm not sure if that's a pretty bad bukkit bug, a multiverse bug or maybe I'm just missing something. Either way I don't think there should be more then one World object per world floating around.
Any help would be appreciated...

 

Compare the world names, not the world. I don't trust .equals methods unless it's for Java libraries, or has custom javadocs for it.

 

Thanks, that's how I solved it in other cases but I really want to use the Location objects for HashMap keys and this issue makes it impossible. There's no problem with Location.equals() except that it needs to match World objects and these don't have an equals() or hashCode() methods so their object hash codes are compared.
But anyways, as I wrote, The fact that there are two World objects for one actual world is even more troubling.

 

I made a plugin to illustrate the problem
https://github.com/eisental/RedstoneChips/blob/master/testrelease/WorldHash.zip

Source + jar file in the /target folder.
Run /worldhash in one of the worlds to see if that world has two different hash codes.
For me the default world (world 0) has 1 code and other worlds have 2 different codes.

I'll be grateful if someone could test it on their multiworld setup and tell me if they have 2 different hash codes as well.

 

Yeah, I see it too.

As for a solution, you could create a world wrapper or superclass that just adds a working .equals method, and use that instead.

 

Thanks for testing it. I don't think what you're suggesting is really possible in Java. Can you give an example?

 

Okay - so you want to use Location as a key, right? This should work:

Code:

import org.bukkit.Location;
import org.bukkit.World;

public class LocationKey extends Location{
	public LocationKey(World world, double x, double y, double z) {
		super(world, x, y, z);
	}
	public LocationKey(Location loc) {
		super(loc.getWorld(), loc.getX(), loc.getY(), loc.getZ());
	}

	public boolean equals(Object o) {
		if (o instanceof LocationKey) {
			LocationKey key = (LocationKey)o;
			if (key.getX() != this.getX()) {
				return false;
			}
			if (key.getY() != this.getY()) {
				return false;
			}
			if (key.getZ() != this.getZ()) {
				return false;
			}
			if (!key.getWorld().getName().equals(this.getWorld().getName())) {
				return false;
			}
			return true;
		}
		return false;
	}
}

Create a location key, and use it for the hashmap key. Similar things could be done for the World classes so the .equals methods works for them too.

 

My god the bukkit core is confusing. This is my best guess as to the cause and solution to this problem.

createWorld creates a new WorldServer object (CraftServer.java:286) for the world, then initializes the world, then creates a new CraftWorld object (CraftServer.java:321)

The WorldServer object however creates its own CraftWorld object (WorldServer.java:29)

When you get a world using p.getWorld() it gets one of these worlds, while using getWorld(name) gets the other.

What should _probably_ fix this is if CraftServer.java:321 were changed to:

Code:

return internal.getWorld();

Just tested and that does indeed fix the issue, and I don't foresee it causing any other issues as the CraftWorld object is still created in the WorldServer constructor.
I put in a pull request, so hopefully it gets merged into the bukkit repo

Attached is a picture showing that two different worlds show the same hash code, are equal when compared with .equals, and are equal when compared with ==

Had to add one more change, the WORLD_LOADED event wasn't being called in the right place, so had to move it. Git pull request: https://github.com/Bukkit/CraftBukkit/pull/202

 

@Drakia Great! Thanks a lot for taking care of this. I didn't test your fix yet, but the screenshot is very encouraging I hope to see it pushed quickly...

@Afforess Oh ok, I thought you meant using the original Location object with a different World class. That's indeed not possible in Java. Changing the Location to my own version would require about 100 changes to my plugin code, so i'm gonna pass

 

@eisental I actually ran into this problem ages ago while porting Stargate, but didn't take the time to dig through the Bukkit core to see what was causing it. You noticing that there were actually two instances of the world object gave me a better idea of where to look though, so it was only ~8 files I needed to look through instead of most of the source Haha. Well, that and I've been slowly reading over most of the Bukkit source while I develop just so I know how things work.
Other than the WORLD_LOADED event not being called with my initial commit I haven't run into any errors with this code change, and that was fixed in the second commit.

 

Shouldn't your class also be overriding int hashCode()? Given two objects for which equals() == true, hashCode() must return the same value.

This brings up a broader issue - As plugin developers, we really don't know which of bukkit's objects are compatible with HashSet and HashMap. Really, should we be writing wrappers for everything we put into a hash table?

I hate to make more work for the bukkit devs, but I have to say that bukkit's classes should either be made hash compatible, or should be documented.

 

Worlds (with my pull), Locations and Entitys are hash compatible. There are just some bugs still, like with how a world was being created twice. When I'm doing a HashMap of blocks, normally I just make a wrapper class.