Jeef_ Posted April 2, 2018 Share Posted April 2, 2018 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; } } Quote Link to comment Share on other sites More sharing options...
d0zza Posted April 2, 2018 Share Posted April 2, 2018 NPC cow = getNpcs().closest("Cow", "Cow calf"); That line is causing your script to not start. You need to call it in your onLoop(). 2 Quote Link to comment Share on other sites More sharing options...
Jeef_ Posted April 2, 2018 Author Share Posted April 2, 2018 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. Quote Link to comment Share on other sites More sharing options...
Alek Posted April 3, 2018 Share Posted April 3, 2018 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. 3 Quote Link to comment Share on other sites More sharing options...
Jeef_ Posted April 3, 2018 Author Share Posted April 3, 2018 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. Quote Link to comment Share on other sites More sharing options...
battleguard Posted April 4, 2018 Share Posted April 4, 2018 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! 3 Quote Link to comment Share on other sites More sharing options...
Alek Posted April 4, 2018 Share Posted April 4, 2018 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. Quote Link to comment Share on other sites More sharing options...