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

Reintroduce BeforeBake* and AfterBake* model modifiers #4320

Merged

Conversation

PepperCode1
Copy link
Member

This pull request effectively reverts 2424258 and adds some minor improvements, including a fix for #4310 and documentation improvements.

The decision to reintroduce these events was made due to the following reasons.

  • Wrapping unbaked models could lead to duplicate baked model wrapping. An unbaked model with no elements and a non-null parent delegates baking to the parent, so if both the child model and parent model are wrapped, the baked model produced by the parent will be double wrapped, even though that's usually not preferable. This is not too much of an issue for simple wrappers in an isolated environment, but in the general case with multiple mods doing arbitrary wrapping, correctly handling duplicate wrapping becomes complex or impossible.
  • Mods may need to wrap certain models based on conditions that cannot be computed as early as when the OnLoad* events run. While such mods can still use the OnLoad* events to accomplish their goal, it requires wrapping the unbaked model unconditionally and doing the real wrapping within UnbakedModel#bake or GroupableModel#bake. This pollutes the vanilla code path with model wrappers even when it's not necessary and can increase the amount of allocations.
    Additionally, such mods cannot benefit from two of the three main reasons previously used as the basis for the removal of the *Bake* events (those being the one related to model resolution and the one related to block model groups).
    One specific mod for this reason is Continuity, which only wraps models based on which sprites are present in the atlas. Model loading and atlas loading/stitching happen concurrently, but model baking happens after both.

@PepperCode1 PepperCode1 added enhancement New feature or request priority:medium Medium priority PRs that should get reviews labels Dec 22, 2024
@modmuss50 modmuss50 added last call If you care, make yourself heard right away! merge me please Pull requests that are ready to merge labels Dec 22, 2024
@modmuss50 modmuss50 merged commit 8ca2ae8 into FabricMC:1.21.4 Dec 24, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request last call If you care, make yourself heard right away! merge me please Pull requests that are ready to merge priority:medium Medium priority PRs that should get reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ModelLoadingEventDispatcher.BlockStateResolverContext#setModel() does not null-check the given blockstate
3 participants