Add Support for Custom Item Settings

[Haven-King]

Currently, if a mod wants to pass custom settings for items, the best way is to subclass either Item.Settings or FabricItemSettings and add the behavior there yourself. An example of this is in The Aether, where custom item settings are used for a custom rarity and enchantment glint color. If another mod, however, adds an item setting for their items via the same method, there's no way for one item to share both properties.

This pull request solves that by allowing mods to attach arbitrary data to item settings. As part of this change, both the EquipmentSlotProvider and CustomDamageHandler have been migrated to use this system. Their public-facing API's remain unchanged.

This pull request also fixes a small bug in the testmod, wherein one of the test entrypoints would fail to launch.

[liach]

Pretty good. I assume these settings are all used by other vanilla items in modified bytecode, right? If those are exclusively used by mods mods should just add extra constructor parameters.

[Haven-King]

Pretty good. I assume these settings are all used by other vanilla items in modified bytecode, right? If those are exclusively used by mods mods should just add extra constructor parameters.

Adding constructor parameters isn't an elegant solution when you want to be able to add settings to a wide range of item types, potentially from different mods, or if you want to ensure a large group of items have the same settings.

[liach]

https://github.com/FabricMC/fabric/pull/1359/files#diff-9e98586cb53b15bc81915c0e3bf96ce908536638e4e075730d83a9bf53153404R29-R30

[Technici4n]

So I thought of another way of doing this, based on one of Lambda's idea.

Let's say we add .then(Consumer<Item>... itemConsumers) to FabricItemSettings that registers consumers to be invoked at the end of the item constructor, then we could achieve exactly the same thing, and we could replace ExtraData with List<Consumer> basically.

For example

public FabricItemSettings equipmentSlot(EquipmentSlotProvider equipmentSlotProvider) {
    FabricItemInternals.computeExtraData(this).equipmentSlot(equipmentSlotProvider);
}

becomes

public FabricItemSettings equipmentSlot(EquipmentSlotProvider equipmentSlotProvider) {
    return then(item -> ((ItemExtensions) item).fabric_setEquipmentSlotProvider(equipmentSlotProvider);
}

By separating the Consumer<Item> from the item settings, this can then be trivially expanded upon by other mods, for example:

public static Consumer<Item> fuel(int burnTicks) {
    return item -> FuelRegistry.add(item, burnTicks);
}

// used like this:
.then(fuel(42))

The only downside is that it's not possible to get the current setting as it's stored within the lambda, but the current PR doesn't provide that option either.

[LambdAurora]

@Technici4n imo, the consumers being executed at the end of the constructor and not after item registration is a bit painful as it would prevent calling anything that relies on the item to be in the register.
Other issue is fake items stuff, it's possible to instantiate an item without registering it, not recommended but the consumers being executed at constructor end would very likely cause issues in that case.

[Technici4n]

If you register a then() for your own fake Item then that's not my problem. ^^

[Haven-King]

I agree that .then would be a good feature, but I don't see how it relates directly to this PR. I suppose it could replace the intermediary step of copying the map in the constructor, but that has its own issues. The most important issue I see is that when then is fired you expect the item to be fully initialized, which might not be the case if custom item settings are being copied via the same method.

[LambdAurora]

If you register a then() for your own fake Item then that's not my problem. ^^

Can't wait to have to strip every consumers from the re-usable settings for the fake item :|

Plus it's mutable and there's no way to copy an item settings properly so really not happy about that.

[i509VCB]

With regards to new item settings added by fabric api, those should still recieve dedicated setting methods.

[LambdAurora]

Uh... The usual rule of thumb in Fabric API is that parameters shouldn't be null unless @Nullable is specified, we don't really use @NotNull annotations here.

[liach]

Also I meant to add Objects.requireNonNull calls than @NotNull annotations.

[sfPlayer1]

The is an overlap with the api-lookup api that needs further invesigation.

This PR handles both a) transporting the data from Item.Settings.custom(data) to the constructor and b) holding and retrieving said data externally with any item instance. The latter is what api-api does as well, although significantly less conveniently.

[sfPlayer1]

I forgot to mention there that adding such a mechanism would imply doing the same for blocks. I don't really want to clutter the interface with a generic arg for block vs item, so some code duplication is in order. Not sure how much the impl can be shared.

[Haven-King]

I do plan on adding a similar mechanism for block settings. I want to get this PR merged as a precedent for how I will handle blocks.

[PseudoDistant]

@sfPlayer1 was something like this ever finished?

I could pick it up if needbe.

[liach]

Can probably be replaced by #2172

[PseudoDistant]

Ah, maybe.

[Technici4n]

@liach No, this PR is for immutable data that's stored in the Item and set at creation time. #2172 is for mutable data that is attached to many game objects (worlds, chunks, entities, block entities) and can be updated / persisted.