Skip to content
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

Ink Transfer API #462

Draft
wants to merge 9 commits into
base: 1.20.1-aria-for-painters
Choose a base branch
from

Conversation

imreallybadatnames
Copy link
Contributor

@imreallybadatnames imreallybadatnames commented Aug 22, 2024

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 imreallybadatnames marked this pull request as draft August 22, 2024 06:29
@imreallybadatnames
Copy link
Contributor Author

Marked as draft because duh.

@imreallybadatnames
Copy link
Contributor Author

cc @DaFuqs @Noaaan
(discussing the TODOs)

@Azzyypaaras
Copy link
Collaborator

Damn not CCing me. Meanie

@imreallybadatnames
Copy link
Contributor Author

i forgor 💀

@imreallybadatnames
Copy link
Contributor Author

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

@Azzyypaaras
Copy link
Collaborator

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)

@DaFuqs
Copy link
Owner

DaFuqs commented Aug 22, 2024

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
Copy link
Owner

DaFuqs commented Aug 22, 2024

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?

@Azzyypaaras
Copy link
Collaborator

Azzyypaaras commented Aug 22, 2024

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.

@Noaaan
Copy link
Collaborator

Noaaan commented Aug 22, 2024 via email

@DaFuqs
Copy link
Owner

DaFuqs commented Aug 26, 2024

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).

@f-raZ0R
Copy link
Contributor

f-raZ0R commented Oct 15, 2024

bump

@imreallybadatnames
Copy link
Contributor Author

this thread dead as fuck lmAo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants