TESLA

TESLA

15M Downloads

Drop EnumFacing parameters from ITeslaHandler

Darkhax opened this issue ยท 9 comments

commented

Arguments in favor

  • You already pass a face to the capability user when you get access to the capability. Using additional face checks is redundant.
  • It will make the intention of the API much clearer, and prevent developers from picking up bad habbits such as using up for getting the capability.
  • The current confusion makes it difficult to implement side awareness. Some devs will do side awareness in the getCapability method while others will use the internal capability methods.

Arguments against

  • Mod authors would have to maintain multiple instances of the ITeslaHandler for multiple faces or use an internal buffer if they want to have sided logic on things like getCapacity.
commented

Arguments in favor

  • You already ask about an entity about the API on a specific side to use it from this side. Any additional side is just redundant or plain wrong.
  • It makes the API more robust. It prevents mod devs from say always query the top side for the capability and then just try different EnumFacing for this handler. Not all mod devs might consider this approach and it can therefore cause issues
  • It would make it harder to implement side awareness correctly. You have to handle it at capability level and at API level again.

This can technically be done by creating a wrapper that redirects method calls from the getCapability hook.

The wrapper is completely optional. If you do not care about any side aware handler, you can expose a single handler instance for everything. Or already handle side awareness on the capability level by not exposing a handler at all.
A wrapper is only necessary in very few cases. Like you want to use an object as internal buffer and have different sides handle different energy throughputs or similar.

Mod authors would have to maintain multiple instances of the ITeslaHandler

As above. They do not have to. But can, if they wish.

Mod authors would not have the same flexibility out of the box.

The only flexibile they use is that they cannot try to give/take power on a side different than they have asked the tile entity. Which is already a bad idea because why should it be acceptable that something does not report it supports the API on a specific side, but you can then still try another side and still use the first EnumFacing?

Arguments against

  • Being able to use all 36 or 49 permutations of a tuple of EnumFacing might allow some special cases for a mod. Like use a convention that using NORTH for the capability and SOUTH for the handler to indicate a wireless connection. But I would still consider the potential issues caused by it as more important than giving a way to basically abuse the API in a non standard way.
commented

Thank you @yueh for going in depth with some of your points. I did not mean to unfairly represent your suggestion, I just haven't had time complete the entire thing or ask you for further clarifications. I will work what you said into the main post.

commented

Just take your time. A good solution will probably take a few weeks for a first useable alpha/beta stage.

I would also recommend to actively reach out to any other energy API and ask them about their pitfalls or what they would like to improve, but is impossible without breaking the API.
Neither I nor you can imagine every issue. Especially if it should replace any of the common energy APIs.

commented

After messing around with this idea, I have come up with a solution that I like, and follows the example set forth by Vanilla/Forge. In this proposed solution I would create a new interface called ISidedTeslaHandler. This interface would extend ITeslaHandler and define a method for determining if a side is valid or not. This new method would function like the one described in #7 and would allow a container to optionally define the function of a side. This mimics the concept behind ISidedInventory in vanilla MC. From there, I would create another class called SidedTeslaWrapper which would serve as a default implementation for the new ISidedTeslaHandler, similarly to how forge provides a SidedInvWrapper.

commented

An ISidedTeslaHandler would be certainly a bad design decision.
Forge provides an ISidedInventory explicitly because vanilla has no sided inventories and it has to provide an alternative because it is very common in modded minecraft.

But capabilities are already a sided aware thing, thus an ISidedTeslaHandler will always be redundant.
Even worse either users are forced to use instanceof again. Something capabilities want to prevent. Or you have to provided two different capabilities and everyone is forced to handle both or risking incompabilities and making it more harder to implement a correct solution.

commented

Vanilla does actually have sided inventories. The furnace for example has different outcomes depending on where the hopper is pointing. ISidedInventory is a vanilla class. Forge only provides the SidedInvWrapper.

The reason for creating a sided interface method is so handlers can provide hints about what a face actually does. For example you can ask if a side is an input or an output. This can be done by using the getCapabilities method however I feel that is much less reliable. For example, if the upper side returns a Tesla Handler instance, that does not tell you if the side being used should be treated as an input/output, it only tells you that the side gives access to a handler. Further checks could be made to simulate attempts to give or take power, however if the capacity is full or empty, it would throw things off. I originally wanted to make this part of ISidedTeslaHandler to mimic what Vanilla does, however to prevent instance checks, it might be better to put such logic in the main interface, which is where the current version of these methods are.

As for the SidedTeslaWrapper, the goal of this class would be to provide a default thin wrapper that could be used, and to demonstrate how one could take sides into account in their own implementations.

commented

It really overlaps with #7 as prerequisite for an actual decision.

Removing the EnumFacing is really just about:
You already asked, if the TE supports it on side X. Do you you really need to ask again, if it supports it on X by asking the handler again?

By also splitting the handler into say IConsumer, IProducer and IHandler, you can do the following

if (te.hasCapability(IHandler, side)) { // can already exits here, if not give AND take
    handler = te.getCapability(IHandler, side); // only called when actually supporting it
    handler.givePower(n); // guaranteed to work
    handler.takePower(n); // guaranteed to work
}

Compared to having only a single capability and still having using EnumFacing for the handler:
More or less ignoring the "sideness" for capability.

if (te.hasCapability(IHandler, side)) { // no hard condition, except no support at all
    handler = te.getCapability(IHandler, side); // MUST be called to check further support. 
    // Might be more expensive than has()

    if (handler.isInputSide(side)) { // still no clue, have to explicitly check
        handler.givePower(n, side); // finally guaranteed to at least can given power to it
    }
    // can we use a other side here?
    // Even when already asked for a completely different one?
    // Maybe loop over EnumFacing until we find a working one?

    if (handler.isOutputSide(side)) { // still no clue, have to explicitly check
        handler.takePower(n, side); // finally guaranteed to at least can taken power from it
    }
}

If you just want to give or take power it is still

if (te.hasCapability(IConsumer, side)) { // Cannot support giving power on this side? done.
    handler = te.getCapability(IConsumer, side);
    handler.givePower(n);
}
if (te.hasCapability(IHandler, side)) { // Might support. Further checks needed.
    handler = te.getCapability(IHandler, side); // Have to call it

    if (handler.isInputSide(side)) { // Finally we can determine, if it can be given power.
        handler.givePower(n, side);
    }
}

Which ones look cleaner or is less ambiguous about what the right way might be?

commented

I think sided-ness should be handled by the capability provider, not by the handler.

eg. IItemHandler in forge doesn't have any side arguments to any of it's methods.

commented

Merging #6 and #7 into #10