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

Add powder mining tracker #1065

Merged
merged 18 commits into from
Jan 1, 2025

Conversation

Emirlol
Copy link
Collaborator

@Emirlol Emirlol commented Nov 24, 2024

Powder mining tracker that tracks rewards and calculates your total profit.
This does not include the money from blocks/other drops you might gain along the way (such as sludge from jungle pick if you go down that route).

image

Has a configurable filter to block items from being listed and used in profit calculation.

image

Saves rewards to disk upon closing the game and reads from disk upon launching the game.

Also adds some chat events that are fired regardless of cancels with no modifications (realistically only against those done via fabric's events).

Some considerations

  • A timer that can be started/stopped via commands or config buttons, to display alongside the total profit and powder to see average coins/powder per hour
  • Perhaps a proper movable gui element? Not sure since it can go very tall and our gui elements have very opaque backgrounds. Though, it could at least be movable if not one of those gui elements.
  • Some actual commands. There are currently 2 commands that were my initial way of debugging the rewards, with no categorization whatsoever. I have no idea what would be useful, though.
  • Confirmation screen for the config filter? It's probably very easy considering how the list works, just didn't bother.

@LifeIsAParadox LifeIsAParadox added the reviews needed This PR needs reviews label Nov 24, 2024
@Emirlol Emirlol added the new feature This issue or PR is a new feature label Nov 24, 2024
Copy link
Collaborator

@AzureAaron AzureAaron left a comment

Choose a reason for hiding this comment

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

This also needs to be rebased for 1.21.4

return NAME2ID_MAP.getOrDefault(itemName, "");
}

private static Path getRewardFilePath() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be a constant unless it can't be?

Copy link
Collaborator Author

@Emirlol Emirlol Dec 13, 2024

Choose a reason for hiding this comment

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

It can be, but I had a better solution on another branch that is based on this so I didn't bother to change to avoid a rebase.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason for making chat events as opposed to using Fabric's ones?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fabric's injection is like this:

if (ClientReceiveMessageEvents.ALLOW_GAME.invoker().allowReceiveGameMessage(message.get(), overlay)) {
	message.set(ClientReceiveMessageEvents.MODIFY_GAME.invoker().modifyReceivedGameMessage(message.get(), overlay));
	ClientReceiveMessageEvents.GAME.invoker().onReceiveGameMessage(message.get(), overlay);
} else {
	ClientReceiveMessageEvents.GAME_CANCELED.invoker().onReceiveGameMessageCanceled(message.get(), overlay);
	ci.cancel();
}

To listen to every incoming message, you'd have to register a listener to ALLOW_GAME that always returns true and never has anything to do with allowing messages or not. On top of that, if any listener before yours in ALLOW_GAME cancels it, your listener does not get called. So you have to register the same listener to GAME_CANCELED as well and complicate things even more. Oh and the 2 functional interfaces don't match due to their return values of boolean and void, so you need 3 whole methods (or lambdas) to achieve this.

This is easily solved by adding an event that passes the message to every listener without allowing any modifications, which is what the events I added do. They also don't interfere with fabric's events since they don't modify the message in any way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've talked to fabric maintainers about this. The most probably fix that we can put in fabric is just to make ALLOW_GAME not short curcuit. But they're skeptical of such use cases, and it might help if you bring this problem up with them.

Copy link
Collaborator Author

@Emirlol Emirlol Dec 13, 2024

Choose a reason for hiding this comment

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

I can see why they wouldn't want to make a breaking change like that, and using ALLOW_GAME to listen to all incoming messages is still going to be confusing due to the name and the unnecessary return value, so I'm not sure if that's the solution I'd want. Perhaps I could PR this event to fabric, but I can't think of any use cases outside of this either.

@LifeIsAParadox LifeIsAParadox added changes requested This PR need changes and removed reviews needed This PR needs reviews labels Dec 12, 2024
@Emirlol Emirlol force-pushed the powder-mining-tracker branch from 43f8b6d to 33a9cc8 Compare December 13, 2024 12:32
@LifeIsAParadox LifeIsAParadox added reviews needed This PR needs reviews and removed changes requested This PR need changes labels Dec 13, 2024
@AzureAaron AzureAaron added the merge conflicts This PR has merge conflicts that need solving. label Dec 22, 2024
@LifeIsAParadox LifeIsAParadox removed the merge conflicts This PR has merge conflicts that need solving. label Dec 23, 2024
@Emirlol Emirlol force-pushed the powder-mining-tracker branch from 0f2fb40 to 3d6642b Compare December 23, 2024 09:11
@LifeIsAParadox LifeIsAParadox added changes requested This PR need changes merge conflicts This PR has merge conflicts that need solving. and removed reviews needed This PR needs reviews changes requested This PR need changes labels Dec 24, 2024
@Emirlol Emirlol force-pushed the powder-mining-tracker branch from 3d6642b to 074b818 Compare December 24, 2024 04:13
@LifeIsAParadox LifeIsAParadox added reviews needed This PR needs reviews and removed merge conflicts This PR has merge conflicts that need solving. labels Dec 24, 2024
Copy link
Collaborator

@AzureAaron AzureAaron left a comment

Choose a reason for hiding this comment

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

looks good

@LifeIsAParadox LifeIsAParadox added merge me please Pull requests that are ready to merge and removed reviews needed This PR needs reviews labels Dec 30, 2024
@AzureAaron AzureAaron merged commit a09c6ee into SkyblockerMod:master Jan 1, 2025
1 check passed
@LifeIsAParadox LifeIsAParadox removed the merge me please Pull requests that are ready to merge label Jan 1, 2025
@Emirlol Emirlol deleted the powder-mining-tracker branch January 2, 2025 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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