Jump to content
×
×
  • Create New...

Pseudo's Script Writer Application


Pseudo
 Share

Recommended Posts

Hi bois. Currently stuck in isolation (thanks Omicron), so have popped out a few skrepts this morning that have been of benefit to myself. Suffice to say I'm dying of boredom and undoubtedly will be producing more over the coming days. It goes without saying that the two I've written today are extremely basic, but are fit for purpose, and allowed me a few hours to acclimatise somewhat to the Powbot API.

 

Github: https://github.com/ps-eu-do/Powbot-Mobile-

 

Cheers.

Link to post
Share on other sites

Hey, don't have a ton of time to go through this but here's a few things:
 

  • Your validateItem method is kind of redundant. The API is null safe when you use .first() so just check if item.valid(). 
  • Look into guard clauses https://refactoring.com/catalog/replaceNestedConditionalWithGuardClauses.html It will make your nested conditionals more readable. Also refer to my comments on this application: 
  • Following up on the previous point, with your logic nested, you're running into potential issues where the script would get stuck. For example, if your fishingSpot.inViewport() fails, you're script with just sit there. These little things are easier to catch with guard clauses. 
  • Your hasBait function accepts an InventoryItemSteam, but it never used.

 

Overall, a pretty decent job. Both scripts are fairly simple, so it would nice to see something a little bit more complex, including multiple files. I'll wait to see what others think though. 

  • Like 1
Link to post
Share on other sites

  • Moderators

Hi mate, thanks for the application.

I agree with ADivorcedFork on his points also. 

In terms of the application in general, both scripts are a little on the simpler side and would rather something more complex for SW. In terms of the scripts themselves, you should look at modularising them, moving your cases into tasks. Much easier to maintain and manage when it's not all smushed into one file.

These are a little rushed, as previously suggested, you have a check if the fishing Spot is in Viewport () but do nothing if it's not? You don't try and walk closer, move the camera or nothing, it's almost like you added the in-viewport check because you felt you had to but didn't really understand why.

In your swamp toad script, you're adding custom strings to the paint for inventory tracking and tracking those manually. 
Firstly, look at the track items option in the paint builder https://docs.powbot.org/#/Basic_Fundamentals/PaintBuilderAPI?id=track-items
Secondly, the way you track your items has little to no validation, you're tracking all items that enter your inventory. No matter what the item is, if you were to missclick and pick up some sort of junk item you're classing that as swamp toad increase. You should really validate what item is going into your inventory.

Overall, a lot of the issues would be solved with a little more TLC. However, if they were a little more complex, you'd find yourself having to add more strict validations etc. 

I'll keep this open for a while if you have anything more you wanted to put forward for your application.

  • Like 1
Link to post
Share on other sites

5 hours ago, ADivorcedFork said:

Hey, don't have a ton of time to go through this but here's a few things:
 

  • Your validateItem method is kind of redundant. The API is null safe when you use .first() so just check if item.valid(). 
  • Look into guard clauses https://refactoring.com/catalog/replaceNestedConditionalWithGuardClauses.html It will make your nested conditionals more readable. Also refer to my comments on this application: 
  • Following up on the previous point, with your logic nested, you're running into potential issues where the script would get stuck. For example, if your fishingSpot.inViewport() fails, you're script with just sit there. These little things are easier to catch with guard clauses. 
  • Your hasBait function accepts an InventoryItemSteam, but it never used.

 

Overall, a pretty decent job. Both scripts are fairly simple, so it would nice to see something a little bit more complex, including multiple files. I'll wait to see what others think though. 

Thanks for the feedback lad, I'll make the appropriate changes to accomodate your suggestions. Hadn't even ackowledged that it was null safe (old habits die hard ey?). Definitely hear you on the guard clauses.

3 hours ago, Dan said:

Hi mate, thanks for the application.

I agree with ADivorcedFork on his points also. 

In terms of the application in general, both scripts are a little on the simpler side and would rather something more complex for SW. In terms of the scripts themselves, you should look at modularising them, moving your cases into tasks. Much easier to maintain and manage when it's not all smushed into one file.

These are a little rushed, as previously suggested, you have a check if the fishing Spot is in Viewport () but do nothing if it's not? You don't try and walk closer, move the camera or nothing, it's almost like you added the in-viewport check because you felt you had to but didn't really understand why.

In your swamp toad script, you're adding custom strings to the paint for inventory tracking and tracking those manually. 
Firstly, look at the track items option in the paint builder https://docs.powbot.org/#/Basic_Fundamentals/PaintBuilderAPI?id=track-items
Secondly, the way you track your items has little to no validation, you're tracking all items that enter your inventory. No matter what the item is, if you were to missclick and pick up some sort of junk item you're classing that as swamp toad increase. You should really validate what item is going into your inventory.

Overall, a lot of the issues would be solved with a little more TLC. However, if they were a little more complex, you'd find yourself having to add more strict validations etc. 

I'll keep this open for a while if you have anything more you wanted to put forward for your application.

Nice one, Dan.

 

Yeah well aware both are very 'basic' scripts (hence why I didn't opt to implement any kind of task/node based framework). Can definitely admit that both were chopped together fast and without too much logical thought. In lieu to the custom strings in my paint, I did initially use the item tracker in the paint class but it seemed to be accounting for both the toads in the inventory changed event and the paint and so showing up double the actual figure (though on reflection if I'm using the paint tracker then I needn't be subscribing to the onItemChanged event seperately anyway!). 

 

Cheers for the feedback, both. I'll make some necessary amendments today and will more than likely be pushing out a different script to suit my needs on my low level account today too.

Edited by Pseudo
  • Like 1
Link to post
Share on other sites

I've made some small logic changes to the two scripts previously submitted as per above discussion, in addition I've since written a PvP planking script which may be found here that ought to demonstrate more complexity.

 

Things I'm aware of and I'll be addressing in coming days (when I'm less busy w/ crimbo):

 

- Currently haven't written the calculations for the hourly profit besides a 'hacky' means of calculating the planks made/hour, which will be improved when I have time as stated above.

 

D6BZEEe.png

Edited by Pseudo
Link to post
Share on other sites

Looks good to me but got some nit picky things :classic_wink:

Looking at the setup you've got going on
For example you've got: https://github.com/ps-eu-do/Powbot-Mobile-/blob/main/src/com/pseudo/scripts/moneymaking/pohplanker/nodes/HandleConversation.java
Which just calls another class which has the actual methods for it. Why not include them in the specific node instead of having a separate class? They're methods you're not gonna be re-using for the most part.


And the for loops in the same class, you can pass through the array into Chat#ContinueChat(ourActions), it will pick any that matches your array, and if none do, it will return false. This will also click continue if no options are available 

 

Other than that, it shows you're capable of writing and understanding scripts, I'd say it's a yes from me, but we'll have to wait for Dan :classic_biggrin:

  • Thanks 1
Link to post
Share on other sites

2 minutes ago, Proto said:

Looks good to me but got some nit picky things :classic_wink:

Looking at the setup you've got going on
For example you've got: https://github.com/ps-eu-do/Powbot-Mobile-/blob/main/src/com/pseudo/scripts/moneymaking/pohplanker/nodes/HandleConversation.java
Which just calls another class which has the actual methods for it. Why not include them in the specific node instead of having a separate class? They're methods you're not gonna be re-using for the most part.


And the for loops in the same class, you can pass through the array into Chat#ContinueChat(ourActions), it will pick any that matches your array, and if none do, it will return false. This will also click continue if no options are available 

 

Other than that, it shows you're capable of writing and understanding scripts, I'd say it's a yes from me, but we'll have to wait for Dan :classic_biggrin:

In regard to the HandleCoversation node I hear where you're coming from, I just felt from a design perspective it made more sense to have my methods seperate as some of the logic in my ButlerHandler is also being used in some of my other classes.

 

That's good to know, I guess we can put that down to a bit of inexperience with the API, I've just made said amendent locally and it will be updated in my next commit.

 

Cheers for the feedback, it's always appreciated.

Link to post
Share on other sites

  • Moderators

Hi mate.

The planker is much better in terms of complexity however I've listed my feedback below.

Butler Handler

https://github.com/ps-eu-do/Powbot-Mobile-/blob/883d19d939e07f19aa2f284df8e542da38f5de94/src/com/pseudo/scripts/moneymaking/pohplanker/fw/logic/ButlerHandler.java#L20
Your validation seems a little wonky here. Your entire isInHouse boolean is based on an object called "Portal" being within 30 tiles, but that would also return true when outside the portal too. While inside your house, it's instanced so a secondary check of your tile would be better. return Objects.stream().at(RimmingtonPortalTile).name("Portal").isEmpty();

You do a lot of .valid() calls, which are much more expensive than counts

!Inventory.stream().filter(i -> i.name().contains("ogs")).first().valid()
vs
Inventory.stream().filtered(i->i.name().toLowerCase().contains("logs")).isEmpty()

So here I've used .filtered() vs .filter(), filter is a java method to filter the stream, filtered is a method we made in the api which optimises the filters you pass.
I also used toLowerCase() so it picks up Logs and * logs* and makes more logical sense the next time you come to it and think "What the fuck is ogs" lol.
I also then check if the stream is empty, by doing this I know that there is no logs in my inventory whereas you're having to return the first item from the stream, and then do the valid() calculations on top of that. Rather than just checking if the stream was empty.

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

Speaking of expensive queries, your component query, while correct, is rather a blanket statement which is pretty resource hungry.
https://github.com/ps-eu-do/Powbot-Mobile-/blob/883d19d939e07f19aa2f284df8e542da38f5de94/src/com/pseudo/scripts/moneymaking/pohplanker/fw/logic/ButlerHandler.java#L50
 

Component callInterface = Components.stream().action("Call Servant").first();
vs
Component callInterface = Components.stream(widgetID).action("Call Servant").first();

by passing the top level widgetID for example 162; it means it'll only search for that action in all of the components for 162. Whereas you're making it search through every loaded widget and all of it's components.

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

https://github.com/ps-eu-do/Powbot-Mobile-/blob/883d19d939e07f19aa2f284df8e542da38f5de94/src/com/pseudo/scripts/moneymaking/pohplanker/fw/logic/ButlerHandler.java#L54

This needs work on a few levels. 

 public int getLogsAmount() {
        int amt = 0;
        for (Item i : Inventory.stream()) {
            if (i.valid() && i.name().contains("ogs")) {
                amt++;
            }
        }
        return amt;
    }

//This is the equivalent steps you've taken

//1. Declare amt;
//2. Stream every item in the inventory;
//3. Check if the item is valid && name contains "ogs";
//4. amt++;
//5. return amt;

VS
  
 public int getLogsAmount() {
  return (int) Inventory.stream().filtered(i->i.name().toLowerCase().contains("logs")).count();
}
//1. Stream filtered inventory
//2. return count();

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

https://github.com/ps-eu-do/Powbot-Mobile-/blob/883d19d939e07f19aa2f284df8e542da38f5de94/src/com/pseudo/scripts/moneymaking/pohplanker/fw/logic/ButlerHandler.java#L119

Game.tab(Game.Tab.INVENTORY) is a boolean and returns true if already open so no need to do if !open, open; just call open and if it's already open, it'll return true.

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

https://github.com/ps-eu-do/Powbot-Mobile-/blob/883d19d939e07f19aa2f284df8e542da38f5de94/src/com/pseudo/scripts/moneymaking/pohplanker/fw/logic/ButlerHandler.java#L120

You're still not moving the camera, walking to the npc doesn't guarantee they're in viewport, only that you've walked to them.

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

https://github.com/ps-eu-do/Powbot-Mobile-/blob/883d19d939e07f19aa2f284df8e542da38f5de94/src/com/pseudo/scripts/moneymaking/pohplanker/fw/logic/ButlerHandler.java#L123

It's better to use smaller wait times and increase the checks, by doing 1000, 1 you're always guaranteed to be waiting 1000ms, that's not a conditional wait as you're not checking the return, that's just static 1000ms wait. Whereas if you did 250, 5, you might only have to wait 250ms, and then it will early exit from the wait, but if there's lag for example and it takes 750ms to open, then it'll early exit then. You can also check the return of the conditional wait, true is your condition is true, false is your conditional wait hit the max ms and attempts and timed out.

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

Item Handler

https://github.com/ps-eu-do/Powbot-Mobile-/blob/883d19d939e07f19aa2f284df8e542da38f5de94/src/com/pseudo/scripts/moneymaking/pohplanker/fw/logic/ItemHandler.java#L17

lines 17 through to 21; I'm not sure where you're setting the value of laws but it'll be cached at the point of setting; so when it comes to line 20, checking the stacksize and validity you're referencing an old and outdated cached reference from whenever it was set. Caching should be done when using an item reference on mulitple occasions almost immediately after setting for example

Item laws = Inventory.stream().name("Law rune").first();
if(laws!=Item.getNil() && laws.stacksize>2){
   System.out.println("Here I accessed the inventory stream once, but used the object reference twice");
   System.out.println("However, accessing laws after this point, it would likely need updating as it's an out of date reference.");
}

There's similar occurances for the coins item too.

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

Teleport Handler

https://github.com/ps-eu-do/Powbot-Mobile-/blob/883d19d939e07f19aa2f284df8e542da38f5de94/src/com/pseudo/scripts/moneymaking/pohplanker/fw/logic/TeleportHandler.java#L12

I don't understand the point of this?

You created a method to pass a spell to, to then call later? rather than just casting Spell.cast(). Imo it just creates another point of failure along the line.

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

https://github.com/ps-eu-do/Powbot-Mobile-/blob/883d19d939e07f19aa2f284df8e542da38f5de94/src/com/pseudo/scripts/moneymaking/pohplanker/fw/logic/TeleportHandler.java#L17

As mentioned earlier, you don't need to check if tab is open, you can just call the method.

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

Call Butler

https://github.com/ps-eu-do/Powbot-Mobile-/blob/883d19d939e07f19aa2f284df8e542da38f5de94/src/com/pseudo/scripts/moneymaking/pohplanker/nodes/CallButler.java#L29

.filter() shouldn't really be used on a name equals comparison when we have .name()

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

Try to keep your writing style consistent, throughout the majority of what I've already read, you use braces for your if statements whereas here you decided not too?

https://github.com/ps-eu-do/Powbot-Mobile-/blob/883d19d939e07f19aa2f284df8e542da38f5de94/src/com/pseudo/scripts/moneymaking/pohplanker/nodes/TeleportToBank.java#L32

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

I've not written feedback for all of the nodes/classes etc as I'd just be repeating myself at this point.

However You're clearly capable, but just lacking knowledge and experience with the API and I'd rather you keep at it for now and get to know the best practices with the api. Even where you did a for loop to count the number of logs in your inventory rather than using .count().

This is a marathon rather than a sprint, take some time, get used to the API and come back in a few weeks. I'll happily accept the POH planker again but yeah just take some time to get used to our API and you'll be sorted. 👌

Link to post
Share on other sites

1 hour ago, Dan said:

Hi mate.

The planker is much better in terms of complexity however I've listed my feedback below.

Butler Handler

https://github.com/ps-eu-do/Powbot-Mobile-/blob/883d19d939e07f19aa2f284df8e542da38f5de94/src/com/pseudo/scripts/moneymaking/pohplanker/fw/logic/ButlerHandler.java#L20
Your validation seems a little wonky here. Your entire isInHouse boolean is based on an object called "Portal" being within 30 tiles, but that would also return true when outside the portal too. While inside your house, it's instanced so a secondary check of your tile would be better. return Objects.stream().at(RimmingtonPortalTile).name("Portal").isEmpty();

You do a lot of .valid() calls, which are much more expensive than counts

!Inventory.stream().filter(i -> i.name().contains("ogs")).first().valid()
vs
Inventory.stream().filtered(i->i.name().toLowerCase().contains("logs")).isEmpty()

So here I've used .filtered() vs .filter(), filter is a java method to filter the stream, filtered is a method we made in the api which optimises the filters you pass.
I also used toLowerCase() so it picks up Logs and * logs* and makes more logical sense the next time you come to it and think "What the fuck is ogs" lol.
I also then check if the stream is empty, by doing this I know that there is no logs in my inventory whereas you're having to return the first item from the stream, and then do the valid() calculations on top of that. Rather than just checking if the stream was empty.

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

Speaking of expensive queries, your component query, while correct, is rather a blanket statement which is pretty resource hungry.
https://github.com/ps-eu-do/Powbot-Mobile-/blob/883d19d939e07f19aa2f284df8e542da38f5de94/src/com/pseudo/scripts/moneymaking/pohplanker/fw/logic/ButlerHandler.java#L50
 

Component callInterface = Components.stream().action("Call Servant").first();
vs
Component callInterface = Components.stream(widgetID).action("Call Servant").first();

by passing the top level widgetID for example 162; it means it'll only search for that action in all of the components for 162. Whereas you're making it search through every loaded widget and all of it's components.

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

https://github.com/ps-eu-do/Powbot-Mobile-/blob/883d19d939e07f19aa2f284df8e542da38f5de94/src/com/pseudo/scripts/moneymaking/pohplanker/fw/logic/ButlerHandler.java#L54

This needs work on a few levels. 

 public int getLogsAmount() {
        int amt = 0;
        for (Item i : Inventory.stream()) {
            if (i.valid() && i.name().contains("ogs")) {
                amt++;
            }
        }
        return amt;
    }

//This is the equivalent steps you've taken

//1. Declare amt;
//2. Stream every item in the inventory;
//3. Check if the item is valid && name contains "ogs";
//4. amt++;
//5. return amt;

VS
  
 public int getLogsAmount() {
  return (int) Inventory.stream().filtered(i->i.name().toLowerCase().contains("logs")).count();
}
//1. Stream filtered inventory
//2. return count();

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

https://github.com/ps-eu-do/Powbot-Mobile-/blob/883d19d939e07f19aa2f284df8e542da38f5de94/src/com/pseudo/scripts/moneymaking/pohplanker/fw/logic/ButlerHandler.java#L119

Game.tab(Game.Tab.INVENTORY) is a boolean and returns true if already open so no need to do if !open, open; just call open and if it's already open, it'll return true.

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

https://github.com/ps-eu-do/Powbot-Mobile-/blob/883d19d939e07f19aa2f284df8e542da38f5de94/src/com/pseudo/scripts/moneymaking/pohplanker/fw/logic/ButlerHandler.java#L120

You're still not moving the camera, walking to the npc doesn't guarantee they're in viewport, only that you've walked to them.

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

https://github.com/ps-eu-do/Powbot-Mobile-/blob/883d19d939e07f19aa2f284df8e542da38f5de94/src/com/pseudo/scripts/moneymaking/pohplanker/fw/logic/ButlerHandler.java#L123

It's better to use smaller wait times and increase the checks, by doing 1000, 1 you're always guaranteed to be waiting 1000ms, that's not a conditional wait as you're not checking the return, that's just static 1000ms wait. Whereas if you did 250, 5, you might only have to wait 250ms, and then it will early exit from the wait, but if there's lag for example and it takes 750ms to open, then it'll early exit then. You can also check the return of the conditional wait, true is your condition is true, false is your conditional wait hit the max ms and attempts and timed out.

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

Item Handler

https://github.com/ps-eu-do/Powbot-Mobile-/blob/883d19d939e07f19aa2f284df8e542da38f5de94/src/com/pseudo/scripts/moneymaking/pohplanker/fw/logic/ItemHandler.java#L17

lines 17 through to 21; I'm not sure where you're setting the value of laws but it'll be cached at the point of setting; so when it comes to line 20, checking the stacksize and validity you're referencing an old and outdated cached reference from whenever it was set. Caching should be done when using an item reference on mulitple occasions almost immediately after setting for example

Item laws = Inventory.stream().name("Law rune").first();
if(laws!=Item.getNil() && laws.stacksize>2){
   System.out.println("Here I accessed the inventory stream once, but used the object reference twice");
   System.out.println("However, accessing laws after this point, it would likely need updating as it's an out of date reference.");
}

There's similar occurances for the coins item too.

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

Teleport Handler

https://github.com/ps-eu-do/Powbot-Mobile-/blob/883d19d939e07f19aa2f284df8e542da38f5de94/src/com/pseudo/scripts/moneymaking/pohplanker/fw/logic/TeleportHandler.java#L12

I don't understand the point of this?

You created a method to pass a spell to, to then call later? rather than just casting Spell.cast(). Imo it just creates another point of failure along the line.

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

https://github.com/ps-eu-do/Powbot-Mobile-/blob/883d19d939e07f19aa2f284df8e542da38f5de94/src/com/pseudo/scripts/moneymaking/pohplanker/fw/logic/TeleportHandler.java#L17

As mentioned earlier, you don't need to check if tab is open, you can just call the method.

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

Call Butler

https://github.com/ps-eu-do/Powbot-Mobile-/blob/883d19d939e07f19aa2f284df8e542da38f5de94/src/com/pseudo/scripts/moneymaking/pohplanker/nodes/CallButler.java#L29

.filter() shouldn't really be used on a name equals comparison when we have .name()

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

Try to keep your writing style consistent, throughout the majority of what I've already read, you use braces for your if statements whereas here you decided not too?

https://github.com/ps-eu-do/Powbot-Mobile-/blob/883d19d939e07f19aa2f284df8e542da38f5de94/src/com/pseudo/scripts/moneymaking/pohplanker/nodes/TeleportToBank.java#L32

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

I've not written feedback for all of the nodes/classes etc as I'd just be repeating myself at this point.

However You're clearly capable, but just lacking knowledge and experience with the API and I'd rather you keep at it for now and get to know the best practices with the api. Even where you did a for loop to count the number of logs in your inventory rather than using .count().

This is a marathon rather than a sprint, take some time, get used to the API and come back in a few weeks. I'll happily accept the POH planker again but yeah just take some time to get used to our API and you'll be sorted. 👌

Thanks for such an in-depth response, a lot of valuable information there to take on-board. I'm not going to write a long winded reply here seen as we've spoken on discord, but I've updated the script to reflect said suggestions and will be commiting the newest version imminently.

Link to post
Share on other sites

  • 2 weeks later...
2 hours ago, Dan said:

Any update?

Hi mate,

 

I've updated the script to reflect the suggestions/critique you provided, so it should be that bit more efficient now. I've also definitely seen an improvement in my general knowledge of the API over the past week or two (I've been working on a few different things behind the scenes).

 

Github has been updated to reflect said changes.

  • Thanks 1
Link to post
Share on other sites

  • Moderators

Can you explain what's going on in the ItemHandler.java, seems to be no change or explanation or response to my first feedback?
for example;

    public boolean hasEnoughLaws() {
        return laws.valid() && laws.stackSize() >= 2;
    }

Here you're not passing laws as a param to the method, but are referring to an old and most likely outdated cached reference?

Link to post
Share on other sites

2 hours ago, Dan said:

Can you explain what's going on in the ItemHandler.java, seems to be no change or explanation or response to my first feedback?
for example;

    public boolean hasEnoughLaws() {
        return laws.valid() && laws.stackSize() >= 2;
    }

Here you're not passing laws as a param to the method, but are referring to an old and most likely outdated cached reference?

The law/coins are being assigned in the Banking class prior to checking the state of the above Boolean, so the reference should be up to date. Could potentially argue that said declarations needn't be in a separate class, just seemed more organised to me.

Link to post
Share on other sites

  • Moderators

I see!

Either way looks good, sorry it took so long, I didn't realise it had all been updated lol; welcome to the team 🙂

@const_

Link to post
Share on other sites

5 hours ago, Dan said:

I see!

Either way looks good, sorry it took so long, I didn't realise it had all been updated lol; welcome to the team 🙂

@const_

Nice one, cheers. And no worries!

Link to post
Share on other sites

  • Dan locked this topic
Guest
This topic is now closed to further replies.
 Share