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

Corpse profit tracker #1152

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Emirlol
Copy link
Collaborator

@Emirlol Emirlol commented Jan 31, 2025

Adds a corpse profit tracker for the glacite mineshafts. The screen is accessed with a command: /skyblocker rewardTrackers corpse list.

Technical changes

  • Extracts CorpseType from the corpse finder into a separate file, with an extra field and some methods to make it easier for the tracker.

    I also changed the color of the types according to the color of the text that appears when you loot a corpse.

  • Refactored some code from the PowderMiningTracker into AbstractProfitTracker and PowderMiningTracker, along with the new CorpseProfitTracker inherit from the said abstract class. There's not much inheritance magic other than some utility methods.

  • Moved the PowderMiningTracker into a separate package

  • Also refactored the commands in PowderMiningTracker to not clutter the first level of subcommands on the namespace, so it goes like /skyblocker rewardTrackers powderMining <actual subcommand> now

  • Added a toggle config for PowderMiningTracker. I can't believe I forgot this on the initial PR...

  • Added 2 new screens and 2 lists for displaying the relevant data for those screens:

    • CorpseProfitScreen & RewardList for summary
    • CorpseProfitHistoryScreen & CorpseList for history

    The summary screen shows the total of all rewards while the history screen shows each individual corpse's rewards, along with timestamps.

Summary Screen

image

History Screen

image

@LifeIsAParadox LifeIsAParadox added the reviews needed This PR needs reviews label Jan 31, 2025
@Emirlol
Copy link
Collaborator Author

Emirlol commented Jan 31, 2025

Some considerations for the future:

  • More translatables
  • CSV export? Idk if anyone would want that. Idk if this would even work, given that the CSV would have to be 3-dimensional. The comma separated values are the actual values of each reward while each new line is another reward, but how do you display different corpses' rewards in a meaningful manner in the same CSV? Or perhaps XLSX export???

Also, the amounts in the initial picture are bugged. The issue causing that was fixed and is in the initial PR but the data was saved when it happened so I cba to reset and mine more.

@Emirlol Emirlol added the new feature This issue or PR is a new feature label Jan 31, 2025
Copy link
Contributor

@olim88 olim88 left a comment

Choose a reason for hiding this comment

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

Looks like good code. Few translations and a unused setting. Would also be nice to be able to open the summary page when clicking on the profit message.

@Emirlol Emirlol force-pushed the corpse-profit-tracker-xd branch from 9860304 to 1e4baab Compare February 7, 2025 18:12
@Emirlol
Copy link
Collaborator Author

Emirlol commented Feb 7, 2025

The changes at 1e4baab need testing. I can't do this since, well, I am banned from Hypixel.

Copy link
Contributor

@olim88 olim88 left a comment

Choose a reason for hiding this comment

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

LGTM. I have tested it as well and it seams to work fine.

@LifeIsAParadox LifeIsAParadox added merge me please Pull requests that are ready to merge and removed reviews needed This PR needs reviews labels Feb 14, 2025
- Extract `CorpseLoot` and `Reward` inner classes into separate classes
- Use translatables where it makes sense
- Add config option to toggle corpse profit tracker (can't believe I forgot this on corpse tracker as well)
- Some minor command adjustments and fixes to both `PowderMiningTracker` and `CorpseProfitTracker`
- Refactored `CorpseProfitHistoryScreen` into `CorpseProfitScreen`. The button that switched from one to the other is now a view switch button that changes the displayed list, and functionality of both screens is combined in one.
- Add hover/click events to the profit text sent when a corpse is looted that opens the screen
This way, they don't display the price per unit when the amount is 1.
@Emirlol Emirlol force-pushed the corpse-profit-tracker-xd branch from 15b1d47 to 4477573 Compare February 21, 2025 00:16
@LifeIsAParadox LifeIsAParadox added reviews needed This PR needs reviews and removed merge me please Pull requests that are ready to merge labels Feb 21, 2025
Copy link
Collaborator

@kevinthegreat1 kevinthegreat1 left a comment

Choose a reason for hiding this comment

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

Looks like a simple rebase so I'll approve.

@LifeIsAParadox LifeIsAParadox added merge me please Pull requests that are ready to merge and removed reviews needed This PR needs reviews labels Feb 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me please Pull requests that are ready to merge new feature This issue or PR is a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants