Allow registration of data resource reloader using a factory and provide a store to store data reload results. #4944
Conversation
…and provide a mechanism to store data in `MinecraftServer` once a resource reload completes.
LambdAurora
added
enhancement
modmuss50
added
the
priority: high
modmuss50
left a comment
modmuss50
left a comment
There was a problem hiding this comment.
Nothing stands out to me about the API, but get a review from someone who riased this issue in the first place. Something does look fishy about the injected interface tho.
modmuss50
added
the
status: last call
Earthcomputer
left a comment
Earthcomputer
left a comment
There was a problem hiding this comment.
The API looks good, it addresses my primary concern about not being able to use JsonDataLoader easily.
|
|
||
| @SuppressWarnings("AddedMixinMembersNamePattern") | ||
| @Override | ||
| public <T> T getOrThrow(Key<T> key) { |
There was a problem hiding this comment.
Super generic method name mixined to MinecraftServer looks a little risky, but it's fine because we own the Key class.
| */ | ||
| void registerReloader( | ||
| Identifier id, | ||
| Function<RegistryWrapper.WrapperLookup, ResourceReloader> factory |
There was a problem hiding this comment.
I'm a bit confused why we are using a WrapperLookup here, rather than the DataResourceStore. Is there a good reason?
There was a problem hiding this comment.
Good question, the main need for this overload is for JsonDataLoader and it only needs WrapperLookup, I'm a bit concerned about how confusing the DataResourceStore might be here.
Also the DataResourceStore does not contain the WrapperLookup, the lookup and other reloaders are also available through the Vanilla ResourceReloader shared state store.
…ide a store to store data reload results. (#4944) * Allow once again to register data resource reloader using a factory, and provide a mechanism to store data in `MinecraftServer` once a resource reload completes. * Remove dead code, add access to advancement and recipe loaders. * Fix import order of `DataResourceLoader`. * Fix interface injection of `DataResourceStore` into `MinecraftServer`. (cherry picked from commit d4d4122)
3 participants
After some discussion on Discord, this PR aims to fix the headaches of using
JsonDataLoaderwith Resource Loader v1.This PR also introduce a new data store for results of a data resource reload as this was requested as well and it is true that currently there's no good way to store data in ways that respects its lifecycle.
There are some things I'm not yet sure in this design, notably with how to access such store using a
MinecraftServerinstance, right now a bareboneDataResourceStoreis interface injected onto it to have the least friction to gather data (thinkserver.getOrThrow(DATA_KEY)), this should be safe in terms of injection as the method as a unique parameter to the resource loader (DataResourceStore.Key), the alternative would be to have a provider interface to inject and doserver.getDataResourceStore().getOrThrow(DATA_KEY).