Adds the brimhaven-agility plugin

[kagof]

Brimhaven Agility Arena plugin

Adds a new plugin to aid with the Brimhaven agility arena. Specifically, it currently computes the shortest path the the active ticket dispenser, and displays that path on the screen for the user. This takes into account whether the user has the agility level for a given obstacle, and is computed using the A* pathfinding algorithm.

(changed following feedback) Line colour, and the ability to avoid specific obstacles are configurable.

Possible future expansions

These are not yet features in this plugin, but I may decide to expand it to include:

• removing the hint arrow when the ticket has been claimed
• highlighting the correct plank to use of the 3 options
• overlay when outside the arena indicating whether the arena can be entered (taking into account teleport timeout & whether the user has paid)

[runelite-github-app]

New plugin brimhaven-agility: https://github.com/kagof/rl-plugin-brimhaven-agility/tree/1c0c137f469da33484dcfaf28df8a0fdf5f870df

[cdfisher]

• If you're using significant portions of code from Quest Helper you should be copying their license headers into the files that use that code.

• If all the plugin currently does is draw that path, that probably shouldn't be a configurable option since the user wouldn't currently be using it to do anything else so they could just turn the plugin off entirely. You can probably remove that and just add it back as an option when the plugin does other things but in its current state it's probably more likely to just confuse users more than anything.

• Similarly, having the obstacle weights as config items is probably more likely to confuse users than it is to be actually helpful.

[kagof]

I see the build failure due to a line longer than 120 chars in my build.gradle, I'm updating to fix this and will address @cdfisher's feedback at the same time.

If you're using significant portions of code from Quest Helper you should be copying their license headers into the files that use that code.

I thought I had already done so, see WorldLines.java, but it looks like I missed QuestPerspective.java. Will add the headers to this file too now.

If all the plugin currently does is draw that path, that probably shouldn't be a configurable option since the user wouldn't currently be using it to do anything else so they could just turn the plugin off entirely.

This is a fair critique, I will remove this configuration option for the time being.

Similarly, having the obstacle weights as config items is probably more likely to confuse users than it is to be actually helpful.

I disagree on this front, as for some users it is possible they may want to tune it themselves. For example, users may want to avoid the dart obstacle, as failing it lowers your agility level by 2, in which case giving it a much higher weighting could be useful. Another example is if someone is very AFK and does not want to look to figure out which plank of the 3 to use. I do admit though that the likelihood someone actually cares enough to do this is fairly low.

Let me know if you still feel strongly that this weighting configuration should be removed and I can do so. Another option would be to replace the weighting configuration with a checkbox for avoid x obstacle, which, if checked, sets the weighting arbitrarily high so the obstacle will never be used. I'd be happy to replace it with this instead.

[cdfisher]

Another option would be to replace the weighting configuration with a checkbox for avoid x obstacle, which, if checked, sets the weighting arbitrarily high so the obstacle will never be used. I'd be happy to replace it with this instead.

This is probably a lot less confusing to users so I would do that

[runelite-github-app]

Internal use only: Reviewer details Maintainer details

[aHooder]

Perhaps this shouldn't be a blocker, but since reviews of changes down the line might not catch it, can you add something to ensure that the path finding cannot get stuck in an infinite loop? Accidentally freezing the client is just no fun, and as far as I can tell, it could get stuck if you were to add an entry in arena_layout.txt with the same source and destination coordinates for instance.

You should also probably run this line on the client thread, so you don't accidentally have multiple path finding loops running simultaneously (even though the code seems thread-safe at a glance). Currently, this line can run on Java's Event Dispatch Thread in response to a user changing a config. Keep in mind that clientThread.invokeLater has a mechanism to retry execution for functions which return booleans, so you'll probably want to also change your recomputePathIfNeeded to return void, since I don't see you using the return value anywhere anyway.

The plugin looks good otherwise though 🙂

[kagof]

Thanks for the prompt & thorough review @aHooder, very much appreciated. I'll aim to get your feedback addressed shortly.

can you add something to ensure that the path finding cannot get stuck in an infinite loop?

This is a very good shout. I agree that it can't happen now because the arena layout is valid, but I'd much rather remove the possibility for it to be introduced in the future. I think the following changes should be enough to mitigate this concern, but let me know if you have a better idea:

1. The pathfinding code should get a maximum number of nodes examined, and if it exceeds this threshold it just returns a null path. This is some belt & braces that should protect us even if an infinite loop does get introduced here
2. when examining neighbours, if the neighbour's coordinates are the same as the source's coordinates, just skip it and move on to the next one. This will prevent the specific issue you've highlighted

You should also probably run this line on the client thread, so you don't accidentally have multiple path finding loops running simultaneously (even though the code seems thread-safe at a glance).

Makes sense, I'll do this. I believe it should be thread safe as well, but there's no benefit to it running multiple times simultaneously anyways.

Forgive the potentially dumb question, as I'm obviously not familiar with the threading model in RuneLite; should I also be wrapping recomputePathIfNeeded with clientThread.invokeLater in the onGameTick and onStatChanged event handlers? Or will those already be running on the client thread and I only need to change the one you've highlighted?

[aHooder]

I think the following changes should be enough to mitigate this concern, ...

Those both sound good to me 🙂

should I also be wrapping recomputePathIfNeeded with clientThread.invokeLater in the onGameTick and onStatChanged event handlers? Or will those already be running on the client thread and I only need to change the one you've highlighted?

Most events like this run on the client thread already, but things which happen in response to user input might not for instance. If you're not sure, you can always wrap it to be safe in clientThread.invoke in order to invoke right away (as opposed to invokeLater) if already on the client thread. In this case though, I believe both onGameTick and onStatChanged always happen on the client thread anyway, so you don't really need to wrap it in these cases unless you want to be extra safe. In theory other plugins could post these events to the EventBus from a different thread, but I don't think such a thing should happen.

[kagof]

Thanks for the explanation @aHooder 🙂 I've updated the plugin addressing your feedback (and also fixed a small bug I found along the way).