FabricMC / fabric

Essential hooks for modding with Fabric.
Apache License 2.0
2.36k stars 416 forks source link

Rework resource reloader API #2577

Open Technici4n opened 2 years ago

Technici4n commented 2 years ago

Goals:

Here is my proposed solution, for server resources. I think we can do something similar for client resources. I am not sure that we need all the AFTER_XXX events.

interface AddServerResourceReloaderCallback {
    // Choose event for ordering wrt. vanilla, and use event phases to order wrt. other modded reloaders.
    Event<AddServerResourceReloaderCallback> BEFORE_ALL = create(...); // before all vanilla reloaders
    Event<AddServerResourceReloaderCallback> AFTER_TAGS = create(...); // after tags
    Event<AddServerResourceReloaderCallback> AFTER_LOOT_CONDITIONS = create(...); // after tags and loot conditions
    Event<AddServerResourceReloaderCallback> AFTER_RECIPES = create(...); // after tags, loot conditions, and recipes
    Event<AddServerResourceReloaderCallback> AFTER_LOOT = create(...); // etc...
    Event<AddServerResourceReloaderCallback> AFTER_LOOT_FUNCTIONS = create(...); // etc...
    Event<AddServerResourceReloaderCallback> AFTER_FUNCTIONS = create(...); // etc...
    Event<AddServerResourceReloaderCallback> AFTER_ADVANCEMENTS = create(...); // etc...
    Event<AddServerResourceReloaderCallback> AFTER_ALL = create(...); // after all vanilla reloaders

    // Server allows getting extra state in the event, but should not be used during the reload!
    // Data pack contents allow querying vanilla reloaders
    // Registry allows adding and querying modded reloaders
    void addResourceReloaders(MinecraftServer server, DataPackContents dataPackContents, ModdedReloaders registry);

    interface ModdedReloaders {
        void add(Identifier identifier, ResourceReloader reloader);
        @Nullable
        ResourceReloader get(Identifier identifier);
    }
}
LambdAurora commented 2 years ago

This API proposition is very hardcode-heavy.

Making Resource Reloaders able to be ordered directly would be both simpler to use, more extensible, and will result in a more stable API. It would also result in something much more consistent to register resource reloaders.

Not to mention how AFTER_TAGS could be confusing to users thinking that having a reloader after that moment gives you free access to tags (it doesn't since they only get applied AFTER resource reloading is done).

The Phase Ordering stuff is great, I'd rather see it being made more abstract to be able to be extracted out of events. To keep a "before/after Vanilla" a "proxy" identifier could be created in such system.

Technici4n commented 2 years ago

My concern with a before/after approach is that it's easy to introduce accidental reorderings of vanilla's resource reloaders.

Maybe something like this would work better? (I really don't like the identifiers)

public enum VanillaReloader {
    TAGS, LOOT_CONDITIONS, RECIPES, LOOT, LOOT_FUNCTIONS, FUNCTIONS, ADVANCEMENTS
}

Event<AddServerResourceReloaderCallback> BEFORE_VANILLA = create();
public Event<AddServerResourceReloaderCallback> afterVanilla(VanillaReloader... vanillaReloaders) {
    // Compute earliest event that satisfies the constraints
}
Event<AddServerResourceReloaderCallback> AFTER_ALL_VANILLA = create();

Not to mention how AFTER_TAGS could be confusing to users thinking that having a reloader after that moment gives you free access to tags (it doesn't since they only get applied AFTER resource reloading is done).

You have access to tags in the apply phase if you query them through the TagManager in the DataPackContents - this is actually one of my motivations, as it would make the "tags populated" resource condition a lot cleaner to implement. But I agree that it would require proper documentation.

Technici4n commented 2 years ago

A problem with the current identifier system is that you can't easily request to run a reloader after "all"/most existing reloaders. With event phases, it would be possible to run a reloader after all reloaders in the default phase, which is already better for something like resource conditions that tries to also work for modded simple JSON reloaders.

LambdAurora commented 2 years ago

You have access to tags in the apply phase if you query them through the TagManager in the DataPackContents - this is actually one of my motivations, as it would make the "tags populated" resource condition a lot cleaner to implement. But I agree that it would require proper documentation.

The issue here is methods like isIn from BlockState won't work properly. That's how it can be confusing.

A problem with the current identifier system is that you can't easily request to run a reloader after "all"/most existing reloaders. With event phases, it would be possible to run a reloader after all reloaders in the default phase, which is already better for something like resource conditions that tries to also work for modded simple JSON reloaders.

What?... I'm still not sure I understand the motivation here.

Technici4n commented 2 years ago

The issue here is methods like isIn from BlockState won't work properly. That's how it can be confusing.

Yeah obviously. But IMO resource reloaders are somewhat tricky and should be careful with what they're doing. :smile:

What?... I'm still not sure I understand the motivation here.

The idea was to be able to clear a thread local cache that runs after all SimpleJsonResourceReloader instances. Maybe there are other ways to do it.

But in any case all of this stuff is still somewhat low priority, and mostly posted here to start a discussion. I have other things to take care of first.

LambdAurora commented 2 years ago

The idea was to be able to clear a thread local cache that runs after all SimpleJsonResourceReloader instances. Maybe there are other ways to do it.

I'd rather look into the existing events that are triggered after a complete reload and make them more consistent to make them usable for that kind of use case.

Technici4n commented 1 year ago

3083 should address all of the bullet points in the issue description. I chose to use the same ordering system as event phases, with a twist to make sure that the ordering of vanilla resource reloaders can never be compromised.