Notes:

• I do not think BakedModel injection is warranted. It's a cheap hack and the few mods which actually need it can just get away with a mixin.
• How to handle blockstate-esque parsing (aka model redirection) in CustomModelLoader? Should a ModelRedirector interface be added, or perhaps a way to have CustomModelLoader load multiple models? Both seem to have drawbacks...

TODO:

• Add a JsonCustomModelLoader which allows using a JsonObject and checks a "type" string key automatically. This will, then, have an optimized lookup mechanism in the CustomModelLoader searching code - only one JSON read will be made for all JsonCustomModelLoaders.

Also, I don't see the problem with blockstate parsing... I'm doing a similar thing in Architect, with different states per model file. I just parse the model resource location the loader gets into a list of properties and select which parts of the model and which changes on them I want based on that. Not sure how the vanilla model parser works, though, and it most definitely works completely differently from mine.

@therealfarfetchd What I'm saying is: What if you want to implement your own blockstate-esque format, but let other mods do the loading? Say, you add a custom blockstate format which lets you join transformations, but you want someone to be able to use Architect's model format for it. Is this even a concern?

Ah, I get what you mean. So you want to be able to load a different model from the CustomModelLoader? Yeah, that might be a thing that needs to be considered.

Maybe pass a Function<ModelIdentifier, UnbakedModel> to the load method, that when called, tries to load a different model (of course, stuff like recursion detection would need to be implemented, that it doesn't call itself with the same ID twice per "top level" load)

Mojang already implements recursion detection, so that's a non-issue. I'm just thinking if that's the best way to do it - perhaps I need to sleep on it.

One thing I'd request is decent support for extending other model formats. The current system would allow that, but I'd have to add an affix to the identifier for the extended version, otherwise it would recurse infinitely or until recursion safeguards kicked in.

That's a bit messy, and I think the situation could be solved fairly simply by adding a way to ask for a model to be loaded, ignoring the "active" loader. Perhaps as a default or static method of CustomModelLoader. Some kind of collection of ignored loaders for the current model being loaded would have to exist though, as loaders farther down the loading chain might want to use the same feature.

I can make a mockup of this if that would explain it better.

@Daomephsta I see what you mean; but I think that's a bit of a hack, really! The better approach would likely be to simply extend another mod's model as a separate file...

@asiekierka
How exactly would that work? The custom model loaders don't, and shouldn't, have any concept of inheritance.

@Daomephsta Except vanilla's model loader already has a model of inheritance. This is nothing new.

One of Player's proposals is replacing accepts+load with a @nullable load instead, as "it's a potential source of problems" otherwise + presumably as it could save a little bit of performance at scale (think in a "not having a duplicate contains/get" kind of way, except with resource resolution).

Another proposal I received is only checking trailing loaders optionally/in development, but the problem here is that issues caused by duplicate loaders are often non-obvious (rendering glitches, etc.) and hard to narrow down just by looking at a mod list, so I'm not keen on that.

Player also argues ModelRequester requires further thought, so if you have any ideas let me know!

This looks OK for model loading. Unfortunately there is no way to handle truly dynamic, run-time model baking in 1.14 and even if Fabric were going to provide that, this is not where it belongs.

Mojang still uses an IdentifyHashMap with BlockState values as keys (the map lives in BlockModels) and at end of baking, every possible block state that may be rendered (without a BlockEntityRenderer) needs to be included in that hash map. And it appears there is no longer any concept of "ActualState" distinct from the BlockState used for the model lookup.

This is a completely rational design decision by Mojang - it ensures all model loading and baking occurs before the main render loop starts. Model loading/baking implementations typically do a lot of memory allocation - which could cause stuttering from garbage collection if it happened at render time. And immutable, prebuilt models and a fast, pre-populated identity hash map avoid complications from concurrent requests for the same model.

I have models with over 2^64 possible states and specialized data structures and handlers to minimize these problems, but I recognize that's not the norm. Fortunately a simple MixIn for BlockRenderManager will probably be sufficient. That could be a PR but I'm a little reluctant to encourage dynamic model generation/baking because of the aforementioned pitfalls.

I'd say "dynamic" model generation/baking belongs outside of API, yes.

BakedModel doesn't actually have to be baked - it can create a different model based on the passed state in the getQuads method and cache that. Unless they removed that parameter, but I think it's still there.

It's still there, yes, and it's not going anywhere.

But I've always seen passing "extended" states as a bit of a hack. Maybe we can just get away with getting a fourth parameter (up until the second getQuads() happens we still have a View/Pos pair) + an AdvancedBakedModel itf? That could probably fit in Fabric itself. I'd particularly like @sfPlayer1 to share his thoughts regarding dynamically providing static data.

Yes, BakedModel can be implemented to generate quads dynamically based on the BlockState parameter passed to getQuads.

However, because that BlockState is also the key used to find the model in an IdentityHashMap, why bother? If you have a number of variants that can reasonably fit within the hash map, you may as well pre-bake all of them.

And if you have too many block states to fit in the hash map, then you will never see those block states as a parameter to getQuads.

Most of the blocks that need this (multi-part, conduit covers, Chisels and Bits) are going to be BlockEntities anyway, so they can (and probably do already) implement a BlockEntityRenderer.

But that is a lost opportunity, performance-wise, because the blocks usually aren't actually dynamic in the world and could be built and rendered as part of the chunk. Rendering from a pre-packed VBO is always going to be faster than uploading the quads each frame.

Yes, what I'm saying is: what way should we look into for rendering into a pre-packed VBO, but with arbitrary data?

I've implemented exactly that in 1.12, with both the vanilla pipeline and a custom pipeline.

In the vanilla pipeline getQuads relies on ExtendedState to return arbitrarily customizable models that are lit and packed in the normal manner.

The custom pipeline uses a custom vertex formats and shaders that allows multiple layers of texture/color/emissive to be sent as a single quad without transparency sorting or potential Z-fighting.

That code is available for anyone who wants it but it's more complex than what belongs in Fabric.

Problem is there's currently no hook to handle the dynamic model dispatch based on extended state information.

edit: typo

Extended state information isn't even a vanilla concept, however.

Also, I agree ExtendedState is a hack. And in 1.13+ it seems better to put as many properties a possible into BlockState itself, with the hope of avoiding the need for BlockEntities.

Assuming the new world format can handle arbitrarily many block states (I haven't worked with it enough yet to know) then, theoretically, we would simply need a way to circumvent the IdentityHashmap in BlockModels and instead route the lookup to a model dispatch mechanism that doesn't rely on a fixed number of block states.

Putting all the variation into BlockState directly (assuming again that the chunk format can handle it) would also prevent the performance-killing neighbor state or BlockEntity NBT lookups that are otherwise needed to determine the full extended state.

it seems better to put as many properties a possible into BlockState itself

NO! The BlockStates themselves still take up space in memory - as you need to store transitions from one BlockState to another, which is at least a large array + all state combinations possible.

Ah, that is good to know. Disappointing but not surprising.

I suppose we could extend BlockState to handle additional attributes that aren't defined as properties and leave the serialization implications up to mod authors. But those will obviously not work with the identity-based lookup as-is. I also don't know if there are other situations where an == comparison needs to work for BlockStates.

Perhaps we do need that fourth parameter with additional attributes.

@grondag Honestly, the main problem here is that looking up and adding all the information to an "extended state" object is costly as-is, I think we'd rather pass something as direct as possible - but we have to watch out for thread safety...

Also agree. Creating fewer objects is better. At the same time immutability is attractive for thread safety.

And we know for a fact that any additional state falls into one of two cases:

• (a) based on neighbouring blocks - which is usually manageable enough to put into a BlockState,
• (b) based on a BlockEntity.

Why not, then, make the "fourth parameter" simply acquired from a BlockEntity instance?

I have one case (dynamic terrain) where the number of additional states is on the order of 2^54 and is based on neighboring blocks. Currently these are implemented without BlockEntities (unless the terrain is "frozen".)

So I'd prefer to have world and position references and then look up BlockEntity if it applies.

Adding a FabricBakedModel type which has an extra getQuads method that accepts a nullable block view + position would solve the vast majority of problems that extended block states are used for in Forge.

All of the usages of extended block states I can think of off the top of my head are used to retrieve data from block entities. This is the approach I take for my multipart mod, a custom block state class and a mixin to pass the "rendering state" into getQuads while still using the normal state for BakedModel lookup.

Having a block view and position would solve this without the need for a mixin or a custom block state.

But that's an issue separate from model loading.

Edit re neighbor block data: that can either be stored in block state properties or in a block entity.

Yes, I think it'd be wise to open a separate issue.

To add to this point: the HashMap lookup may be useless when it routes to your dispatcher, but what if we have multiple possible dispatchers?

Well, that would be strange, right?

I suppose we could add an interface to Block which returns a DynamicModelDispatcher and circumvent the whole system that way (not to mention - automatically filter them out from blockstate processing in the model API), but we have to be careful with sidedness here!

This is more or less what I was imagining. Though still need a way to declare textures for inclusion in texture map.

After more thought, it probably is best to keep the first approach, and simply have the model loaders provide unbaked models that implement the necessary interface to trigger dynamic handling. I say this for three reasons:

1. We don't need a separate mechanism/event for getting textures into the texture map.

2. Reduces special handling elsewhere. The dynamic hook just has to inspect the type of model it gets.

3. Pack makers could, theoretically, substitute a dynamic model for a static model.

To illustrate 3, suppose I made a dynamic stone or sand model that implements flowing terrain and multi-block textures. I would need to give the model an "loader" of some kind, but a pack maker could choose to use my dynamic model simply by changing some JSON files.

As much as I dislike the waste, an IdentityHashMap lookup is always going to be fast...

Check the issue I've linked this in for a superior solution to the IdentityHashMap woes which also doesn't require us to do anything related in Fabric - it lets you remove the lookup independently, and it lets us use the cleanest solution which is the first.

Just one more note.

We don't need a separate mechanism/event for getting textures into the texture map.

We do, actually, have an event for this. There are cases in which you're rendering something which is so out of the ordinary that a model simply doesn't exist for it, yet you still want to benefit from the sprite atlas. Not to mention, there are cases when you want to, say, provide a custom Sprite object with a different loading mechanism.

So, well, we already have that.

@asiekierka I need to step away for a while, just want to say thank you for the excellent work on this PR and the other issue. Shaping up really well.

SO, GETTING BACK ON TOPIC

The only real blockers for this PR as-is are "merging accepts() and load()" and "figuring out ModelRequester", right?

In particular, does anyone have any alternate ideas for ModelRequester? Because I still think it's fine as-is.

Currently ModelRequester will not work with any non-Block objects.

When ModelLoader::loadModel is called with a given id, it first looks for a variant map (from a blockstate definition JSON). But it doesn't just parse each of the variants provided in the state definition into their own ModelIdentifier+UnbakedModel pairs. Instead, it gets the Block with the same id and uses that block's state factory to construct a list of states it should load from each given state definition.

The issue appears to stem from ModelLoader allowing there to be multiple blockstate definitions, as long as each of them provides disjoint sets of blockstates.

The only solution I've though of so far is to tear out all of loadModel and replace it with a Fabric implementation that's independent of block/blockstates and instead just generates ModelIdentifier -> UnbakedModel entries for every variant that's present in the blockstate definitions. This might require removing the ability for blockstate definitions to be split across multiple resource packs (which, as it's removing a vanilla feature, would be sub-optimal).

This solution is admittedly very invasive, so I'd welcome suggestions for any other potential fixes for this problem.

I didn't fully read the community feedback in the PR yet, so I am sorry if I reiterate something, but I want to get my own ideas out before having to head out:

I don't like how ModelLoaderRegistry takes supplier/function instead of the actual model requester/provider. ResourceManager should probably be a load() parameter instead, if needed at all (static access through MC). Edit: If supplier/function are for saving memory, a setup event for (re)triggering the registrations and throwing them away after finishing is probably better. I don't think it'd save much though, assuming reasonable implementations.

The purpose of ModelRequester.receive or LoaderInstance.finish is still unclear to me. If ModelRequester is just meant to tell the model loader what it is needs to (pre)load, why does it need bakedModelGetter? I could understand the model provider's load() receiving it, but not the requester. Why is it being called in finish? What are you supposed to implement in receive()? Note that this also implies some global "all done"/final event. If this is hinting at vanilla's block state or item to baked model repositories, I don't think that's an approach that should be promoted. MC has a global model id -> baked model repo that can be directly queried at runtime to build those caches. Post bake events can be used likewise.

Does vanilla not have some way to do what CustomModelLoaderException does?

I'd be very careful with adding anything to Block/Item as it was suggested in some comments.

Things probably outside this PR's scope, but partially related:

For carrying extra state: getQuads with another java.lang.Object parameter may work well, Forge's approach is overly complicated and sees a lot of abuse. The extra state should - if possible - be obtained only once for each block, before visiting the various sides or layers. It's likely most adequate to query the model instance for the state using block state+world+pos. This extra state shouldn't affect model lookup or anything else, it merely gets passed to all getQuads() calls. The semantics/creation/comsumption of the "Object" are completely internal to the baked model implementation. It nests nicely for nested baked models by putting the parent model's extra state in a field of itself.

Translating from block/item to model id should be possible to bypass similar to the model id -> unbaked model processing, making it easy/safe to avoid or override block state JSONs.

Specific applications like CTM's texture edits and glow may need more than load(). Sometimes you just want to edit a baked model, potentially by wrapping, without having to wrap the unbaked model handling or colliding with an existing load() handler. The same applies to requesters, they may want to be more reactive as it can be hard to determine what models ids will be present/processed. Both cases should be doable with per-model post-prepare and post-bake events, post-prepare has to happen before stitching.

Edit: If supplier/function are for saving memory, a setup event for (re)triggering the registrations and throwing them away after finishing is probably better. I don't think it'd save much though, assuming reasonable implementations.

It's to allow loaders to cache during the loading process. Every time ModelLoader is re-instantiated, you can pretty much throw all assumptions about the resource pack contents out the window - making it a supplier makes it easy to implement caching without worrying about implementing cache invalidation.

MC has a global model id -> baked model repo that can be directly queried at runtime to build those caches. Post bake events can be used likewise.

The idea here is that each ModelRequester only accesses its own area of interest; so that modders don't "cheat", make a dummy ModelRequester and then read all of the baked models out. However, it is true that all of the baked models are stored regardless... It can be dropped, then, and replaced with a post-model-loading event.

I have pushed some tweaks to the API. Of note:

• ModelProvider is the new "loader"; it also has a way to load parent models (vanilla does gracefully handle circular references),
• ModelAppender is a much simplified take on the old ModelRequester,
• In production (so no development and no debug flag enabled), only the first UnbakedModel is used,
• An optimized loading mechanism is used with either approach for custom loaders (though the JsonModelProvider optimization is not present yet).

From chat:

tl;dr: it's not usable for non-Block objects
asieToday at 3:44 PM
But... it is?
> When ModelLoader::loadModel is called with a given id, it first looks for a variant map (from a blockstate definition JSON).
No. It first calls the ModelProviders.
And ModelProviders now have a way to call loadModel back, with a different Identifier.
So you can circumvent the whole logic.
Though it should be documented that ModelIdentifiers are for resolving models and Identifiers are for loading model files...
Or even the ModelProvider split into two based on this dichotomy. Feel free to write up some thoughts on that if you have any.```

Rough proposal:

• ModelResolvers take in a ModelIdentifier and return an Identifier or UnbakedModel.
• ModelProviders take in an Identifier and return an UnbakedModel.

? (Not sure if the split is needed or if we shouldn't just document it. You can already implement all of this as ModelProviders can now call loadModel recursively - Mojang takes care of detecting circular references)

I like the simplification of ModelAppender. Use cases are making more sense to me now.