Skip to content

Commit

Permalink
Made Location less annoying to use. Too bad Location can't simply inh…
Browse files Browse the repository at this point in the history 
…erit Vector at this point without breaking things.
  • Loading branch information
@sk89q
sk89q committed Jun 10, 2011
1 parent 46d4e20 commit 2f06bec
Showing 1 changed file with 161 additions and 15 deletions.
176 changes: 161 additions & 15 deletions src/main/java/org/bukkit/Location.java
View file
Expand Up @@ -212,6 +212,167 @@ public Vector getDirection() {

return vector;
}

/**
* Adds the location by another.
*
* @see Vector
* @param vec
* @return the same location
* @throws IllegalArgumentException for differing worlds
*/
public Location add(Location vec) {
if (vec == null || vec.getWorld() != getWorld()) {
throw new IllegalArgumentException("Cannot add Locations of differing worlds");
}

x += vec.x;
y += vec.y;
z += vec.z;
return this;
}

/**
* Adds the location by another. Not world-aware.
*
* @see Vector
* @param x
* @param y
* @param z
* @return the same location
*/
public Location add(double x, double y, double z) {
this.x += x;
this.y += y;
this.z += z;
return this;
}

/**
* Subtracts the location by another.
*
* @see Vector
* @param vec
* @return the same location
* @throws IllegalArgumentException for differing worlds
*/
public Location subtract(Location vec) {
if (vec == null || vec.getWorld() != getWorld()) {
throw new IllegalArgumentException("Cannot add Locations of differing worlds");
}

x -= vec.x;
y -= vec.y;
z -= vec.z;
return this;
}

/**
* Subtracts the location by another. Not world-aware and
* orientation independent.
*
* @see Vector
* @param x
* @param y
* @param z
* @return the same location
*/
public Location subtract(double x, double y, double z) {
this.x -= x;
this.y -= y;
this.z -= z;
return this;
}

/**
* Gets the magnitude of the location, defined as sqrt(x^2+y^2+z^2). The value
* of this method is not cached and uses a costly square-root function, so
* do not repeatedly call this method to get the location's magnitude. NaN
* will be returned if the inner result of the sqrt() function overflows,
* which will be caused if the length is too long. Not world-aware and
* orientation independent.
*
* @see Vector
* @return the magnitude
*/
public double length() {
return Math.sqrt(Math.pow(x, 2) + Math.pow(y, 2) + Math.pow(z, 2));
}

/**
* Gets the magnitude of the location squared. Not world-aware and
* orientation independent.
*
* @see Vector
* @return the magnitude
*/
public double lengthSquared() {
return Math.pow(x, 2) + Math.pow(y, 2) + Math.pow(z, 2);
}

/**
* Get the distance between this location and another. The value
* of this method is not cached and uses a costly square-root function, so
* do not repeatedly call this method to get the location's magnitude. NaN
* will be returned if the inner result of the sqrt() function overflows,
* which will be caused if the distance is too long.
*
* @see Vector
* @param o
* @return the distance
* @throws IllegalArgumentException for differing worlds
*/
public double distance(Location o) {
if (o == null || o.getWorld() != getWorld()) {
throw new IllegalArgumentException("Cannot measure distance between worlds or to null");
}

return Math.sqrt(Math.pow(x - o.x, 2) + Math.pow(y - o.y, 2) + Math.pow(z - o.z, 2));
}

/**
* Get the squared distance between this location and another.
*
* @see Vector
* @param o
* @return the distance
* @throws IllegalArgumentException for differing worlds
*/
public double distanceSquared(Location o) {
if (o == null || o.getWorld() != getWorld()) {
throw new IllegalArgumentException("Cannot measure distance between worlds or to null");
}

return Math.pow(x - o.x, 2) + Math.pow(y - o.y, 2) + Math.pow(z - o.z, 2);
}

/**
* Performs scalar multiplication, multiplying all components with a scalar.
* Not world-aware.
*
* @param m
* @see Vector
* @return the same location
*/
public Location multiply(double m) {
x *= m;
y *= m;
z *= m;
return this;
}

/**
* Zero this location's components. Not world-aware.
*
* @see Vector
* @return the same location
*/
public Location zero() {
x = 0;
y = 0;
z = 0;
return this;
}

@Override
public boolean equals(Object obj) {
Expand Down Expand Up @@ -298,19 +459,4 @@ public Location clone() {
public static int locToBlock(double loc) {
return (int) Math.floor(loc);
}

/**
* Retrieve the distance between two locations in a world.
*
* @param loc the Location to calculate the distance to
* @return the distance between this location and the parameter
* @throws IllegalArgumentException if the location parameter is null or represents a location in a different world
*/
public double distanceTo(Location loc) throws IllegalArgumentException {
if (loc == null || loc.getWorld() != getWorld()) {
throw new IllegalArgumentException("Cannot measure distance between worlds or to null");
}

return toVector().distance(loc.toVector());
}
}

12 comments on commit 2f06bec

@Afforess
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too bad Location and Vector are not interfaces. I'd say go ahead and break them if you also changed them to interfaces.

@eisental
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vote for breaking as well. A BlockLocation that somehow extends or implements Location for working with integer locations would also help a lot.

@Afforess
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I was thinking of writing a class that both implemented Vector and Location simultanouesly. If they were interfaces, I could.

@xZise
Copy link

@xZise xZise commented on 2f06bec Jun 10, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And what should BlockLocation do? A Location but instead of doubles with integers? And isn't it possible to create a subclass of Vector with the same functionally as Location does?

Fabian

@Afforess
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fabian

Sure, I could create a subclass of a vector that acted exactly like a location. But say I wanted to pass that to a 2nd plugin, that did not have my class in it. It'd treat it like a vector.

With an interface, I could use them interchangably, and design it however I decided. Polymorphism. Interfaces > Classes.

@Afforess
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, esiental's idea of an integer only Location or Vector is not a bad one. Integer math is many orders of magnitude faster than floating point, and if you are not interested in the extra percision (e.g only looking at integer coords anyway), the speed up is considerable. Also, sqrt and power functions are MUCH MUCH faster with integers (if you design them yourself, at any rate)

@eisental
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding BlockLocation would have the same rationale as BlockVector does. As Afforess said, a standard way to pass block coordinates. The problem is that the Location interface would anyway need method declarations that use doubles and return doubles so I'm not sure how to prevent this conversion.
Also, creating a new object instance by using Location.toVector() whenever I want to make a vector calculation of a Location object coordinates is very wasteful. I use static methods for that, modifying existing objects instead.

@eisental
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see that's the purpose of this commit (not making new instances). Thanks a lot :)

@krsmes
Copy link

@krsmes krsmes commented on 2f06bec Jun 10, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

    interface WorldSpecific {
        public World getWorld();
    }

    interface EntityLocation {
        public double getX();
        public double getY();
        public double getZ();
        ...
    }

    interface BlockLocation {
        public int getBlockX();
        public int getBlockY();
        public int getBlockZ();
        ...
    }

    class Location implements WorldSpecific, EntityLocation, BlockLocation {
        ...
    }

    class Vector implements EntityLocation, BlockLocation {
        ...
    }

    class AnyLocation implements EntityLocation, BlockLocation {
        public AnyLocation(EntityLocation l) {}
        public AnyLocation(BlockLocation l) {}
        public AnyLocation(int x, int y, int z) {}
        public AnyLocation(double x, double y, double z) {}
    }

    class AnyWorldLocation extends AnyLocation implements WorldSpecific {
        public AnyLocation(WorldSpecific w, AnyLocation l) {}
        public AnyLocation(WorldSpecific w, EntityLocation l) {}
        public AnyLocation(WorldSpecific w, BlockLocation l) {}
        public AnyLocation(WorldSpecific w, int x, int y, int z) {}
        public AnyLocation(WorldSpecific w, double x, double y, double z) {}
    }

Location and Vector should disappear … or just become 'static' classes (like java.util.Math, etc) with a bunch of utility methods for doing Location/Vector calculations. Block and Entity should implement BlockLocation and EntityLocation instead of having to have separate object instances for these values. Something like an AnyLocation could exist as a value holder apart from a specific Block or Entity — or Vector could be kept around for that purpose but it really is a poor naming choice.

@xZise
Copy link

@xZise xZise commented on 2f06bec Jun 11, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea, but for example the spawn... methods in the world interface. What location type do they accept? Both? Then you need for many methods which uses locations two methods. Or could you convert from a block location to an entity location? But then you have to create two objects every time, which reduces the advantages by using integers. Or could the block location act like an entity location? Then you have to separate the modifier methods from the getter methods. Meaning: Both location types return the same values (both int and doubles) but support only one modifier type (a integer location couldn't be modified with a double location.

And where is the advantage of supporting a integer and double constructor? Isn't the integer constructor covered by the double constructor?

I uploaded a gist example which also supports a fixed location (allows final location variables). At the moment there is no world and yaw/pitch support. This is only a idea how to accomplish this. The spawn…-methods then don't accept a Location or something like this instead they accept the “LocationGetter” variable (so allows block based, entity based and fixed locations).

Okay based on the gist I uploaded already some files. It is mostly done, and in CraftBukkit and Bukkit are only warnings about this. The only problem could be other plugins using the classes Location, Vector and BlockVector. Please report any problems/ideas/feature request to me.

I also added changes for CraftBukkit, which uses the new location types. On the new-locations branch it don't break any API. On the branch new-locations-break I included the locations types fully. But this will break the API as some return types changes.

On CraftBukkit: new-locations and new-locations-break

Fabian

@krsmes
Copy link

@krsmes krsmes commented on 2f06bec Jun 11, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice job Fabian… I like it. The only (very minor) suggestion I would have would be some name changes:

  • DirectionalLocationGetter to Directional
  • DirectionalLocation to MutableDirectional and have it extend Directional…
  • on the "break existing code" version LocationGetter mind as well just be Location with LocationModifier becoming MutableLocation (Directional could even extend Location)
  • FixedIntLocation and FixedDoubleLocation could be a single "FixedLocation implements Location"

@xZise
Copy link

@xZise xZise commented on 2f06bec Jun 12, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks ;) but I have some questions:

  • What is DirectionLocationGetter?
  • The DirectionalLocation class/interface? Is this the Directional interface (as it is mutable?). This is extending DirectionalGetter.
  • What do you mean here? If the location is simply a x,y,z (e.g. World w.getBlockAt(LocationGetter)) I used LocationGetter, as this is the lowest class. This doesn't need a world/directional definition. And it isn't relevant if it is mutable. Also have everything that will change the location also a setter (as you could react only on a setter and not “location change”).
  • And then? I couldn't say: T is either a double or a integer. So I couldn't calculate with variables of type T. And the FixedIntLocation shouldn't store the location as double, as we want to use faster integer calculations.

Also I will rename the class files in a more consistent way:

  • LocationGetter → Location
  • WorldSpecificGetter → WorldLocation
  • WorldSpecific → MutableWorldLocation
  • DirectionalGetter → Directional(Location)
  • Directional → MutableDirectional(Location)
  • Fixed/Mutable location classes → <Fixed/""><Block/Entity><World/Directional/"">Location

Also these classes are optimized on speed and not on the dry principle. It is possible to create a abstract class, which contain the implementations of the methods and using the getter (getX, getY, …) and not the variables directly. This is maybe slower (don't know if the JVM is optimizing anything there) and couldn't differ between integer and double implementation. So it couldn't profit from the faster integer calculations.

Fabian

Please sign in to comment.