Spectrum icon indicating copy to clipboard operation
Spectrum copied to clipboard

Ink Transfer API

Open imreallybadatnames opened this issue 1 year ago • 13 comments

Adds FAPI's Transfer API support for ink.

Since I'm hoping to actually get the whole R&D & discussion localized within this PR's thread, I'd rather have this specific PR be refactored multiple times than be closed and reopened with a different implementation.

DONE:

  • Implement Transfer API for the storages themselves.
    • It is currently an admittedly questionable design, but it will hopefully be refactored and ironed out to an acceptable design by the end of this PR's dev process.
    • Coupling the storages to the actual storage holders for proper transaction commit handling is a TODO. Also related to the lookups question.

TODO:

  • Implement ink storage lookup mechanisms.
    • Create lookups for the current API's interfaces? Go all-in into Transfer API? Or have both lookups?
    • What context parameters should we use?
  • Migrate to Transfer API usage
    • Probably. We could keep the current ink storage APIs as a second option, though juggling between Transfer API usage and the current API usage could result in undefined behavior if not coded carefully.
    • Repurpose InkStorageItem and InkStorageBlockEntity as context-holding transfer processing wrappers or (in line with Fabric) as context-holding ink storage wrappers (being ink storages themselves)
      • Maybe add a way to attach mark-dirty callbacks to ink storages dynamically/at creation time? Would (mostly) rid of the current storage context classes, but could be flaky if not done carefully
      • What would the wrappers look like and how would they work?
  • Actually use the APIs
    • Probably a thing for the future major updates, but using them now would at least give a good head start.
    • Do we bake the pressurized transfer system into the very transfer API, or should we keep the old logic of doing so in separate logic? Including the question since DaFuqs asked about this.

imreallybadatnames avatar Aug 22 '24 06:08 imreallybadatnames

Marked as draft because duh.

imreallybadatnames avatar Aug 22 '24 06:08 imreallybadatnames

cc @DaFuqs @Noaaan (discussing the TODOs)

imreallybadatnames avatar Aug 22 '24 06:08 imreallybadatnames

Damn not CCing me. Meanie

Azzyypaaras avatar Aug 22 '24 06:08 Azzyypaaras

i forgor :skull:

imreallybadatnames avatar Aug 22 '24 06:08 imreallybadatnames

Anyways, do you currently have any comments relating to the PR? (apart from killing me with exploding hammers)

imreallybadatnames avatar Aug 22 '24 06:08 imreallybadatnames

I think it is best if we just keep it close to how the preexisting TAPI stuff works, no reason to reinvent the wheel when there is so much convention.

Also, imho the pressure system should not be inforced in-api. Advanced details like that should be left to the implementer, especially since they may have good reasons why they would want to avoid it (eg: AE2 ink storage)

Azzyypaaras avatar Aug 22 '24 06:08 Azzyypaaras

There is no sensible reason AE2 shouln't follow the same convention. Everything that does will inherently allow the player to break progression and encourage an afk playstyle, compared to upgrading the network

DaFuqs avatar Aug 22 '24 07:08 DaFuqs

I will have to look at the todos later, but I like the general idea. What benefits would it have, compared to the current system?

DaFuqs avatar Aug 22 '24 07:08 DaFuqs

There is no sensible reason AE2 shouln't follow the same convention. Everything that does will inherently allow the player to break progression and encourage an afk playstyle, compared to upgrading the network

Firstly. No, there is absolutely sensible reason why AE2 could disregard that convention, as it could (and imo should) come into play in endgame where it doesn't make sense. Plus it is kind of necessary for any sensible implementation of ink storage I kind of feel. Regardless though, what I feel does not matter as it is not our job to dictate what addons should or should not be doing.

It is our job to give users as much power to make what they desire as possible. It is their job to make it balanced. Neither of those things imply us enforcing a design ideal unto the very foundations of the api. We do not decide what people get to make.

Azzyypaaras avatar Aug 22 '24 07:08 Azzyypaaras

Generally the main benefits for implementing FTRAPI would be mod compat would be very straight forward - you would simply use Fabric API Lookup API (aka. FAPI API API) to query the storage before transfer. You would also swap from simulations to transactions for the transfer themselves, which should be more robust.22. aug. 2024 09:05 skrev DaFuqs @.***>: I will have to look at the todos later, but I like the general idea. What benefits would it have, compared to the current system?

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

Noaaan avatar Aug 22 '24 11:08 Noaaan

Create lookups for the current API's interfaces? Go all-in into Transfer API? Or have both lookups? / Migrate to Transfer API usage

  • If we do this an all-in sounds like the best solution, so we do not have to tinker with two different ways to do things.
  • One thing that might become tricky if we do this route is NeoForge. Since we plan to port there eventually and transfers are done differently, we might have to re-do the whole construct for two loaders. Might be nice to know how others handle this
  • As noted, we would need to communicate to other mods that use the Transfer API, that the sensible reason is to equal out containers, instead of moving arbitrary amounts from a->b

What context parameters should we use?

I feel the default FAPI TransactionContext will do for the most part. Source Storage, Destination Storage and optionals for block & player (for advancement shenanigans).

DaFuqs avatar Aug 26 '24 06:08 DaFuqs

bump

f-raZ0R avatar Oct 15 '24 23:10 f-raZ0R

this thread dead as fuck lmAo

imreallybadatnames avatar Jan 01 '25 05:01 imreallybadatnames