Spoilage

[TarLaboratories]

What

Spoilage yay!
Added API to modify item outputs and read item inputs in recipe modifiers.
Also made an event that fires just before a machine is registered to allow modifying it's parameters from addons (most notably add recipe modifiers)

Implementation Details

Stores the creation tick of a spoilable item in its nbt, thus bypassing the need to actively tick it.
Should be noted that the item has to be updated at least once after it finished crafting for it to start spoiling (done by updating items inserted into CustomItemStackHandler)
Basically everything is done via a mixin into ItemStack lols
Doesn't actually make any items spoilable, only adds the functionality to do so.

Outcome

GLEBA

[jurrejelle]

Very quick review before I have to go do other things, these are my initial thoughts

[jurrejelle]

Why is this done in isSameItemSameTags? Now, when we e.g. compare two itemstacks for recipe checking but the recipe can't run, it will still assign the average spoil value

[jurrejelle]

To be fair, it's very difficult to know when to merge spoilability status and items don't really have a "merge" function. I'm not sure how we would implement this

[jurrejelle]

Some thoughts I had laid out in the discord, ramble style, that I'd like to document here:

1. The thing with isItemStackEqual being used as a "merge" check when there might be a case that we're just checking without merging, although I have no clue how to solve that, any ideas welcome
2. The fact that we have to deeply modify GTRecipe and NotifiableItemStackHandler means our API isn't flexible enough to do these things without. Ideally, when implementing content features, we setup API and then use said API instead of messing with internals. Probably this can be done as a RecipeModifier and then we add API to attach to existing machines? then use a new or existing forge event to let users modify all machines to attach the new RecipeModifier? More input needed. But we definitely should do / be able to do this with public API, otherwise we should improve the API until we can.
3. The fact that when things spoil, the item just gets turned in the spot. Gameplay wise you'd hope it'd get put somewhere else like in the output bus, or at least it's possible to extract them (which is impossible with input busses since the items are inaccessable)
4. I'm a little worried at the overhead of checking/updating the state at each of the following: method = { "getItem", "getCount", "getTag", "getOrCreateTag", "getTagElement", "getOrCreateTagElement","getItemHolder" }. Maybe we can profile that or have others look at the method when reviewing
5. (tiny thought) Injecting into ItemStack this heavily might break compatibility with other mods, not 100% sure, I'm not super well known on mixins
6. With how many internal methods this touches I would not be comfortable having this in this release cycle without it being in testing / snapshot for a while, although that might be mitigated with the "testing release" that we're planning. More input needed by others too

Stale