-
Notifications
You must be signed in to change notification settings - Fork 150
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
Create the Fluid Voiding cover #1529
base: master
Are you sure you want to change the base?
Create the Fluid Voiding cover #1529
Conversation
I don't really see it's usability as a cover if it just voids everything. I would recommend reworking it to allow "void on overflow" kind of functionality additionally, which would make it much more useful. |
There are some recipes where there is an output that I would rather void every time the recipe runs, like recipes that have water, hydrogen, diluted x, etc, as an output in addition to another fluid. I would rather void these fluids and never have to move them into fluid storage. An overflow cover would also have its usages, but could be implemented in addition to this cover to allow for voiding fluids once the internal tank limit is reached, instead of voiding fluids all the time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have partially reviewed this but then I got stuck. And some questions from given implementation arose. Most important is why IFluidVoiding
is not just extending IFluidHandler
? In that case you would not need to change GTFluidUtils
probably at all and it would simplify this quite a lot. Or is there something which prevents you to do that and I missed it?
Other comments bellow can be for now ignored.
return 0; | ||
} | ||
|
||
return voidingAmount; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should return amount of resource which will be transferred and that is maximally amount that is available all maximum which can given tier of cover transfer (in case that there would be tiers).
I will look into addressing the first comment about extending |
I definitely need to take another look to this as describe functionality regarding how void works seems strange to me. |
ec3763f
to
993afa4
Compare
993afa4
to
f77e523
Compare
I need to review what I have done in this PR, as I am not quite sure it is implemented how it was expected to be, or as cleanly as possible. |
What:
As mentioned in #1141, there is a desire for a fluid voiding implementation, including a fluid voiding cover. This PR attempts to implement such a Fluid Voiding cover and by doing so addresses the first point brought up in #1141.
How solved:
The Fluid Voiding cover does not actively pull fluids into itself to void, but instead will void any fluids pushed through it or into it that match the filter.
To accomplish this:
IFluidHandler
s for the target inventories andPredicate
s used for the fluid filter. This way, the fluid can go through the cover, be voided if it matches the filter, and into a target fluid handler inventory.pushFluidsIntoNearbyHandlers
inMetaTileEntity
was adjusted so that the Voiding Cover is a valid destination for the target push, even if there is no fluid handler in the blockspace that the fluid output is facing.The model for this item was taken from one of the unused assets in the resources folder.
Outcome:
Introduce a Fluid Voiding Cover.
Additional info:
This section is for screenshots which are required for any GUI, rendering or recipe changes. Or any additional info that reviewers should be aware of.
Possible compatibility issue:
The changes to the fluid transfer should be fine, as the original method was redirected to the new method. The changes to MTE should also be fine; any addons that were overriding the push method though will not have compatibility for that machine for the new cover