Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvement: Berberis Helper Rework #3147

Open
wants to merge 21 commits into
base: beta
Choose a base branch
from

Conversation

RevengeLordOfMaj
Copy link

@RevengeLordOfMaj RevengeLordOfMaj commented Jan 2, 2025

What

Berberis is a crop in the Rift that grows one at a time, then needs to be broken in the same order they grew in. The base game guides you to each bush using particles. The problem with this is that the particles don't have the best visibility, and you can break the next bush as soon as the previous was broken, having you waste time waiting to see where the particles lead you.

SkyHanni already has a helper for this, which solves the particle visibility. It highlights the bush the particles are currently on when not moving, and follows them to the next bush while moving.

This new system shows you two bushes ahead of the one you need to break so you no longer have to wait for the particles!

It does this by watching them as they spawn in, and remembering the order they did so. It then highlights the next one to break, and the next two after that.

Images
2025-01-02.03-46-47.-.Trim.1.mp4

Changelog Improvements

  • Reworked Berberis Farming Helper in the Rift. - Maj
    • Previews what bushes need to be broken after the current one if you were in the plot when they last regrew.
    • If you were in another plot when they grew, or it is wrong about the order for any reason, it will fall back to the original helper.

@github-actions github-actions bot added the Wrong Title/Changelog There is an error in the title or changelog label Jan 2, 2025
Copy link

github-actions bot commented Jan 2, 2025

I have detected some issues with your pull request:

Body issues:
Change should end with a full stop in text: Initial Rework

Please fix these issues. For the correct format, refer to the pull request template.

@github-actions github-actions bot added the Detekt Has detekt problem label Jan 2, 2025
Copy link

github-actions bot commented Jan 2, 2025

One or more Detekt Failures were detected:

@github-actions github-actions bot removed the Wrong Title/Changelog There is an error in the title or changelog label Jan 2, 2025
Copy link

github-actions bot commented Jan 2, 2025

One or more Detekt Failures were detected:

@github-actions github-actions bot removed the Detekt Has detekt problem label Jan 2, 2025
@github-actions github-actions bot added the Detekt Has detekt problem label Jan 3, 2025
Copy link

github-actions bot commented Jan 3, 2025

One or more Detekt Failures were detected:

@hannibal002
Copy link
Owner

If you use IntelliJ, please click the auto refomat hotkey (control + alt + L) before every commit

@RevengeLordOfMaj
Copy link
Author

oh my god thank you so much

Copy link

github-actions bot commented Jan 3, 2025

One or more Detekt Failures were detected:

@RevengeLordOfMaj
Copy link
Author

ok it didnt fix everything ig

@RevengeLordOfMaj
Copy link
Author

torry I'm struggling with this so much 😭

@github-actions github-actions bot removed the Detekt Has detekt problem label Jan 3, 2025
@hannibal002
Copy link
Owner

the wording of the changelog should be readable for users without technical knowledge (except the technical details/backend category). their "main line" should explain in a whole sentence what got changed in a broad statement, the "small line below" can include additional information about that change.

Your current wording is neither explanatory nor user ready, please change that

@hannibal002 hannibal002 added this to the Version 1.1.0 milestone Jan 4, 2025
@RevengeLordOfMaj
Copy link
Author

Updated. I hope this is better :)

@hannibal002
Copy link
Owner

image
im talking about those two statements. they dont mean anything on its own. they are the only lines showing up in the changelog, the pr name or the description above is not there. so the user lacks the context what is this about at all. and the extra text below should not provide context but rather better explain what this change means.

e.g. you could call them "Reworked Berberis Particle Helper in the Rift", then mention that the new design will remember the order after one full clean of the field and additionally still use the legacy system as fallback/while the order is not loaded yet. also you can merge both entires into one and have two smaller sentences below.
e.g.

+ title. - your name
    * line 1
    * line 2

@RevengeLordOfMaj
Copy link
Author

Ohh sorry my bad, 1 sec

@RevengeLordOfMaj
Copy link
Author

That better?

@hannibal002
Copy link
Owner

yes, way better. thanks!

@hannibal002 hannibal002 added the Soon This Pull Request will be merged within the next couple of betas label Jan 4, 2025
Copy link
Owner

@hannibal002 hannibal002 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The feature works already perfectly fine for me. great job!

the code needs more cleanup before we can merge the pr though.

maybe you can separate the old and new logic into different functions and then remove the comments about it?

generally I think the code style needs a bit more kotlin-like funtions, e.g. the one i used in my cleanup commit above.

Maybe add an option how many lines/blocks in the future you want to see at once. for me, showing 3 blocks is too much :/

@RevengeLordOfMaj
Copy link
Author

How does that look?

@github-actions github-actions bot added the Detekt Has detekt problem label Jan 5, 2025
Copy link

github-actions bot commented Jan 5, 2025

One or more Detekt Failures were detected:

@github-actions github-actions bot removed the Detekt Has detekt problem label Jan 5, 2025
@RevengeLordOfMaj
Copy link
Author

there :)

@hannibal002
Copy link
Owner

please click on the resolve button when a comment is done. also please rerequest a review from me when you want me to look again at that pr

image
image

@RevengeLordOfMaj
Copy link
Author

Oh sorry, this is my first time doing a pull request if it wasn't obvious :P

Copy link
Owner

@hannibal002 hannibal002 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more changes, but we are getting close to a ready PR :)


@HandleEvent
fun onConfigFix(event: ConfigUpdaterMigrator.ConfigFixEvent) {
event.move(71, "rift.area.dreadfarm.wiltedBerberis.hideparticles", "rift.area.dreadfarm.wiltedBerberis.hideParticles")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you dont need to bup the version from 60 to 71, you dont change existing config elements

// calculates the player's distance to the center of each plot, then sets closestPlot to the smallest
val plotDistances = arrayListOf(0.0, 0.0, 0.0, 0.0, 0.0, 0.0)
for (i in 0..5) {
val plotCenter = plots[i].a.middle(plots[i].b)
Copy link
Owner

@hannibal002 hannibal002 Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please only calculate the middle location once, then reuse the same value

event.move(71, "rift.area.dreadfarm.wiltedBerberis.hideparticles", "rift.area.dreadfarm.wiltedBerberis.hideParticles")
}

private fun getClosestPlot() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of this approach with array list, two for loops and manual comparing each entry against the prevous losetest plot, you can use lamda functions on a map like .minBy() to get the clostest plot. i think of something like cloestPlot = plots.map{ it to it.middle.distanceToPlayer() }.minBy{it.second}.first. then you can store the instance inseatd of the index in the closestPlot value.

val plotCornerA = plots[closestPlot].a.toBlockPos()
val plotCornerB = plots[closestPlot].b.toBlockPos()
for (block in BlockPos.getAllInBox(plotCornerA, plotCornerB)) {
if (block.toLorenzVec().getBlockAt() == Blocks.deadbush && !berberisList.contains(block.toLorenzVec())) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge all usages of block.toLorenzVec() into one variable

list = list.editCopy { removeIf { it.lastTime.passedSince() > 500.milliseconds } }
}

private fun sync() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

give this function a better name

for (berberis in list) {
with(berberis) {
if (currentParticles.distanceToPlayer() > 20) continue
if (y == 0.0) continue

val location = currentParticles.fixLocation(berberis)
if (!moving) {
event.drawFilledBoundingBoxNea(axisAlignedBB(location), Color.YELLOW, 0.7f)
event.drawFilledBoundingBoxNea(axisAlignedBB(location), config.highlightColor.toSpecialColor(), 0.7f)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toSpecialColor is a heavy logic, please move it outside of the for loop

@HandleEvent
fun onConfigFix(event: ConfigUpdaterMigrator.ConfigFixEvent) {
event.move(60, "rift.area.dreadfarm.wiltedBerberis.hideparticles", "rift.area.dreadfarm.wiltedBerberis.hideParticles")
private fun primaryRender(event: LorenzRenderWorldEvent) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better name

@@ -33,10 +36,30 @@ object RiftWiltedBerberisHelper {
private val config get() = RiftAPI.config.area.dreadfarm.wiltedBerberis
private var isOnFarmland = false
private var hasFarmingToolInHand = false
private var berberisList = listOf<LorenzVec>()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

give this list below better names to differentiate

}

private fun axisAlignedBB(loc: LorenzVec) = loc.add(0.1, -0.1, 0.1).boundingToOffset(0.8, 1.0, 0.8).expandBlock()
private fun nearestBerberis(location: LorenzVec): WiltedBerberis? =
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try to change the logic so that the distance is only getting calculated once per entry, not twice

)

private var closestPlot = 0
private var oldClosest = 0
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change those two variable sto be of type Plot?

@CalMWolfs CalMWolfs removed the Soon This Pull Request will be merged within the next couple of betas label Jan 18, 2025
@github-actions github-actions bot added the Merge Conflicts There are open merge conflicts with the beta branch. label Jan 24, 2025
Copy link

This pull request has conflicts with the base branch "beta". Please resolve those so we can test out your changes.

…work

# Conflicts:
#	src/main/java/at/hannibal2/skyhanni/features/rift/area/dreadfarm/RiftWiltedBerberisHelper.kt
@github-actions github-actions bot removed the Merge Conflicts There are open merge conflicts with the beta branch. label Jan 25, 2025
Copy link

Conflicts have been resolved! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants