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

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)

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

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.

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

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.

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

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.

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

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.

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

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.

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
}

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.

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

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

and don't even get me started on how blockstates work; they maintain an instance of every single possible blockstate when it would be trivial to instead have a blockstate be a block-int pair that acts as "flags" and doesn't need to have all possible states in memory at all times. The only big changes that would require would be having block properties no longer be static (since they would be responsible for identifying the "location" of flags in the int) and that blockstates would be mutable

EDIT: dunno why I didn't think of it sooner but an Object array would be better than an int, lol. each property would have an index in that array. For sending to clients you could definitely serialize it to an int though.

an Object array would be better than an int

Actually, the int representation is substantially more compact and also faster to test for equality. What you described is basically what FerriteCore does under the hood (except for removing the global registry of state objects).

well it would be faster for that, but I'm thinking of how generally, when working with blockstates, you are interested in the properties themselves, and indexing an object array would be faster than the int compression, which would require an int division, int modulo, and then an array lookup (or a method call that leads into some conversion method that could be an array lookup, a boolean test, or a range conversion)

regardless, you don't get much performance boost outside of smaller RAM usage without converting blockstate properties from static fields to instance fields, since you'd still need some sort of mapping to get from property to index/location.

The bigger takeaway is that so much of minecraft's code is actually garbage and should have been redone YEARS ago.

Yeah, so I checked, and at least in 1.16.5 every single missing model is baked into a unique block model. Which is not only incredibly annoying but also incredibly inefficient, and I sure hope newer versions don't do that.

[09:15:48] [Render thread/INFO] [Additional Placements/]: additionalplacements:minecraft.heavy_weighted_pressure_plate#ap_placing=east,power=14: net.minecraft.client.renderer.model.SimpleBakedModel@21a87972
[09:15:48] [Render thread/INFO] [Additional Placements/]: additionalplacements:minecraft.heavy_weighted_pressure_plate#ap_placing=east,power=15: net.minecraft.client.renderer.model.SimpleBakedModel@1d27d981

Is there a way to reliably determine if a baked model is the missing block model? beyond just checking if the particle texture is missing texture as that would false positive manually provided models with a missing particle texture.

I suppose I could cache the missing model blockstates in the same mixin that would supress the errors and just work off that.

The problem is that the error code likely won't fire when dynamic resources is enabled, or at least won't fire for all models early enough for your event to work off of.

Yeah, I just thought of that too. Their really just doesn't seem to be a better way to do this. Either I need to special case ModernFix, not allow custom block models with missingtex particle sprites, or not allow partial blockstate json (for instance, to only define models for the stair shapes that don't have a vanilla counterpart). All because vanilla model loading is completely and totally fecked.

I do think adding an event for model loading would be the most beneficial solution to special casing modernfix. I don't think it would actually be that difficult to maintain, and would let me implement this fairly effortlessly. Especially since the event would only need to fire on missing models. Like I said before, I kinda wish forge, neoforge, and fabric already had an event for this, but since I might be the only person who would ever use it, I understand why they don't.

I might try working backwards from 1.21.5 instead of forwards from 1.16.5 for this actually. I think newer versions don't generate missing block models until called for, which would allow me to easily identify the missing models to generate from.
Though I have to ask, how does the dynamic resource loading work with the modify baking result event? Does that event fire every time you load a model? does it load before? if so, do any models added during that event prevent an attempt to load the model through the normal model loading process?

Though I have to ask, how does the dynamic resource loading work with the modify baking result event?

The event is fired once at the end of the resource reload (same as vanilla) and the Map<ModelResourceLocation, BakedModel> is faked. The implementation does its best to behave indistinguishably to the real map used when dynamic resources is off.

• All ModelResourceLocation keys for registered blocks, items, and extra ("standalone") models are presented in the keySet, even though they're not yet loaded.
• Models are baked lazily when values are retrieved from the map and cached - they can be unloaded later if they haven't been referenced in a while.
• If a model is inserted into the map, either with a new key or by replacing an existing value, the replacement is remembered permanently and is not unloaded.

There are tricks employed to support things like replaceAll performantly, but that's not used by most mods (Mekanism is the most prominent example I can think of). The rest of the Map APIs are quite conducive to lazy loading, e.g. when iterating the entrySet, I don't actually have to load the model unless getValue is called on the entry. This is important as a lot of mods have the habit of iterating over every entry in the map and wrapping/replacing based on what they see in the key. I cannot afford to let them load every mode in that event or it nullifies the improvement to game launch time. 😛

There's quite some complexity in that code but at least it's fully on my end and I don't really have to deal with Mojang/Forge changes much (the event design has stayed the same for years, it's only finally getting replaced soon-ish with neoforged/NeoForge#1884). Much easier to maintain than having to rewrite the patches to the model bakery every 6 months. It's one of the reasons why in 1.21.4 I gave up and decided it's easier to reimplement all the model loading logic in my own class rather than trying to hack vanilla to do it lazily.

That turned into a bit of an information dump but hopefully it's helpful.

if so, do any models added during that event prevent an attempt to load the model through the normal model loading process?

If you put an entry into the map for a given ModelResourceLocation, then yes, it will skip the normal model loading process - if you want to trigger that you have to obtain the original baked model from the map first.

you have to obtain the original baked model from the map first.

That would be fine then, you'd lose the benefits of dynamic resource loading for basically any blocks this mod adds (and their original counterparts), but otherwise it would still work.

I still stand by the suggestion of adding an event for this though, as it would allow users to continue to gain the dynamic resources benefits with your mod. It would be a special case in my mod but I think it would be worth doing.

Quick question - does calling the "containsKey" method load the model, or would I need to actually call "get" and then check if it's a missing model?

You have to call get with ModernFix - not sure about vanilla, it's probably the same since it will populate the map with missing models.

ah, I don't have to do that currently in 1.21.5 but I'll change that to account for this. IDK how it is in 1.20 but I know at least a few versions in 1.21 don't populate missing block models on loading - only when a call is made to try to get a block model that wasn't able to be loaded. Which is good, because it means you can have state property combinations that don't actually exist in world without having to add models for them or get missing model errors.