Dynamic Resources breaks Additional Placements' model baking

[UnusedCactus]

Bug Description

When Additional Placements attempts to bake models for most stairs variants and vertical slabs (as in only some blocks have this issue, not that some of the variants always bake), the model baker fails to function.
(More info here)

Reproduction Steps

1. Install Additional Placements and ModernFix
2. Enable ModernFix's Dynamic resources and restart
3. Load into a world and place a slab or stair vertically
   (You can also just load into a world with already placed vertical slabs/stairs and it'll work)

Log File

latest.log

debug.log

[FirEmerald]

as a potential fix, is there an api/event I could use to fix this on my end? something like "override model loading after attempting to load model for my block fails, load model for parent block(state), then construct mine" (I would prefer to do it after fail only so that if a resourcepack does provide a model, it gets used instead)

[embeddedt]

@FirEmerald The issue is almost certainly caused by your mixins to BlockStateModelLoader/ModelBakery. The primarily supported way for mods to work with dynamic resources is to use the standard NeoForge APIs (e.g. the ModifyBakingResult event or a custom NeoForge model loader). Those should work properly with both vanilla & dynamic resources.

I make a best effort to run through vanilla code paths where feasible to try to support some mixins, but it's just a best effort and not guaranteed to be stable. It's become harder to do that since 1.20, and looks pretty much completely impossible in 1.21.5+, although I haven't ported there yet.

[FirEmerald]

@FirEmerald The issue is almost certainly caused by your mixins to BlockStateModelLoader/ModelBakery. The primarily supported way for mods to work with dynamic resources is to use the standard NeoForge APIs (e.g. the ModifyBakingResult event or a custom NeoForge model loader). Those should work properly with both vanilla & dynamic resources.

See, that is what I used to do, but not even all vanilla calls to blockmodel rendering code let you get either the blockstate directly, or through the modeldata extension, leading to cases where you would have "missing texture" or even "missing model" instead of the correctly wrapped model. Not to mention that fabric does not have such a system. So I found the next best thing I could do, with the least amount of mixins.
Anyways, like I said earlier, I would be more than happy to support ModernFix by utilizing a hook to provide a custom unbaked or baked model as a substitute to missing model when the dynamic resource loader can't get one.

[FirEmerald]

I did also once consider having the models be dynamically generated through an in-code resourcepack, getting their "modeled state" models via a cached blockstate, but decided against that as this would mean having to bake a unique block model for every single possible blockstate that my mod adds, which would have a non-trivial RAM impact.

[embeddedt]

Not even all vanilla calls to blockmodel rendering code let you get either the blockstate directly, or through the modeldata extension, leading to cases where you would have "missing texture" or even "missing model" instead of the correctly wrapped model.

Can you explain further? From what I can tell your mixin is essentially injecting an unbaked model for certain blockstates (when a model is missing, and the original block is an AdditionalPlacementBlock). I'm not sure why you wouldn't be able to accomplish something similar by injecting the models in ModifyBakingResult for the specific ModelResourceLocations that relate to your blockstates. E.g. (untested)

public void modifyModels(ModelEvent.ModifyBakingResultEvent event) {
        for (Block block : BuiltInRegistries.BLOCK) {
            if (block instanceof AdditionalPlacementsBlock) {
                for (BlockState state : block.getStateDefinition().getPossibleStates()) {
                    var mrl = BlockModelShaper.stateToModelLocation(state);
                    event.getModels().put(mrl, <replacement model>);
                }
            }
        }
}

You can obtain unbaked models (if needed) from the ModelBakery provided in the event.

On Fabric, there is a model loading API that lets you swap at the baked or unbaked model level: https://github.com/FabricMC/fabric/blob/dafaca052d58e888e64fcd47fc83a265048bf285/fabric-model-loading-api-v1/src/client/java/net/fabricmc/fabric/api/client/model/loading/v1/ModelLoadingPlugin.java#L79-L96

[FirEmerald]

I'm not sure, I'm fairly positive I tried that at one point. I do know one thing I have is that I absolutely do NOT want to override models that were actually loaded with the normal model loading system.

[FirEmerald]

I've overhauled the system many times and it's hard to keep track of what I have and haven't used.

[embeddedt]

Ok, I ran your code locally to understand how it works. I think this logic may not function as intended in vanilla all the time either. You rely on ModelBakery.topLevelModels being populated with the prototype unbaked model you use as a reference for creating yours. The issue is that topLevelModels is indirectly populated by the very same function your injector is in. So, unless I missed something you essentially rely on the prototype block happening to be loaded before yours. I think this will be true for vanilla blocks because of registry order. I don't think that guarantee extends to modded blocks.

In ModernFix, this will never work reliably because topLevelModels never holds all models at once, and does not dynamically load models when get is called on it. (It's a private field in ModelBakery, not the canonical baked model registry, so in general there's no need to provide that behavior for mods.)

I came up with the following event handler, which seemed to work in my limited tests.

Event handler code

	@SubscribeEvent
	public static void onModelBakeModify(ModelEvent.ModifyBakingResult event) {
		var models = event.getModels();
		var missingSprite = event.getTextureGetter().apply(new Material(TextureAtlas.LOCATION_BLOCKS, MissingTextureAtlasSprite.getLocation()));
		ModelBakery.TextureGetter textureGetter = (loc, m) -> event.getTextureGetter().apply(m);
		for (Block b : BuiltInRegistries.BLOCK) {
			if (b instanceof AdditionalPlacementBlock<?> block) {
				for (BlockState ourState : block.getStateDefinition().getPossibleStates()) {
					var ourLocation = BlockModelShaper.stateToModelLocation(ourState);
					var existingModel = models.get(ourLocation);
					try {
						// Do not replace when a model exists for that state
						if (existingModel != null && existingModel.getParticleIcon(ModelData.EMPTY) != missingSprite) {
							continue;
						}
					} catch (Exception e) {
						// Definitely don't replace when the model behaves weirdly
						continue;
					}
					// No existing model, try to inject ours
					BlockState theirState = block.getModelState(ourState);
					BakedModel theirModel = models.get(BlockModelShaper.stateToModelLocation(theirState));
					BakedModel replacementModel;
					if (block.rotatesModel(ourState)) {
						BlockRotation theirModelRotation = block.getRotation(ourState);
						replacementModel = BakedRotatedPlacementModel.of(theirModel, theirModelRotation, block.rotatesTexture(ourState));
					} else {
						StateModelDefinition modelDefinition = block.getModelDefinition(ourState);
						ResourceLocation ourModel = modelDefinition.location(block.getBaseModelPrefix());
						ModelState ourModelRotation = PlacementModelState.by(modelDefinition.xRotation(), modelDefinition.yRotation());
						var baker = event.getModelBakery().new ModelBakerImpl(textureGetter, ourLocation);
						replacementModel = BakedRetexturedPlacementModel.of(BakingCache.bake(baker.getModel(ourModel), ourModelRotation, event.getTextureGetter(), baker), theirModel);
					}
					models.put(ourLocation, replacementModel);
				}
			}
		}
	}

It might be useful as a starting point. Note that it has some rough edges - for example the vanilla model loader now spams log messages about missing variants for the Additional Placement blocks. You'd likely need a mixin to suppress the log message.

If it's absolutely not possible to do this with the event then I can look at adding a custom hook, but I really try to avoid that nowadays where possible since it's a maintenance burden and the event is generally the better option than the equivalent vanilla mixin.

[FirEmerald]

It's entirely possible I never used the event, since the code has been carried forward from so many versions, but it seemed familiar. I can certainly mixin to disable the missing model messages, as I know I have to do that on some other versions. It still doesn't solve the fabric issue; that wrapped hook has existed for a while, but I think the reason it didn't work for me was either that it didn't fire for missing models or it doesn't let me ensure my hook runs before other mods (note: continuity) that can end up covering up the missing model? I'll have to spend more time looking at this stuff.

[FirEmerald]

as for the hook, if it ends up being necessary, a simple "missing model" event with a callback that can supply another loaded model would work fine. Honestly I wish Forge, NeoForge, and Fabric just had something like that, as it would completely negate the need for me to use mixins for this entirely

[embeddedt]

Just brainstorming, but another idea, if detecting missing models directly through the API is difficult, is to call ModelBakery.BLOCKSTATE_LISTER.listMatchingResourceStacks(resourceManager) once yourself (this is reasonably fast) and then just build a set of which blockstate definition files pertaining to your blocks are not present. This should be reliable since resourcepacks have to provide this file for the vanilla model system to load their model.

[FirEmerald]

That would work. Btw here's what I meant in terms of an event, in Forge/Neoforge style

public class MissingBlockModelEvent extends Event {
    BlockState blockState; //the blockstate for which a model is missing
    BakedModel model; //would be whatever the model ends up being, by default a missing model model

    //omitting field getters/setters
    
    public BakedModel getModel(BlockState blockState); //method to attempt to grab the model for another blockstate
}

[FirEmerald]

Also, the reason why detecting missing models directly is hard is because, for some reason, iirc minecraft doesn't have a "global" missing block model; it creates a baked model using a cube with missingtex, and only the unbaked model actually has a field you could even attempt to use. I am sure that the unbaked model is linked to the missing model in a cache somewhere.
Honestly I hate how mojang has implemented the blockmodel system, it's such a mess.

[embeddedt]

IMO, it's even worse (from the perspective of altering its behavior) in 1.21.4 & 1.21.5, lol.

[FirEmerald]

hey, it's better than when they were just straight up loading every single block and item model from every available resource pack regardless of if it was going to be used or not