Creating a Botting Script ( But not running on client)

[OSBOTTESTER101]

public class Attack {
    private static Script sI;
    NPC npc = sI.getNpcs().closest("Rat");
    public void getAllNpcs(Script sI, String npcsName){
        if(!sI.myPlayer().isUnderAttack()){
            if(npc.exists()){
                sI.npcs.closest(npcsName).interact("Attack");
            }else{
                sI.log("Cant find the current NPCS " + npcsName);
            }
        }
    }
}

Above is my code and this is only the " ATTACK class, that creates a getAllNpcs  which when is suppose to attack the given String npcsName

now before I had a collection of Npcs that is why you see the method ask for a given String 
basically it was like this 

private static List<NPC> npc = sI.npcs.getAll();

rather then the method you use above
anyways am I doing something wrong ? because when I click play and try to use the bot script it does nothing and after I do I can't even click on the osrs game anymore it is basically like if there were a JFrame over my game not letting me click anything on that screen after I try to run the unsucessful script

 

Anyways I was thinking that maybe I should use a constructor to pass the script instance? Such as

 

public class Attack {
    private Script sI;
	public Attack(Script sI){
		this.sI = sI;
}
    //private static List<NPC> npc = sI.npcs.getAll();
    NPC npc = sI.getNpcs().closest("Rat");
    public void getAllNpcs(Script sI, String npcsName){
        if(!sI.myPlayer().isUnderAttack()){
            if(npc.exists()){
                sI.npcs.closest(npcsName).interact("Attack");
            }else{
                sI.log("Cant find the current NPCS " + npcsName);
            }
        }
    }
}

and below I will give you my main method

 

 

 

 

public int onLoop ()  throws InterruptedException{

        switch (getState()){
            case EAT:
                print(getState());
                Eat.eatFood();
                break;
            case ATTACK:
                print(getState());
                attack.getAllNpcs(this, "Rat");
                break;
            case BANK:
                print(getState());
                bank.bankMode(this, Banks.VARROCK_WEST);
                break;

        }

        return random(3000);

    }

private States getState(){

        if(myPlayer().getHealthPercent()<20){
                return
                        States.EAT;

        }else if(!myPlayer().isUnderAttack()){
                return
                        States.ATTACK;

        }else

            return States.BANK;
// do not mind the return States.BANK as I have not finished the code but all I was trying to do is make the player attack is all and my player is not under attack so it should return States.attack		
                                            

 

 

Oh yes by the way I have never created States or Classes that have separate task, this is my first time and that is why maybe it is not working for me 

 

but if the code looks fine to everyone than maybe I am not setting the build path correctly and I am using IntelliJ

Edited June 20, 2020 by OSBOTTESTER101

[Token]

Don't call API methods before the onStart() executes, so nothing that assigns the result to class fields or in constructors/initializer blocks

[OSBOTTESTER101]

On 6/20/2020 at 1:25 PM, Token said:

Don't call API methods before the onStart() executes, so nothing that assigns the result to class fields or in constructors/initializer blocks

What do you mean exactly because I did not call anything on the onStart method I just used onLoop method

[Camaro]

You're generating a null pointer when you run the script.

NPC npc = sI.getNpcs().closest("Rat");

This will initialize before the constructor code runs, so the script object never actually gets set. This will cause your script to crash.

if(npc.exists()){
  sI.npcs.closest(npcsName).interact("Attack");

There are also multiple issues in these two lines. Since the 'npc' variable is only set at the beginning of the script, it never gets updated. exists() will always return false once that npc despawns. 

You are also not null checking anything. npcs.closest(npcsName) will return null when no npcs are found, which will crash your script.

Since this is such a small amount of code, you don't really need to create a separate class for it yet. Start with just calling things within onloop until you understand how a script works more.

case ATTACK:
  if (!myPlayer().isUnderAttack()) {
    NPC rat = getNpcs().closest("Rat");
    if (rat != null && rat.interack("Attack")) {
      Sleep.sleepUntil(() -> myPlayer().isUnderAttack(), 5000);
    }
  }
  break;

This should be enough for your attack state.

Notice how I use a conditional sleep after attacking the rat. If you don't, then the bot will spam-click the rat until you are actually under attack. This is an extremely necessary thing to learn for writing efficient scripts.