First Script! (Feedback wanted)

[JoshuaMiddleton]

So basically I've decided to start creating my own script for personal use, very basic script to Runecraft air runes, let me know what you think/any general improvements moving forwards, thanks.

import org.osbot.rs07.api.map.Area;
import org.osbot.rs07.api.map.Position;
import org.osbot.rs07.api.map.constants.Banks;
import org.osbot.rs07.script.Script;
import org.osbot.rs07.script.ScriptManifest;

@ScriptManifest(author = "Jashy", info = "Air Runecrafter - First Script.", logo = "", version = 0.1, name = "Air Runecrafter")
public final class Air extends Script  {

    private final Area AIR_ALTAR = new Area(2989,3295,2982,3289);
    private final Area AIR_RUNES = new Area(2839, 4826, 2849, 4840);

    @Override
    public final int onLoop() throws InterruptedException {
        if (hasPureEssense()) {
            runecraft();
        } else {
            bank();
        }
        return random(2500, 4000);
    }

    private void runecraft() throws InterruptedException {
        if(!AIR_ALTAR.contains(myPosition())) {
            getWalking().webWalk(new Position(2987, 3294, 0));
            sleep(random(2500, 4000));
            useAltar();
            sleep(random(2500, 4000));
        }
        craftRunes();
        sleep(random(3500, 5000));
    }

    private boolean hasPureEssense() {
        return getInventory().contains("Pure essence");
    }

    private boolean useAltar() {
        return getObjects().closest("Mysterious ruins").interact("Enter");
    }

    private boolean craftRunes() {
        return getObjects().closest("Altar").interact("Craft-rune");
    }

    private boolean usePortal() {
        return getObjects().closest("Portal").interact("Use");
    }

    private void bank() throws InterruptedException {
        //Added this in case script was started inside air runes area.
        while(AIR_RUNES.contains(myPosition())) {
            usePortal();
            sleep(random(1500, 2500));
        }
        if (!Banks.FALADOR_EAST.contains(myPosition())) {
            getWalking().webWalk(Banks.FALADOR_EAST);
        } else if (!getBank().isOpen()) {
            getBank().open();
            sleep(random(2000, 3000));
        } else if (!getInventory().isEmptyExcept("Pure essence")) {
            getBank().depositAll();
            sleep(random(2000, 3000));
        } else if (getBank().contains("Pure essence")) {
            getBank().withdrawAll("Pure essence");
            sleep(random(2000, 3000));
        } else {
            stop(true);
        }
    }
}

 

[Apaec]

Really nice!

This code is, for the most part, well structured, readable and tidy. It's stateless and conditional which is nice.

A few suggestions:

• Replace the static sleeps with conditional sleeps. This will ensure you only sleep as long as you have to.
• Null check the altar, portal and ruins, otherwise you'll end up with some nasty client crashing errors. You could always write a method to wrap that stuff in an Optional so you don't forget.
• Make the runecraft() method conditional, at the moment it is not

Other than that, well done - this is great for a first script

Apa

[JoshuaMiddleton]

1 hour ago, Apaec said:

Really nice!

This code is, for the most part, well structured, readable and tidy. It's stateless and conditional which is nice.

A few suggestions:

• Replace the static sleeps with conditional sleeps. This will ensure you only sleep as long as you have to.
• Null check the altar, portal and ruins, otherwise you'll end up with some nasty client crashing errors. You could always write a method to wrap that stuff in an Optional so you don't forget.
• Make the runecraft() method conditional, at the moment it is not

Other than that, well done - this is great for a first script

Apa

Thanks for the feedback!

Adressing some of the points you made:

 

make runecraft() method conditional (now called airRunecraft())

    private void airRunecraft() throws InterruptedException {
        if (!AIR_ALTAR.contains(myPosition())) {
            getWalking().webWalk(new Position(2987, 3294, 0));
            sleep(random(2500, 4000));
        }
        if (AIR_ALTAR.contains(myPosition())) {
            useAltar();
            sleep(random(2500, 4000));
        }
        if (AIR_RUNES.contains(myPosition())) {
            craftRunes();
            sleep(random(3500, 5000));
        }
        while (AIR_RUNES.contains(myPosition())) {
            usePortal();
            sleep(random(1500, 2500));
        }
    }

 

Could you elaborate on null checking objects and optional wrapping please?

Also, I was under the impression that having static (random) sleeps was better simply to create random gaps inbetween tasks to increase anti-ban percentage? Could you elaborate on why you would have conditional sleeps/better ways to create random breaks inbetween tasks?

 

Thanks

Edited May 21, 2019 by JoshuaMiddleton

[ProjectPact]

Be careful with while loops, they can sometimes bite you in the butt

[JoshuaMiddleton]

10 minutes ago, ProjectPact said:

Be careful with while loops, they can sometimes bite you in the butt

Can't think of a better way to check if player is still at the Air altar.

Guess the only other option would be to check if the webwalker can find a way to the Falador bank?

[Apaec]

1 hour ago, JoshuaMiddleton said:

Thanks for the feedback!

Adressing some of the points you made:

 

make runecraft() method conditional (now called airRunecraft())

    private void airRunecraft() throws InterruptedException {
        if (!AIR_ALTAR.contains(myPosition())) {
            getWalking().webWalk(new Position(2987, 3294, 0));
            sleep(random(2500, 4000));
            useAltar();
            sleep(random(2500, 4000));
        }
        if (AIR_ALTAR.contains(myPosition())) {
            craftRunes();
            sleep(random(3500, 5000));
        }
        while (AIR_RUNES.contains(myPosition())) {
            usePortal();
            sleep(random(1500, 2500));
        }
    }

 

Could you elaborate on null checking objects and optional wrapping please?

Also, I was under the impression that having static (random) sleeps was better simply to create random gaps inbetween tasks to increase anti-ban percentage? Could you elaborate on why you would have conditional sleeps/better ways to create random breaks inbetween tasks?

 

Thanks

Your changes:

When I said conditional, I meant more that only one line would run with any given onLoop iteration. The reason for this is that the API interacts with the live game - you cannot expect any API call to return successfully every time and as a result chained API calls which depend on one another (for example, opening the bank and then depositing an item) can fail. This is relevant in your code - you are calling getWalking#webWalk and immediately after you are interacting with the altar. What if the walking failed (for whatever reason)? Then you wouldn't be in range of the altar and the script would be stuck.

Null checking:

As for null checking, at the moment you are grabbing and interacting with the game objects as follows:

getObjects().closest("Altar").interact("Craft-rune");

This is fine if getObjects().closest("Altar") always returned a game object, however this is not the case. If closest did not find any object matching your filter, it will simply return null, at which point you are calling 'interact' on null which will result in a Null-pointer error (for obvious reasons).

Instead, do something like this:

private boolean someMethod() {
  RS2Object altar = getObjects.closest("Altar");
  return altar != null && altar.interact("Craft-rune");
}

Sleeps:

Again, the script is interacting with a live game, so there are all sorts of barriers between your script and game state. One of these is latency, fluctuations in which mean timing in your script is never going to be perfect. As a result, adding random sleeps here and there gives a false sense of security. Instead, to prevent bans, focus on making your script as reliable as possible, and then use the script carefully and keep a low profile. If you do add any randomisation, doing so in interaction timing is probably not the place. To make the script reliable, conditional sleeps are needed to retry actions only when needed, as well as to maximise script responsiveness (minimise the time the script spends sleeping, and maximise the time it is aware of game state).

Stick with conditional sleeps

---------------------

Good luck!

-Apa

Edit: 

Also, as @ProjectPact mentioned, avoid while loops. Why would you need a while loop if you already have a perfectly good one: onLoop()?

Edited May 21, 2019 by Apaec