-
-
Notifications
You must be signed in to change notification settings - Fork 190
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[1.21.4] Add model loading plugins to replace ModelEvent.ModifyBakingResult
#1884
base: 1.21.x
Are you sure you want to change the base?
Conversation
Last commit published: 1530b5bfd43eb5e5b74a74aaa2b85dcb809bd240. PR PublishingThe artifacts published by this PR:
Repository DeclarationIn order to use the artifacts published by the PR, add the following repository to your buildscript: repositories {
maven {
name 'Maven for PR #1884' // https://github.com/neoforged/NeoForge/pull/1884
url 'https://prmaven.neoforged.net/NeoForge/pr1884'
content {
includeModule('net.neoforged', 'neoforge')
includeModule('net.neoforged', 'testframework')
}
}
} MDK installationIn order to setup a MDK using the latest PR version, run the following commands in a terminal. mkdir NeoForge-pr1884
cd NeoForge-pr1884
curl -L https://prmaven.neoforged.net/NeoForge/pr1884/net/neoforged/neoforge/21.4.76-beta-pr-1884-model_loading_plugins/mdk-pr1884.zip -o mdk.zip
jar xf mdk.zip
rm mdk.zip || del mdk.zip To test a production environment, you can download the installer from here. |
3c4627c
to
4b1c988
Compare
private static final Map<ResourceLocation, ModelModifier.ModifyOnLoad> ON_LOAD_MODIFIERS = new Object2ReferenceLinkedOpenHashMap<>(); | ||
private static final Map<ResourceLocation, ModelModifier.ModifyBeforeBake> BEFORE_BAKE_MODIFIERS = new Object2ReferenceLinkedOpenHashMap<>(); | ||
private static final Map<ResourceLocation, ModelModifier.ModifyAfterBake> AFTER_BAKE_MODIFIERS = new Object2ReferenceLinkedOpenHashMap<>(); | ||
private static final Map<ResourceLocation, ModelModifier.ModifyBlockOnLoad> ON_LOAD_BLOCK_MODIFIERS = new Object2ReferenceLinkedOpenHashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These would be better off as a list of pairs (or records); I don't see you ever querying these maps by key, and iterating over a list should be a bit faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can kinda see the point but I'm not sure the benefit of marginally faster iteration is large enough to justify making duplicate detection during registration a complete pain (unless I register into temporary maps and flatten those into lists afterwards).
import org.jetbrains.annotations.Nullable; | ||
import org.slf4j.Logger; | ||
|
||
@ApiStatus.Internal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be ideal if this wasn't internal, so ModernFix doesn't have to reimplement the entire class (or more likely, use it anyway knowing it could break).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just made it internal as a starting point with the intention of relaxing it later because I'm not sure yet what the best way would be to expose this for use by ModernFix and others, particularly for the blockstate on-load modifiers and resolvers since you'd want to be able to run those on a per-block basis or even per-model basis for the former and a per-block basis for the latter.
private final Map<Block, BlockStateResolver> resolvers; | ||
private final BiFunction<ResourceLocation, ModelModifier, @Nullable ModelModifier> modifierRegistrar; | ||
|
||
@ApiStatus.Internal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See other comment about this being internal; any mod implementing dynamic model loading probably needs to fire this (if the other class stays internal).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally would prefer exposing the necessary parts of ModelLoadingPluginManager
.
src/main/java/net/neoforged/neoforge/client/event/ModelEvent.java
Outdated
Show resolved
Hide resolved
4b1c988
to
6135c07
Compare
This PR adds model loading plugins which are intended to replace
ModelEvent.ModifyBakingResult
. These allow more precise model wrapping, including wrapping unbaked models, and support loading custom blockstate-like formats. Another benefit of this API design is that it allows mods which implement on-demand model loading/baking a lot more control and improves compatibility between those mods and ones using the model wrapping APIs.As is probably very obvious, this PR is very heavily inspired by Fabric API's model loading APIs and therefor has an almost 100% overlap in terms of API surface and injection points.
The model loading plugins provide seven entrypoints into the model loading and baking process:
BlockStateResolver
s: blockstate resolvers are registered for specific blocks and are responsible for providing the state->model mapping for all states of the block they are registered for. This allows implementing custom blockstate-like formats, among other thingsModelEvent.ModifyBakingResult
)Apart from the missing documentation, there are still a few questions open:
BlockStateResolver
s provide access to theUnbakedBlockStateModel
s loaded by vanilla as this allows use cases where all models of a particular block have to be wrapped without having to fall back to checking every single model going through the pipeline in an on-load-block modifier.Should this access be limited to models for the states being resolved or should it be able to provide access to any model (I have a use case for this which also relies on the other block's resolver having run) which has the downside of potentially recursive resolution with on-demand loading/baking?