BuildCraft|Builders

BuildCraft|Builders

6M Downloads

Should ITankBlockConnector be moved the the API?

Indemnity83 opened this issue · 4 comments

commented

I'll admit I'm not fluent with the codebase and how it functions, but I'm wondering why the ITankBlockConnector interface isn't part of the buildcraft.api.blocks package?

package buildcraft.factory.block;
import buildcraft.factory.tile.TileTank;
/** Marker interface for {@link BlockTank} to determine if the block below should be considered a tank block when
* visually connecting below. This should only really be implemented when the block creates an instance of
* {@link TileTank}. */
public interface ITankBlockConnector {}

I ask primarily because my IronTanks mod is very reliant on the core BuildCraft codebase and I'm looking into ways to implement it using the more stable API. This is one of the pieces of that puzzle that has no API equivalent (I can't get a BuildCraft tank to "stack" with non-BuildCraft blocks short of pulling in this core BuildCraft interface.

commented

Currently it's because of the "when the block creates an instance of {@link TileTank}" part of the javadoc: TileTank is hardcoded to only balance fluids with other TileTank instances, and ITankBlockConnector just allows custom Block classes (which still create a TileTank instance) to cisually connect.

I think to move it to the API properly I'd need to either expose a "createTileTank" method (which won't work very well if you need to extend it), or make TileTank instead work with other tile entity types if they also implemented an interface for managing fluid balancing.

commented

Now that you say that I think we’ve talked about this before.

Not sure if I can put together an elegant solution, but would you be interested in a PR that pulls this interface into API and likely creates another for the tile entity?

commented

Yes, I would accept a PR like that.

However I'm concerned that it wouldn't actually make it easier in the future to update to newer versions of minecraft+buildcraft, because you'd effectively have to re-implement everything that buildcraft's TileTank does in order to keep it compatible - whereas extending TileTank for all of your custom tanks will make it much simpler as then all you have to do is override a few methods and buildcraft can take care of the rest.

(It really doesn't help that I'm porting to fabric for 1.14, rather than forge, which has a whole different set of API's for fluid handling).

Essentially I don't think there's a very good solution to this, as you're always going to be somewhat reliant on buildcraft's internals for a mod that so closely duplicates buildcraft's functionality.

commented

I'll look into it, maybe I can just figure out the architecture that benefits all to help with the implementation in fabric.

For now, I'll close this issue, and if something comes out of my tinkering I'll submit a PR and we can discuss it in more there.