Fabric API

Fabric API

106M Downloads

Rework resource reloader API

Technici4n opened this issue ยท 7 comments

commented

Goals:

  • Allow communication between resource reloaders.
  • Allow ordering of modded reloaders.
  • Allow ordering of modded and vanilla reloaders (including running before some vanilla reloaders).
  • Prevent changes to the ordering of vanilla reloaders.

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);
    }
}
commented

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.

commented

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.

commented

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.

commented

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.

commented

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. ๐Ÿ˜„

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.

commented

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.

commented

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