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

FIX: Use correct priority increment amounts #192

Merged
merged 1 commit into from
Feb 29, 2024

Conversation

michaeldoylecs
Copy link

@michaeldoylecs michaeldoylecs commented Feb 26, 2024

Problem

When setting the priority on an ME Fluid Storage Bus, the displayed button values are using the craftAmtButton# config values instead of the priorityAmtButton# config values. This is inconsistent with the non-fluid ME Storage Bus behavior.

The interface in question:
image

Solution

Add an overridable method, getIncrementQuantity, to FCGuiAmount so that the subclass GuiFCPriority can specify to use priority values instead of the default crafting values.

Adds an overridable method to FCGuiAmount to allow subclasses to specify
increment values other than the default.

AE2 has config values to set how much each button in the priority screen
should increment a fluid storage bus's (or other's) priorty by. Prior to this
change, the priority interface on fluid storage bus' were using the
increment values for crafting, instead of those for priority.
@michaeldoylecs michaeldoylecs changed the title Use correct priority increment amounts FIX: Use correct priority increment amounts Feb 26, 2024
@Dream-Master Dream-Master requested review from Laiff, a team and MCTBL February 26, 2024 19:10
@MCTBL
Copy link
Member

MCTBL commented Feb 28, 2024

is this really necessary to add this encapsulation?

@Hikari1nVoid
Copy link

Increase code complexity for no reason.

@michaeldoylecs
Copy link
Author

michaeldoylecs commented Feb 28, 2024

is this really necessary to add this encapsulation?

It is not. I would actually prefer having a separate class for priority gui. I went with this approach since it was a significantly smaller commit and can be both reviewed, added, or removed quite quickly if ever wanted to. If the alternative approach, which would match base AE2 implementation, is suggested, I can update with that.

Copy link
Member

@Caedis Caedis left a comment

Choose a reason for hiding this comment

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

I actually had this exact same fix ready but got distracted by something else

image

@Caedis Caedis merged commit 5cc8488 into GTNewHorizons:master Feb 29, 2024
1 check passed
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