Fluid Conduits - Iterate fluid tanks #840

Merged

Metalit merged 1 commit from release/1.20.1 into dev/1.20.1

2024-10-12 00:59:07 +00:00

Metalit commented

2024-10-01 00:17:05 +00:00

(Migrated from github.com)

Description

Changes the single extract for (non-locked) fluid conduits to an iteration over all the stored fluids. Should fix #791 (without needing anything on gregtech's side), and more significantly, fluid filters disabling conduits in some cases with a non-filtered fluid in the container. (Couldn't find an issue for that one.)
Not sure what the status is on 1.21, I saw there was a conduit-related PR recently but it didn't seem to touch the logic here.

(The iterative approach is referenced from ae2, so I assume it's probably a correct thing to do.)

My code follows the style guidelines of this project (.editorconfig, most IDEs will use this for you).
I have made corresponding changes to the documentation.
My changes are ready for review from a contributor.

# Description Changes the single extract for (non-locked) fluid conduits to an iteration over all the stored fluids. Should fix #791 (without needing anything on gregtech's side), and more significantly, fluid filters disabling conduits in some cases with a non-filtered fluid in the container. (Couldn't find an issue for that one.) Not sure what the status is on 1.21, I saw there was a conduit-related PR recently but it didn't seem to touch the logic here. (The iterative approach is referenced from ae2, so I assume it's probably a correct thing to do.) - [x] My code follows the style guidelines of this project (.editorconfig, most IDEs will use this for you). - [x] I have made corresponding changes to the documentation. - [x] My changes are ready for review from a contributor.

Rover656 commented

2024-10-01 11:22:57 +00:00

(Migrated from github.com)

Could you describe the problem that you are trying to solve here? The issue you've linked is confirmed to be a Greg Tech issue, so im not sure why you've made this change.

IFluidHandler#drain is designed to allow the underlying fluid handler to decide how to extract from the tanks - if a mod isn't implementing that correctly then it's that mod's problem to be honest, it exists to avoid us needing to add additional logic to loop over the tanks.

Could you describe the problem that you are trying to solve here? The issue you've linked is confirmed to be a Greg Tech issue, so im not sure why you've made this change. `IFluidHandler#drain` is designed to allow the underlying fluid handler to decide how to extract from the tanks - if a mod isn't implementing that correctly then it's that mod's problem to be honest, it exists to avoid us needing to add additional logic to loop over the tanks.

Rover656 commented

2024-10-01 11:23:34 +00:00

(Migrated from github.com)

Also, I will move the base branch to lts/1.20.1 for you, as that's where we do work on the mod :) (which reminds me I should get rid of release/1.20.1 like I did for 1.21)

Metalit commented

2024-10-01 16:19:50 +00:00

(Migrated from github.com)

The main issue this fixes is with fluid filters. As you said, IFluidHandler#drain will allow the underlying fluid handler to decide what fluid to provide. But if you had a tank with both water and lava in it and want to extract the lava, it might provide water every time you call drain, which wouldn't match the lava filter. With the current implementation, it would just stop there and fail to extract anything - that's what this fixes.

The main issue this fixes is with fluid filters. As you said, `IFluidHandler#drain` will allow the underlying fluid handler to decide what fluid to provide. But if you had a tank with both water and lava in it and want to extract the lava, it might provide water every time you call `drain`, which wouldn't match the lava filter. With the current implementation, it would just stop there and fail to extract anything - that's what this fixes.

Rover656 (Migrated from github.com) approved these changes

2024-10-12 00:51:57 +00:00

Rover656 (Migrated from github.com) left a comment

After thinking about this, I think I'm happy with this approach. Thank you.