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: Add ability to use /shedittracker with CorpseTracker #2582

Closed

Conversation

DavidArthurCole
Copy link
Contributor

What

Because CorpseTracker uses separated logic and buckets, it didn't fit the generic /shedititemtracker logic. This PR adds separate logic for it.

Changelog Improvements

  • Added ability to use /shedittracker on Corpse Profit Tracker. - Daveed

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.

not sure how easy this is to implement. but i dont like the code duplication

ProfileStorageData.profileSpecific?.mining?.mineshaft?.corpseProfitTracker?.let { trackerData ->
trackerData.getSelectedBucket()?.let {
tracker.addItem(it, event.internalName, event.amount)
ChatUtils.chat("Added ${event.internalName.itemName} §8x${event.amount} to Corpse Tracker §8(${it.displayName}§8)§e.")
Copy link
Owner

Choose a reason for hiding this comment

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

this should be abstracted into the bucketed item tracker.
ideally the bucketed item tracker would reuse the logic from skyhann iitem tracker so that this unique chat message is not necessary in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't re-use the logic - they're entirely separate trackers. The item trackers just have the concept of one set of global "loot" that's added to - this is an enum driven base of buckets, the logic doesn't fit. The only way I could think to do this was using the currently-selected bucket.

Best I could do would be to abstract it back to the bucketed item tracker, however that message is going to end up having to be very generic. We can't do it in SkyhanniItemTracker and expect it to work here - the addItem signatures are already overridden for SkyhanniBucketedItemTracker, as it takes a bucket as first param.

We also don't have the context of the tracker instance in the tracker itself, which would prevent actually moving this into the generic SkyhanniBucketedItemTracker, as we need to know the currently selected bucket;

ProfileStorageData.profileSpecific?.mining?.mineshaft?.corpseProfitTracker?.let {
}

Copy link
Owner

Choose a reason for hiding this comment

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

The only way I could think to do this was using the currently-selected bucket.

I was not expecting anything more.

Copy link
Owner

Choose a reason for hiding this comment

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

How about abstracting the existing item tracker class and creating a new "single bucketed tracker" for everything else?

tracker.addItem(it, event.internalName, event.amount)
ChatUtils.chat("Added ${event.internalName.itemName} §8x${event.amount} to Corpse Tracker §8(${it.displayName}§8)§e.")
} ?: ChatUtils.chat(
"§cYou do not have a Corpse Type selected, and cannot add loot to this tracker generically.\n" +
Copy link
Owner

Choose a reason for hiding this comment

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

i think this will clash with the default answer messages from /shedittracker

@@ -36,7 +36,7 @@ class SkyHanniBucketedItemTracker<E : Enum<E>, BucketedData : BucketedItemTracke
addItem(bucket, SKYBLOCK_COIN, coins)
}

fun addItem(bucket: E, internalName: NEUInternalName, amount: Int) {
fun addItem(bucket: E, internalName: NEUInternalName, amount: Int, manuallyAdded: Boolean = false) {
Copy link
Owner

Choose a reason for hiding this comment

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

as mentioned before, i recomment moving this into skyhanni item tracker, avoiding the code duplication alltogether

@hannibal002 hannibal002 added this to the Version 0.28 milestone Sep 24, 2024
@hannibal002 hannibal002 added the Soon This Pull Request will be merged within the next couple of betas label Sep 29, 2024
@hannibal002 hannibal002 self-requested a review September 29, 2024 23:50
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.

doesnt work for me
image
image

@DavidArthurCole
Copy link
Contributor Author

doesnt work for me

https://youtu.be/osf3jJuzVOE

So, it is working, it just isn't updating until something else triggers the tracker. Which, seems weird, but also I have up until now been unable to fix this, and can't get it to update when /shedittracker is called....

@hannibal002 hannibal002 modified the milestones: Version 0.27, Version 0.28 Oct 6, 2024
@hannibal002 hannibal002 removed the Soon This Pull Request will be merged within the next couple of betas label Oct 6, 2024
@hannibal002
Copy link
Owner

moving it t0 0.28 for now

@github-actions github-actions bot added the Merge Conflicts There are open merge conflicts with the beta branch. label Oct 13, 2024
Copy link

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

@DavidArthurCole
Copy link
Contributor Author

I'm gonna close this out. I truly can't figure out what's going wrong with the update() not taking, and it doesn't seem to be high profile anyways.

@DavidArthurCole DavidArthurCole deleted the CorpseTrackerAddItem branch October 16, 2024 01:45
@github-actions github-actions bot removed the Merge Conflicts There are open merge conflicts with the beta branch. label Oct 16, 2024
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.

2 participants