¿ This code is efficient ?

Hi, im trying to check if a player is inside an area with this :

Border class :

Code:java

 
public class Border {
 
    private Vector p1;
    private Vector p2;
 
    public Border(Vector p1, Vector p2) {
        int x1 = Math.min(p1.getBlockX(), p2.getBlockX());
        int y1 = Math.min(p1.getBlockY(), p2.getBlockY());
        int z1 = Math.min(p1.getBlockZ(), p2.getBlockZ());
        int x2 = Math.max(p1.getBlockX(), p2.getBlockX());
        int y2 = Math.max(p1.getBlockY(), p2.getBlockY());
        int z2 = Math.max(p1.getBlockZ(), p2.getBlockZ());
        this.p1 = new Vector(x1, y1, z1);
        this.p2 = new Vector(x2, y2, z2);
    }
 
    public boolean contains(Location loc) {
        if(loc == null) {
            return false;
        }
        return loc.getBlockX() >= p1.getBlockX() && loc.getBlockX() <= p2.getBlockX()
                && loc.getBlockY() >= p1.getBlockY() && loc.getBlockY() <= p2.getBlockY()
                && loc.getBlockZ() >= p1.getBlockZ() && loc.getBlockZ() <= p2.getBlockZ();
    }
 
}
 

Event class :

Code:java

 
@EventHandler
public void onMove(PlayerMoveEvent event) {
    if(event.getFrom().getX() == event.getTo().getX() && event.getFrom().getBlockY() == event.getTo().getBlockY() && event.getFrom().getBlockZ() == event.getTo().getZ()) {
        return;
    }
    if(Koth.koths.isEmpty()) { return; }
 
    Player player = event.getPlayer();
    Boolean IsIn = false;
    Koth koth = null;
    for(Koth k : Koth.koths) {
        for(UUID uuid : k.getPlayers()) {
            if(uuid == player.getUniqueId()) {
                IsIn = true;
                koth = k;
                break;
            }
        }
    }
 
    if(IsIn && koth != null) {
        Location l1 = koth.getFirstLocation();
        Location l2 = koth.getSecondLocation();
 
        Border border = new Border(new Vector(l1.getX(), l1.getY(), l1.getZ()), new Vector(l2.getX(), l2.getY(), l2.getZ()));
        if(!border.contains(event.getTo())) {
            KothsManager.getManager().removePlayer(player, koth);
            s(player, KothMain.prefix + "&7Leaving the koth &b"+koth.getName());
        }
    } else {
        for(Koth k : Koth.koths) {
            Location l1 = k.getFirstLocation();
            Location l2 = k.getSecondLocation();
            Border border = new Border(new Vector(l1.getX(), l1.getY(), l1.getZ()), new Vector(l2.getX(), l2.getY(), l2.getZ()));
            if(border.contains(event.getTo())) {
                KothsManager.getManager().addPlayers(player, k);
                s(player, KothMain.prefix + "&7Entering koth &b"+k.getName());
                break;
            }
        }
    }
}
 

I wonder if there's any other way to do this more efficient.

 

Hmm generally speaking, move event is not the most efficient thing :3 but in your case it is the most decent way to deal with regions.
What is Koth#arenaObjects ?

 

That returns all Koths == Arenas, ¿ i should use a runnable instead of PlayerMoveEvent ?

 

@MaTaMoR_ i dont think so, runnable runs regardless to if player is moving. the thing that matters now is the case. in a server with 50 players afk (or moving just by yaw and pitch) then the runnable will be a pain with no reason, whereas in a server with 50 players running arround it might be better than an event called about a milion tims :/ idk what to say. Smarter guys like @teej107 might have a better solution :3
Okay, genera;;y im suspicius of static xD i have seen it miss used a bilion times with people puting static in arena classes and woops all objects (arenas) have the same names and characteristics -_- xD
good.

 

@MaTaMoR_, @mine-care PlayerMoveEvent gets runned alot. You are probably better off making your own class that implements Runnable. That has a "run" section. Every so-many ticks you call that event. I'd call it every second to two seconds.

!!! UNTESTED CODE !!!

Code:

public class CheckPlayers implements Runnable{
   @Override
   public void run( ){
      Iterator it = Bukkit.getServer().getOnlinePlayers().Iterator();
      while( it.hasNext( ) ){
         Player player = it.next();
         // Run your code to check if player is in border here
         // Make sure to use "player" and not it.next(), as that would break the loop and may cause NPE and could skip players
      }
   }
}

Then in your main class (DEPRECATED CODE (Works though))

Code:

    @Override
    public void onEnable(){
       long checkPlayersTickDelay = 20L;
       getServer().getScheduler().runTaskTimer(this, new CheckPlayers( ), 0L, checkPlayerTickDelay);
       // Your normal onEnable code of course
    }

 

@rickydaan ^

 

@mine-care You can add in a hashmap with Player and Location, and check if the positions are the same. Uses more RAM and saves CPU.

 

@rickydaan Player? then it is the ultimate ram leak.... especially when not handled properly :/ And what is the reason for this if he adds the proper checks to "filter" the move event as he already did

Then there is no reason to mess with runnables.

 

When I made a KingOfTheHill-like game I used a runnable every 2 seconds, I just checked the players in game and if they were in or not, I found it better since it was for a server with lots of people, and lost of people moving it's more overkill than a simple runnable in this case. I'd go with runnable for this.

 

@MaTaMoR_

1. Your Border class is not needed. Everything you need can be found in the Vector class. http://docs.codelanx.com/Bukkit/1.7...rg.bukkit.util.Vector-org.bukkit.util.Vector-
2. PlayerMoveEvent does get called a lot. Sometimes it is better to schedule a task because the users won't see a difference, but it will be easier on the server. However, if you value precision, use the PlayerMoveEvent. Just don't do any heavy tasks in the PlayerMoveEvent.

If it was up to me, I would probably go with using a scheduler since it seems you are looping through quite a bit of things.

 

Hmm. Now if you are okay with using round regions, this can be done a lot easier using the distance from the center squared.

 

@Tecno_Wizard Circles are the root of all evil