Skip to content
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

Draft
wants to merge 3 commits into
base: 1.21.x
Choose a base branch
from

Conversation

XFactHD
Copy link
Member

@XFactHD XFactHD commented Jan 21, 2025

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:

  • BlockStateResolvers: 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 things
  • On-load/on-load-block modifiers: on-load modifiers are registered globally and allow wrapping unbaked models and unbaked blockstate models respecitively as they are loaded and therefor affect all further uses of the model in question
  • Pre-bake/pre-bake-block modifiers: pre-bake modifiers are registered globally and allow wrapping unbaked models and unbaked blockstate models respecitively right before baking and therefor only affect the specific baking process following their execution
  • Post-bake/Post-bake-block modifiers: post-bake modifiers are registered globally and allow wrapping baked models right after baking (effectively equivalent to wrapping models in ModelEvent.ModifyBakingResult)

Apart from the missing documentation, there are still a few questions open:

  • BlockStateResolvers provide access to the UnbakedBlockStateModels 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?
  • Should we try to aggressively re-use context objects?
  • What would be the preferred way of exposing the plugins to mods implementing on-demand loading/baking?

@XFactHD XFactHD added enhancement New (or improvement to existing) feature or request rendering Related to rendering 1.21.4 Targeted at Minecraft 1.21.4 labels Jan 21, 2025
@neoforged-pr-publishing
Copy link

neoforged-pr-publishing bot commented Jan 21, 2025

  • Publish PR to GitHub Packages

Last commit published: 1530b5bfd43eb5e5b74a74aaa2b85dcb809bd240.

PR Publishing

The artifacts published by this PR:

Repository Declaration

In 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 installation

In order to setup a MDK using the latest PR version, run the following commands in a terminal.
The script works on both *nix and Windows as long as you have the JDK bin folder on the path.
The script will clone the MDK in a folder named NeoForge-pr1884.
On Powershell you will need to remove the -L flag from the curl invocation.

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.

@XFactHD XFactHD force-pushed the model_loading_plugins branch from 3c4627c to 4b1c988 Compare January 21, 2025 03:13
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<>();
Copy link
Member

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.

Copy link
Member Author

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
Copy link
Member

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

Copy link
Member Author

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
Copy link
Member

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

Copy link
Member Author

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.

@XFactHD XFactHD force-pushed the model_loading_plugins branch from 4b1c988 to 6135c07 Compare January 21, 2025 21:38
@XFactHD XFactHD requested a review from embeddedt January 21, 2025 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.21.4 Targeted at Minecraft 1.21.4 enhancement New (or improvement to existing) feature or request rendering Related to rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants