Jump to content
Sign in to follow this  
Jeef_

So.. My Script Not Starting

Recommended Posts

Okay, So this is my second Script. First one is a simple Shrimp catcher at and it does the basic it fishes and drops.

Trying to make a combat script that loots and banks but I don't understand why my code doesn't even start up, Literally does nothing.

import java.util.Objects;

import org.osbot.rs07.api.GroundItems;
import org.osbot.rs07.api.Inventory;
import org.osbot.rs07.api.map.Area;
import org.osbot.rs07.api.model.GroundItem;
import org.osbot.rs07.api.model.NPC;
import org.osbot.rs07.api.model.RS2Object;
import org.osbot.rs07.script.Script;
import org.osbot.rs07.script.ScriptManifest;

@ScriptManifest(author = "Jeef_", info = "Kills Cows and Banks", name = "CowKiller", version = 0, logo = "")
public class main extends Script{
	
	Area Cowpen = new Area(3253, 3255, 3265, 3296).setPlane(0);
	Area lumbank = new Area(3208, 3218, 3210, 3220).setPlane(2); 
	
	
	
	NPC cow = getNpcs().closest("Cow", "Cow calf");
	
	public void onStart() {
		
	}
	
	public enum State {
		WALK, BANK, KILL, WAIT;
	}
	
	public State getState() {
		
		if (getInventory().isFull()) { // FULL COWHIDE
			return State.BANK;
		}
		if (!Cowpen.contains(myPlayer())) { // IF I'M NOT AT THE COWPEN
			return State.WALK;
		}
		if (cow != null && cow.exists() && cow.isAttackable() && !inventory.isFull()) { //KILLING COWS
			return State.KILL;
		}
		if (myPlayer().isAnimating()) { // PROGRESS KILLING COWS
			return State.WAIT;
		}
		
		return null;
		
	}

	@Override
	public int onLoop() throws InterruptedException {
		
		switch (getState()) {
		
		case BANK:
			RS2Object banker = getObjects().closest("Bank booth");
			if (getWalking().webWalk(lumbank)) {
				if (banker != null) {
					if (banker.interact("Bank")) {
						if (bank.isOpen()) {
							if (bank.depositAll()) {
								Timing.waitCondition(() -> inventory.isEmpty(), random (2000, 5000));
							}
						}
					}
				}
			}
			break;
			
		case WALK:
			if (getWalking().webWalk(Cowpen)) {
				Timing.waitCondition(() -> Cowpen.contains(myPlayer()), random(2000, 5000));
			}
			break;
			
		case KILL:
			GroundItem hide = getGroundItems().closest("Cowhide");
			if (cow.interact("Attack")) {
				Timing.waitCondition(() -> myPlayer().isUnderAttack() || myPlayer().isAnimating(), random(2000, 3000));
				if (!inventory.isFull()) {
					hide.interact("Take");
					sleep (random(500,900));
				}
			}
			break;
			
		case WAIT:
			sleep (random(500, 900));
			break;
		
		}
		
		return 0;
	}

}

 

Share this post


Link to post
Share on other sites
11 hours ago, d0zza said:

NPC cow = getNpcs().closest("Cow", "Cow calf");

That line is causing your script to not start. You need to call it in your onLoop().

I moved it to the onLoop(), but I also had to add it in my getState(). Is it supposed to be like that? Because it seems redundant.

Share this post


Link to post
Share on other sites

States aren't bad, especially for this level of "abstraction" - I actually use them in a few of my scripts. There was a redundancy concern but I don't see that in your original concern you posted above.


The only recommendation I have is to not return a null in your getState(), use something like "State.IDLE" - this will save you at some point! Now if your script was bigger, like if it had tanning support, perhaps "Tasks" would be better than states. If you had less, perhaps no banking at all, I'd suggest removing your states and just including everything in your onLoop.

Just remember the more you abstract, the more the computer has to work. In your implementation, there's no performance concern at all. 
 

For the answer to your question, as others have stated, you need to get an NPC in your onLoop. Think about this logically.

Cow = getclosest("Cow");
Cow = alive
Player->Attack(Cow);
Cow = dead
Player->Attack(Cow); // ERROR: Cow doesnt exist

You must now get a new closest cow.

 

Now to remove this "redundancy" by searching for a new cow in both the getState() and onLoop(), think about it more logically (again). How about you combine the attack/search state?

If your inventory isnt full, you're not animating, and you're in the cow pen, return State.ATTACK.

In your attack state code, here you can implement searching for the cow and attacking it.

  • Like 3

Share this post


Link to post
Share on other sites
5 hours ago, Alek said:

States aren't bad, especially for this level of "abstraction" - I actually use them in a few of my scripts. There was a redundancy concern but I don't see that in your original concern you posted above.


The only recommendation I have is to not return a null in your getState(), use something like "State.IDLE" - this will save you at some point! Now if your script was bigger, like if it had tanning support, perhaps "Tasks" would be better than states. If you had less, perhaps no banking at all, I'd suggest removing your states and just including everything in your onLoop.

Just remember the more you abstract, the more the computer has to work. In your implementation, there's no performance concern at all. 
 

For the answer to your question, as others have stated, you need to get an NPC in your onLoop. Think about this logically.

Cow = getclosest("Cow");
Cow = alive
Player->Attack(Cow);
Cow = dead
Player->Attack(Cow); // ERROR: Cow doesnt exist

You must now get a new closest cow.

 

Now to remove this "redundancy" by searching for a new cow in both the getState() and onLoop(), think about it more logically (again). How about you combine the attack/search state?

If your inventory isnt full, you're not animating, and you're in the cow pen, return State.ATTACK.

In your attack state code, here you can implement searching for the cow and attacking it.

Oh I remember your name, you made the Macro Killer I believe.

So I changed up a few things.

public enum State {
		WALK, BANK, KILL, WAIT, IDLE;
	}

public State getState() {
		
		NPC cow = getNpcs().closest("Cow", "Cow calf");
		
		if (cow != null && cow.exists() && cow.isAttackable() && !myPlayer().isUnderAttack()) { //KILLING COWS
			return State.KILL;
		}
		if (myPlayer().isAnimating()) { // PROGRESS KILLING COWS
			return State.WAIT;
		}
		
		return State.IDLE;
		

public int onLoop() throws InterruptedException {
		
		NPC cow = getNpcs().closest("Cow", "Cow calf");
		
		switch (getState()) {
		
		case KILL:
			
			if (myPlayer().isAnimating()) {
				lastAnimation = System.currentTimeMillis();
			} else if (System.currentTimeMillis() > (lastAnimation + 2500)) {
				if (cow.interact("Attack")) {
					Timing.waitCondition(() -> myPlayer().isUnderAttack(), random(2000, 3000));
				}
			}
			
			break;
			
		case WAIT:
			sleep (random(500,900));
			break;
		
		}
	return 0;
	}

Just showed what I thought would be important.

My Logic behind this, and please correct me if I'm wrong - which I'm sure I am.

  • If an Attackable Cow is there, It'll return State.KILL
  • That cow becomes the target and then Player interacts "Attack ".
  • Player is now underattack so it won't return State.Kill, but State.WAIT
  • After Cow is dead It'll search for a new cow and cycle repeats.

I thought it would work like this but there are multiple issues I'm having.

Having performance issues where once the script is on, the client will be really laggy, sometimes to the point where I have to open task manager to even close the client.

Takes forever for me to even attack a Cow in the first place

Sometimes would spam click on the cow.

Share this post


Link to post
Share on other sites
11 hours ago, Alek said:

States aren't bad, especially for this level of "abstraction" - I actually use them in a few of my scripts. There was a redundancy concern but I don't see that in your original concern you posted above.


The only recommendation I have is to not return a null in your getState(), use something like "State.IDLE" - this will save you at some point! Now if your script was bigger, like if it had tanning support, perhaps "Tasks" would be better than states. If you had less, perhaps no banking at all, I'd suggest removing your states and just including everything in your onLoop.

Just remember the more you abstract, the more the computer has to work. In your implementation, there's no performance concern at all. 
 

For the answer to your question, as others have stated, you need to get an NPC in your onLoop. Think about this logically.

Cow = getclosest("Cow");
Cow = alive
Player->Attack(Cow);
Cow = dead
Player->Attack(Cow); // ERROR: Cow doesnt exist

You must now get a new closest cow.

 

Now to remove this "redundancy" by searching for a new cow in both the getState() and onLoop(), think about it more logically (again). How about you combine the attack/search state?

If your inventory isnt full, you're not animating, and you're in the cow pen, return State.ATTACK.

In your attack state code, here you can implement searching for the cow and attacking it.

Its nice to see a developer for a bot that actually takes the time to write very informative responses. I am sure you have seen these type of questions time and time again but you still tailor a unique response and dont just tell them to use the search function so kudos to you!

  • Like 3

Share this post


Link to post
Share on other sites
18 hours ago, Jeef_ said:

Oh I remember your name, you made the Macro Killer I believe.

So I changed up a few things.


public enum State {
		WALK, BANK, KILL, WAIT, IDLE;
	}

public State getState() {
		
		NPC cow = getNpcs().closest("Cow", "Cow calf");
		
		if (cow != null && cow.exists() && cow.isAttackable() && !myPlayer().isUnderAttack()) { //KILLING COWS
			return State.KILL;
		}
		if (myPlayer().isAnimating()) { // PROGRESS KILLING COWS
			return State.WAIT;
		}
		
		return State.IDLE;
		

public int onLoop() throws InterruptedException {
		
		NPC cow = getNpcs().closest("Cow", "Cow calf");
		
		switch (getState()) {
		
		case KILL:
			
			if (myPlayer().isAnimating()) {
				lastAnimation = System.currentTimeMillis();
			} else if (System.currentTimeMillis() > (lastAnimation + 2500)) {
				if (cow.interact("Attack")) {
					Timing.waitCondition(() -> myPlayer().isUnderAttack(), random(2000, 3000));
				}
			}
			
			break;
			
		case WAIT:
			sleep (random(500,900));
			break;
		
		}
	return 0;
	}

Just showed what I thought would be important.

My Logic behind this, and please correct me if I'm wrong - which I'm sure I am.

  • If an Attackable Cow is there, It'll return State.KILL
  • That cow becomes the target and then Player interacts "Attack ".
  • Player is now underattack so it won't return State.Kill, but State.WAIT
  • After Cow is dead It'll search for a new cow and cycle repeats.

I thought it would work like this but there are multiple issues I'm having.

Having performance issues where once the script is on, the client will be really laggy, sometimes to the point where I have to open task manager to even close the client.

Takes forever for me to even attack a Cow in the first place

Sometimes would spam click on the cow.

You don't need to search for a cow in your getState(). Just think of all the conditions necessary to attack a cow, then return the Attack state.

If my inventory isn't full, if I'm not animating, if I'm not in the bank, and if I'm in the cowpen < return Attack.

See how I didn't search for a cow?

Now in your attack state, you can search for the cow once. If the cow doesn't exist, sleep for 1000ms. The reason you want a long sleep is so you're not continually doing all these checks when it can take some time for you to find yourself a suitable target.

Share this post


Link to post
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

Sign in to follow this  

  • Recently Browsing   0 members

    No registered users viewing this page.

×
×
  • Create New...