Jump to content
×
×
  • Create New...

Klumel8 script writter application


Recommended Posts

  • Moderators

Hey mate, I'll try and keep this compact as possible


3 things here; not relating to the script but just in general, not all food has "Eat" some have drink etc.
Secondly, rather than use .filter(i -> i.actions().contains("Eat")), you  could instead just use .actions("Eat")
And lastly, if you really must use a filter, try to use .filtered() instead. This has been tweaked to be optimised for our stream queries whereas .filter is just a generic java method.
https://github.com/klumel8/LavaRunes/blob/1b2d5387bcc744bf318f17d165f58e005d1a9f09/KlumScripts.java#L57

 

Paint builder
You're manually tracking your own lavas using the inventory change listener which is fine, however the paint builder already has a .trackInventoryItem(ID) method.

 

Banking

Your banking from what I can tell, is likely to be reeaaal slow.
Everything is in an if else chain meaning it will only complete 1 action each time the task runs. When it comes to things like runecrafting, people have a hard on for super fast banking because the xp is already wank. Slowing down banking is going to make it even worse.

Pouches; I'm not sure what you're doing here but there seems to be no validation on what you've successfully filled.
https://github.com/klumel8/LavaRunes/blob/1b2d5387bcc744bf318f17d165f58e005d1a9f09/nodes/Banking.java#L96
From my understanding you're looping the pouches, attempting to fill the pouches and then logging those to a list. My first concern is that you're not even checking the return value of the .interact("Fill") so you don't know if the pouch was even successfully filled and are then adding it to the filled pouches list. You then have a conditional wait which wil ltime out at 3 seconds, and then another wait after this, none of those are being checked against their return. Condition waits have a boolean return as well as .interact() so you should really be validation what's successfully happened before logging it to a list. 
However, essence pouches actually have a varpbit for their contents. The varp is only accessible while the bank is open however it is a categorical way to confirm the pouch was/is full.

Even ignoring the if else chain slowing you down, you have an awful lot of static sleeps; ks.cWait()'s everywhere which is really not advised. You seem to use a mix of conditional waits and static waits which doesn't make any sense to me. As humans we react to changes, nobody sits there and goes "Ok chaps, I've closed the bank, lets generate a random number and sit idle for that many ms" They react to the bank being closed which is what a conditional wait is, a reaction.

 

Modularisation (CraftRunes)

I was a little shocked when I saw your craft runes task. It hsould in theory be only a few lines; validate ready to craft, get the altar object, interact and done but instead your craft task has like 5 different responsibilities.
Each task should have 1 sole responsibility. Here I would suggest MoveToAltar, CraftRunes, CastImbue, EmptyPouches or something along those lines, this one task shouldn't have this many responsibilities.

https://github.com/klumel8/LavaRunes/blob/main/nodes/TravelToAltar.java

https://github.com/klumel8/LavaRunes/blob/1b2d5387bcc744bf318f17d165f58e005d1a9f09/nodes/RepairPouch.java

These are all the same, too much shit in each task

 

Magic numbers & naming

https://github.com/klumel8/LavaRunes/blob/1b2d5387bcc744bf318f17d165f58e005d1a9f09/nodes/RepairPouch.java#L21

Here you define some poorly named widgets and components, nobody is going to be able to work out what that is, if you come back after a year and need to work on the script, you'll hate yourself for not giving them proper names.
 

However despite defining them, you're not using them;
https://github.com/klumel8/LavaRunes/blob/1b2d5387bcc744bf318f17d165f58e005d1a9f09/nodes/RepairPouch.java#L72
Here it should be widget1 and component1 but instead you've got magic numbers in their place.

Again refering back to modularisation; this task has banking, it has spell casting and has conversation handling.. too much for 1 task.

Also, your hasRunes() is a somewhat defunct method as we have Spell.canCast() which does the same thing.

 

Caching

I'm not sure what's happening here?
https://github.com/klumel8/LavaRunes/blob/1b2d5387bcc744bf318f17d165f58e005d1a9f09/nodes/TravelToAltar.java#L51
Here you make a single call to cache the first item which matches your query, you then go to do a conditional wait for it to be valid. If it's not valid, it's going to get stuck there in a loop for 3 seconds unnecessarily as if it's not valid the item cache prior isn't being called on loop till it becomes valid.

Instead you can cache the item and just check the .valid() without putting it in a wait.

 

Overall it needs modularising and speeding up. I look forward to the changes 🙂

 

Link to post
Share on other sites

Posted (edited)

Hi Dan thanks for the feedback 🙂,

Sorry for that eat part.. it should not have been there, its a remnant from another code but your point is valid.

I completely overhauled the banking part and added a lot of modulization as you suggested. By removing the conditional waits and instead virtually keeping track fo which items are present I sped up the banking a lot! You can see the new speed in this clip https://streamable.com/rramt4

Also personally I like to do multiple (unneeded) clicks when walking somewhere since I think thats a very human thing to do, but I also removed those.

For the canCast() comment I have tried to implement this but it does not work with a runepouch.

Let me know what you think of the revisted script! 🙂

Edited by klumel8
Link to post
Share on other sites

  • Moderators

Firstly just wanted to talk about the canCast()... If something isn't working as it should be, you should really report it rather than just finding a work around. The api won't ever improve if these issues are just left in there.
I asked Krulvis to look into it as he wrote the runepouch and magic api and foudn the issue, the data returned from the varpbit changes when more than 8(ish)k runes are in the pouch as the varpbit runs out of bits to return the data. Anyway long story short, it's being worked on.
 

Looking back over your tasks now they're a little more modular but I'm struggling to understand but as far as I can tell, your tasks have no validations? Seemingly at all?
I was going to say despite splitting up the tasks they still contain a lot of unnecessary bloat for example:
https://github.com/klumel8/LavaRunes/blob/ae4a1672fd993ba7fc45870cabb606cb8313fbc2/nodes/CraftRunes.java#L43
And again, I was going to say as part of your validation to craft runes, you should already be checking that you're in the right area but when I checked your validation, there literally isn't anything there to validate anything? Feel free to drop me a message on discord again if you wanted me to explain anything. However in short, your validate() should be somethign that validates if the task needs to subsequently run the execute() method. Something like:

 @Override
    public boolean validate() {
        return gearHandler.hasEarthRunes() && lavaConstants.altarArea.contains(Players.local());
    }

You then remove 12 lines of code from your execute() streamlining executes. The idea here is to actually validate if execute needs to run since you check if (node.validate()) { in your poll loop.

You also then save time in the poll() loop as you're not unecessarily running every single execute when it doesn't need to. I'm not going to link them, but every single one of your nodes/tasks has this issue. 

 

https://github.com/klumel8/LavaRunes/blob/main/nodes/PrepareInvForMage.java
You updated your banking class but still needlessly looping execute() here to withdraw all your items

 

https://github.com/klumel8/LavaRunes/blob/main/nodes/TravelToAltar.java

1 responsibility per task, this one has like 10...

Link to post
Share on other sites

Hi Dan,

Thanks for the feedback. I almost completely restructured the script but it is now fully TBL 🙂

Hopefully the 3rd time is a charm

Link to post
Share on other sites

  • Moderators

Looks  great!

Just some final bits of feedback and/or advice;

https://github.com/klumel8/LavaRunes/blob/f5881b2cec5fcaa7650223ca09d8fdc7f465c800/klumel8/Lavas/Branches/GoToRuins/Leaves/AlwaysRun.java#L15
To turn run on, you need to pass a bool, for example Movement.running(true) to enable, otherwise it just returns the bool of whether it's on or off.

 

https://github.com/klumel8/LavaRunes/blob/f5881b2cec5fcaa7650223ca09d8fdc7f465c800/klumel8/Lavas/Branches/AtAltar/AtAltar.java#L23
Something I noticed when writing my nature script is streaming the altar seemingly took anywhere from 200-850ms, which isn't a huge problem, however I then realised an Area.contains(Players.local()) is like a 0-3ms check. No need to change it, but just some advice in the future 🙂

 

https://github.com/klumel8/LavaRunes/blob/f5881b2cec5fcaa7650223ca09d8fdc7f465c800/klumel8/Lavas/Branches/PrepInvDarkMage/Leaves/FetchRunes.java#L19
You can do isEmpty() and isNotEmpty() checks on bank streams to, it's also less intensive to check if the stream is empty vs return the actual item, then access the items info to see if the stacksize is !=0. Again no need to change, but a useful thing to know.

 

However, you should be proud. From our discussions on discord in dm's and your messages previously about the corp script and the intro to tasks etc. You've come a long way in a short time with little to no help. 👍

Welcome to the team 😄

@const_ pls do your thing.

Link to post
Share on other sites

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