1.21.5+ Models

[spare bloom]

removed it

[warped sluice]

Can I have the first commit's diff?

[spare bloom]

https://gist.github.com/Technici4n/6e1c36387eac7e4d15c3df110e3627c3

[warped sluice]

Alright, I suppose this is fine with sufficient documentation. The null contract still seems a bit weird to me but I'll add it to Fabric.

In that diff I don't see a reason to include this model in the key for multipart and composite models

[spare bloom]

Yeah I think you're right

[warped sluice]

What is the exact contract of a null key?

I assume it means no key exists for the given parameters

[hardy talon]

Null means that the model doesn't support geometry keys and therefore has to be considered effectively uncacheable

[warped sluice]

It is possible for a model to return null only in some cases though

[hardy talon]

No, it either needs to always return non-null or always null (i.e. not implement it)

[warped sluice]

That's why the nullable annotation seems weird to me

[spare bloom]

That's not possible

In a MultiPartModel, some parts might implement the method and some might not

[last wave]

I have more of a conceptual question, with regards to the cache key. Can we cache RenderType implementations when on fabulous (aka using the entityXYZ(...) variant)?

As far as I can see these don't have a hashcode nor do they have an equals

And can be called on the fly to create new instances dynamically....

Which feels wrong somehow....

I specifically ask, because for Fluids in the past we did not have the RenderTypeGroup nonesense, requiring me to figure out how to get the correct rendertype for caching that particular layer of fluid bits when the game was running in fabulous mode.

[spare bloom]

I am not sure I see what you mean. For sure some sort of fluid caching does not exist rn

Well ok maybe I misunderstood

You are right that we need to do something about the fabulous config changing? Maybe?

Thinking of leaves right now...

[last wave]

Right now part of my cache key is a RenderType. Do cache the geometry that is needed for that specific rendertype layer.

[last wave]

That means that the RenderTypes hashcode is part of the cachekey

This works fine for Fancy rendering

As the RenderTypes are static instances and calling RenderType.translucent returns the exact same instance over and over again with the exact same hashcode in toe

However as soon as you switch to Fabulous

That does not hold anymore

Because for example RenderType.entityTranslucent(...) returns a new rendertype every call and in turn a new hashcode

Busting the cache

[elfin junco]

uh.. no it doesnt, RenderType.entityTranslucent() delegates to RenderType.ENTITY_TRANSLUCENT which is a memoized BiFunction returns the same instance for the same set of inputs
iirc almost all if not all of the render types are memoized like this

using a ConcurrentHashMap and Pair on the backend of the memoizing iirc

[last wave]

Ahhh

See that is what I missed

It is memoized

Derp me....

[hardy talon]

Fabulous mode no longer uses a different entity render type, that's why the rendertype group only has two instead of three types since 1.21.4. The fast/fancy switch on leaves is the only annoying case like that and I personally ignored that in the past and now handle it by nuking my caches on renderer reload if the graphics mode changes.
With that said, the rendertype is now part of the BlockModelPart, each part can only have one rendertype and the quad lookup is no longer rendertype-sensitive, so there's little value in caching per rendertype since all parts, regardless of rendertype, are collected at once.

[hardy talon]

Ugh, right, I completely forgot about that

[last wave]

Okey that makes a lot of sense, i have been mostly working on 21.1 in the last few days so I was looking at that code this morning, and just thinking about some usecases, while planning my port to 21.4

[hardy talon]

I'm currently playing around with custom blockstate models and datagen thereof and ran into two things:

• We desperately need a hook point that handles all client-side registries instead of duplicating the individual calls between normal runtime and datagen, ideally one mods can hook into as well. I just wasted 20 minutes wondering why I'm getting an "unknown element" error for the entire custom blockstate model codec only to find that custom blockstate model codecs are not registered in datagen.
• The blockstate a given unbaked blockstate model is being baked for is currently only available to BlockStateModel.UnbakedRoots but not BlockStateModel.Unbakeds. I think it would be nice to forward this info to the latter during baking

[spare bloom]

BlockStateModel.Unbaked right?

(not custom definitions, I mean)

[hardy talon]

Yes

[spare bloom]

I'm not sure it's a great idea given SimpleCachedUnbakedRoot

is it not practical to override asRoot?

[hardy talon]

That only works for some cases. If the custom model is part of a weighted list or a multipart, then asRoot() won't be called on the custom model

[spare bloom]

well otoh that is already deep inside blockstate-aware variant selection 🤔

[hardy talon]

Yeah, ok, nevermind, this will indeed not work because of the shared state caching in multipart models. I guess I'm going to have to live with only having a potentially untrustworthy blockstate in the model (yes, I know that I caused this exact thing myself)

[spare bloom]

I mean this is one more reason to have custom blockstate definitions

because then you completely take over the BlockState -> Model mapping

[hardy talon]

Yeah

[spare bloom]

why do you prefix your branches with port/1.21.5?

[hardy talon]

To be perfectly honest, I just copied what shartte was doing for his branches

Anyway, here we go: https://github.com/neoforged/NeoForge/pull/2052

[random pebble]

because of the workflow rule on port/

[spare bloom]

Yeah that's what I thought

Do you know why it's broken?

It should also skip JCC on PRs to the port/ branches

See discussion in #1330296017005842513

[random pebble]

it should yeah, haven't looked into it yet

[spare bloom]

you have a license violation

(can't have a look at the actual code now)

[hardy talon]

God dammit

[hardy talon]

Next stop: custom block model definitions: https://github.com/neoforged/NeoForge/pull/2053

[warped sluice]

Neo should patch vanilla models that delegate to submodels to also delegate the extended particle sprite method

[spare bloom]

Delegate to what?

Such a patch didn't exist in the past, even though the method has existed for a long time

[warped sluice]

Without it, a custom block model inside a variant or multipart won't have its extended method ever called

It's the same reason as patching to call the extended collectParts

[spare bloom]

Well perhaps. Vanilla only uses the first particle sprite anyway so I didn't think much about it

[warped sluice]

That's true

I added the overrides to Fabric so I'm just letting you know

[spare bloom]

Anyway @hardy talon I reviewed the datagen PR please have a look

[hardy talon]

Yup, I saw

[dry tapir]

is it possible to load obj models that only uses triangles now?

[stray fog]

I don't know if this has changed in 1.21.5

but it was definitely possible in 1.14-1.21.4

the loader just duplicates one of the vertices

to turn them into degenerate quads

[hardy talon]

I spent some time earlier updating my model loading plugins PR (https://github.com/neoforged/NeoForge/pull/1884). I've made it a draft again for now because it needs some discussion:

• Pre- and post-bake modifiers for UnbakedModels are dead because there is no longer a central point where they are baked
• Load modifiers for UnbakedModels seem to be fine but I'm not 100% sure about the injection point in ModelDiscovery
• Load, pre-bake and post-bake modifiers for ItemModels are trivial to implement but, for lack of a specific use-case, I'm having a hard time judging how useful they would be in practice due to ItemModels being basically completely opaque

[warped sluice]

You can see my Fabric port to make decisions

I can give permissions to relicense my work there

Also I plan to deprecate BlockStateResolvers for removal once I add custom block state definition types/codecs to Fabric. If you want to match this decision, then you can remove them from your PR since Neo already has custom definitions.

[hardy talon]

Yeah, that's a thought I also had

[hardy talon]

I'll have to dig into that tomorrow

[spare bloom]

Conceptually, it makes sense to wrap item models to me. But of course one doesn't usually need to do that

I'd focus only on the events for which a use case can clearly be found

[hardy talon]

The before/after bake modifiers are exceptionally trivial to add, so I'm gonna keep them. I will however remove the item on-load modifier because it's effectively equivalent to the before bake modifier since item definitions don't have any inheritance like models do

[warped sluice]

I did not add item on load either

[hardy talon]

Yeah, noticing that made me think about it again

[warped sluice]

You can apply the same argument to block on load, but remember that block models also assemble groups, so to modify those, the model needs to be modified on load

There is another argument for block on load that does also apply to item on load but I don't have a use case for it right now

[hardy talon]

That's fair, yeah

[random pebble]

Did we end up not making ModelData just a Object ?

[hardy talon]

Yes, intentionally

@warped sluice my PR should now basically match your Fabric PR with the only differences being that blockstate resolvers are gone from mine and item bake modifiers also provide the ClientItem as context

[warped sluice]

I suppose I could add those in as well

Did you have a use case in mind?

[hardy talon]

No specific use case, no, but having access to the RegistryContextSwapper and the ClientItem.Properties (assuming Mojang adds further values to that in the future) seemed useful to me

@spare bloom regarding this comment: https://github.com/neoforged/NeoForge/pull/2052#discussion_r2012445090: I tried implementing this and I would end up with this ugly contraption:

@Override
public CustomBlockStateModelBuilder with(VariantMutator variantMutator) {
    CompositeBlockStateModelBuilder builder = new CompositeBlockStateModelBuilder();
    for (BlockStateModel.Unbaked model : this.models) {
        if (model instanceof SingleVariant.Unbaked(Variant variant)) {
            builder.models.add(new SingleVariant.Unbaked(variantMutator.apply(variant)));
        } else if (model instanceof WeightedVariants.Unbaked(WeightedList<BlockStateModel.Unbaked> entries)) {
            builder.models.add(new WeightedVariants.Unbaked(entries.map(entry -> {
                if (entry instanceof SingleVariant.Unbaked(Variant variant)) {
                    return new SingleVariant.Unbaked(variantMutator.apply(variant));
                }
                return entry;
            })));
        } else {
            builder.models.add(model);
        }
    }
    return builder;
}

I think it might be better to just accept that CompositeBlockStateModelBuilder#with(VariantMutator) is a no-op and have people first construct the MultiVariant for the part model, apply all the relevant mutations to that directly and then plug the result of MultiVariant#toUnbaked() into the CompositeBlockStateModelBuilder

[spare bloom]

alright then

an alternative is to introduce some sort of intermediate representation

basically Either<Variant, CustomBlockStateModelBuilder>

not sure it's worth it though

(that's basically how the codecs already work, on the reading side 😄 )

[hardy talon]

Vanilla's intermediate representation of a BlockStateModel.Unbaked during datagen is MultiVariant but, at least currently, it doesn't have a common supertype which could be used.
Specifically the composite model also begs the question: would one even want to apply the exact same mutator to all parts?

[spare bloom]

I prefer to see it as follows: vanilla has a Variant for datagen and a Variant for model loading

similarly, MultiVariant is for datagen whereas Weighted<Variant> (then turned into WeightedVariants) is for model loading

of course we patch the model loading to be able to load more than variants, but this creates an asymmetry in this nice system

[hardy talon]

Within datagen MultiVariant represents both SingleVariant and WeightedVariants

[spare bloom]

ah yes my bad Weighted is still a single element

well in any case we can't realistically patch out Variant from datagen

[hardy talon]

Yeah, that's not gonna work

[spare bloom]

so I guess I agree with your last remark

i.e. just live with it 😛

[hardy talon]

👌

Regarding the other points:

• Yes, the way I implemented the composite builder is stupid, I unfucked that locally
• Javadoc is still pending
• Allowing a weighted list of custom models is possible, I added that locally
• A convience constructor for a weighted list of custom models is possible (or more specifically, a factory method because of generics)

[spare bloom]

sounds good

[random pebble]

Learing this will be... urgh

[random pebble]

Seriously that class structure is... what

Do we have a diagram of the stuff involved?

[hardy talon]

The structure of what specifically?

[random pebble]

The classes that BakedModel and UnbakedModel got turned into

From what I can tell so far, handling of custom model data has essentially moved "up" to blockstate models

I'll move to #modder-1․21․3＋-support ig

[random pebble]

Can we not add renderType to QuadCollection?

Wait nevermind. I missed that it isn't usable directly as a part

[random pebble]

Okay, could we not have put the render-type into ModelRenderProperties itself?

Doing this ceremony everytime I need to resolve a base model for a custom item model sucks:

ModelRenderProperties modelrenderproperties = ModelRenderProperties.fromResolvedModel(modelbaker, resolvedmodel, textureslots);
var renderTypeGroup = resolvedmodel.getTopAdditionalProperties().getOptional(
        net.neoforged.neoforge.client.model.NeoForgeModelProperties.RENDER_TYPE);
var renderType = renderTypeGroup == null ? null : renderTypeGroup.entity();

I guess I can write my own wrapper

IDK if we want to offer sth along those lines in the API itself:

public record ItemBaseModelWrapper(
        List<BakedQuad> quads,
        ModelRenderProperties renderProperties,
        @Nullable RenderType renderType
) {
    public static ItemBaseModelWrapper of(ModelBaker modelBaker, ResourceLocation id) {
        var baseModel = modelBaker.getModel(id);

        var baseModelTextures = baseModel.getTopTextureSlots();
        List<BakedQuad> baseModelQuads = baseModel.bakeTopGeometry(baseModelTextures, modelBaker, BlockModelRotation.X0_Y0).getAll();

        var modelRenderProperties = ModelRenderProperties.fromResolvedModel(modelBaker, baseModel, baseModelTextures);
        var renderTypeGroup = baseModel.getTopAdditionalProperties().getOptional(NeoForgeModelProperties.RENDER_TYPE);
        var renderType = renderTypeGroup == null ? null : renderTypeGroup.entity();

        return new ItemBaseModelWrapper(baseModelQuads, modelRenderProperties, renderType);
    }

    public void applyToLayer(ItemStackRenderState.LayerRenderState layer, ItemDisplayContext context) {
      renderProperties.applyToLayer(layer, context);
      layer.prepareQuadList().addAll(quads);
      if (renderType != null) {
          layer.setRenderType(renderType);
      }
    }
}

(Potentially with an overload that also allows customizing the modelstate)

[spare bloom]

Can't you use BlockModelWrapper directly, at that point?

[random pebble]

I need to customize the tinting for one of them, so no

(dynamically)

For the other one it might be an idea, ig

[random pebble]

@spare bloom As an example how I use this:

var baseLayer = renderState.newLayer();
var tint = baseLayer.prepareTintLayers(2);
tint[0] = -1;
tint[1] = ARGB.opaque(item.getColor(stack));
baseModel.applyToLayer(baseLayer, displayContext);

I don't think I can post-factum override the tinting based on itemstack if i use the default block model wrapper

[spare bloom]

any reason why you don't use a registered tint provider?

I mean ItemTintSource

[random pebble]

Hm, I guess I could 🤔

We'll see, lots of refactoring to do I guess

Ultimately little value to decomposing it into 100 little pieces though, to be honest.

StandaloneModelBaker needs an example, imho

It is also quite annoying that one cannot combine this with SimpleModelWrapper

Unless I missed some class or method, it seems like I need to copy paste the content of SimpleModelWrapper::bake, except for the first line to get one

[hardy talon]

https://github.com/neoforged/NeoForge/pull/2077 😄

[random pebble]

Ahah!

APPROVE! APPROVE! APPROVE!

You should consider making that two overloads

[hardy talon]

What would the second one be?

[random pebble]

To support:

event.register(MolecularAssemblerRenderer.LIGHTS_MODEL, SimpleModelWrapper::bake);

Which defaults to X0_Y0

Annoyingly the event StandaloneModelBaker does not follow the parameter order of SimpleModelWrapper.bake

[hardy talon]

Hmm. Wouldn't it be better to just provide some "default" bakers (i.e. for BlockModelPart and BlockStateModel) instead?

[random pebble]

If you want to transform the quads yourself, having access to QuadCollection::getAll is easier than querying it for each cull face from a BlockModelPart, imho

BlockModelPart doesn't get you anything for a BlockEntityRenderer if I am not mistaken

[hardy talon]

The part baker can be typed to SimpleModelWrapper if that's a concern

[random pebble]

You mean sth along those lines?

interface StandaloneModelBaker<T> {
...
  static StandaloneModelBaker<SimpleModelWrapper> simpleModelWrapper() {
    return (model, baker) -> SimpelModelWrapper.bake(baker, model, X0_Y0);
  }
...

...
  static StandaloneModelBaker<SimpleModelWrapper> simpleModelWrapper(ModelState modelState) {
    return (model, baker) -> SimpelModelWrapper.bake(baker, model, modelState);
  }
}

I get you'd want to call those blockPart, not sure I'd want that

[hardy talon]

I don't particularly care about the name. Though I would make the simple ones constant fields instead of factory methods

[random pebble]

Hm, they're statically registered so the memory allocation doesn't particularly matter, I think
Although I had initially thought about that too

So, sure, why not

[spare bloom]

non-capturing lambdas are cached inside the callsite

[hardy talon]

It's not about the allocation, it just seems kinda pointless to have a factory method return an effectively constant value. Just a matter of taste, I guess

[random pebble]

I mean look at Rendertype 😄

RenderType.solid() et al

Since we have at least one with a parameter, I guess I dont like mixing

[hardy talon]

That's fair, yeah

[random pebble]

Feel free to ping me when you need a review

[random pebble]

We patched in getStandaloneModel, right?
We should maybe consider adding getRequiredStandaloneModel that throws with a useful error message

Or, do we recommend not rendering at all if the models are borked

🤔

[rocky vector]

Not rendering at all is probably the safer option

[spare bloom]

models that fail to load should probably throw, no?

ah you mean later on when you try to actually render stuff?

(e.g. from BER)

[warped sluice]

Vanilla has always treated models that failed to load as missing models and returned the missing model

I don't see why standalone models shouldn't work the same way

[hardy talon]

They do work that way but only during baking and not when retrieving the baked model because at that layer the type is arbitrary. I guess, realistically, the only ways get a null model are to not register the StandaloneModelKey or someone sneaking an empty map of baked standalone models into the system.

[random pebble]

yup

Another interesting point will be how Athena will work in 1.21.5 hm

Both at runtime and datagen

[spare bloom]

Yeah then I say you just don't render it if it's null. Or maybe render a missing mofel

[hardy talon]

Almost certainly the same way it did previously since they were already hijacking the blockstate format

[random pebble]

probably even easier for them now

[spare bloom]

well it's a cross-platform mod so I wouldn't put it past them to not even use our system

[random pebble]

They can replicate our system on Fabric

Which they already kinda do with loader 😛

[hardy talon]

Yup. I did the conversion in both my own CT mod and in FramedBlocks and it works great

[random pebble]

Annoyingly for datagen, some vanilla methods return BlockModelDefinitionGenerator instead of the concrete type (MultiVariantGenerator)

it seemed (at a glance) easier to apply something like Athena to a MultiVariantGenerator than to a BlockModelDefinitionGenerator

Hm although i can probably just move it to the MultiVariant itself

[rocky vector]

if Athena bypasses our system it's likely I can't sanely make it work with dynamic model loading

[spare bloom]

maybe XFact should consider releasing his CTM mod one day 😄

[hardy talon]

As far as I'm aware, Pepper intends to add proper custom block model definitions to Fabric as well, so there shouldn't be much reason to bypass either loader's API

[hardy talon]

I might actually do that, still need to fix some stuff though

[random pebble]

I'll hijack this channel for my inane questions 😄

Why is there now a PropertyDispatch.initial and PropertyDispatch.modify

also god this pisses me off, I just rewrote all this model generator crap like 2 weeks ago 😄

Thank god for structural replace

[hardy talon]

Because MultiVariantGenerator.Empty needs a MultiVariant and MultiVariantGenerator needs a VariantMutator. That split is very

[random pebble]

god syntactically this is such a mess

plainVariant(ResourceLocation) gets you a MultiVariant, meaning the chaining is broken
but variant(ResourceLocation)to chain rotations on it doesn't exist. like what

[hardy talon]

If you want a Variant, then there is plainModel()

[random pebble]

Actually what they seem to intend is something else

once you got a MultiVariant, you can then apply the mutator-style

[hardy talon]

Mhm

[random pebble]

So this: plainVariant(standing).with(VariantMutator.X_ROT.withValue(Quadrant.R180))

Instead of new Variant(standing).withXRot(Quadrant.R180)

which you'd then have to turn into a MultiVariant

[hardy talon]

To be honest tough, regardless of how insane the property dispatch stuff is, it's IMO still saner than our old blockstate datagen. Like, in what world does it make sense that I have to opt unused properties out of the dispatch instead of opting-in the necessary properties???

[random pebble]

Yeah it is better

this new syntax is also better than the old but discoverability is obviously bad

This is not that bad:

[hardy talon]

Yup

[random pebble]

What's going to be interesting is me adding my custom SpinnableVariant 😄

I need x,y and z rotation

That being records all the way down is going to make that a real nightmare, I think 🤔

[hardy talon]

@random pebble I added the factory methods we talked about to the PR I linked earlier

[random pebble]

Lazy me would have appreciated the link again. 😄
I'll just go look for it on GH

[random pebble]

[23:35:22] [resourceLoad/ERROR] [minecraft/BlockStateModelLoader]: Failed to load blockstate definition ae2:16k_crafting_storage from pack mod/ae2
com.google.gson.JsonParseException: Failed to parse either. First: Not a json array: {"type":"ae2:crafting_cube"}; Second: Unknown element name:ae2:crafting_cube missed input: {"formed=true":{"type":"ae2:crafting_cube"}}

How did I manage to datagen this in such a way that it broke 😄

oh that's funny, I think my codec has a "type" field which clashes with the dispatch codecs type field

Yup, that was it. Somewhat confusing error message but I guess we used an "either" codec to make this work in NF

[random pebble]

We forgot the Event suffix in the classname of RegisterBlockStateModels

I think in general the naming is kinda confusing since it registers a type (unbaked) of block state model

not the block state models themselves

[tall glade]

Finally getting round to the 1.21.5 update, and one thing I would kinda like is a way to roll our own ItemModel/BlockStateModel-style loaders. Effectively I want to provide an ItemModel-style class for one of the registries in my mod.
I somewhat wonder if you could build this on top of the standalone model system, but by allowing the baker to also handle resolution (so the standalone model's id is ignored). So something like:

public interface CustomModelBaker<T> extends ResolvableModel {
    T bake(ModelBaker baker);
}
public static class RegisterStandalone extends ModelEvent implements IModBusEvent {
  public <T> void register(StandaloneModelKey<T> modelKey, CustomModelBaker<T> baker) { ... }
}

I don't know — API design is not my strong suite, so wanted to gather thoughts before diving into a PR :).

[random pebble]

I am actually writing that right now for AE2 parts

But it's completely detached from the model baker

standalone models are for block model JSONs (or custom formats implemented through UnbakedModel)

not for item/blockstate models

The only problem I currently have is: how do I implement a dependency from the vanilla model baker to my own reload listener hm

[tall glade]

Oh, currently, yes! I guess my thinking is that the implementation right now is pretty easy to generalise into something which isn't bound to that. Though whether "standalone model" is the right name at that point is a fair question.

[tall glade]

How are you handling the initial model loading? I can see a way to do this with lots of mixins, but was hoping there might be a cleaner way.

[random pebble]

No I don't think there's a relationship there. Standalone models are just static geometry now

[random pebble]

There's really not much of a relationship between loading a custom blockstatemodel/itemmodel-like.
I have a custom reload listener that loads these like any other JSON resource before models are baked.

Then my actual BlockStateModel for the multipart block will bake all of them as part of its own baking

[tall glade]

"Standalone models are just static geometry now" <- Oh, for sure! So I guess what I'm saying though is I want a way to write have my own custom model class. And the "obvious" API for this would be something like:

• A ModelKey<T> class, that acts as an identifier for each model.
• interface UnbakedModel<T> extends ResolvableModel { T bake(ModelBaker bake);
• An event that you register these models with register(ModelKey<T>, UnbakedModel<T>).
• You then read out these models and apply them in the BakingCompleted event.
  And that's basically what the standalone model API is already!

[random pebble]

You initially said "ItemModel"-like"

ItemModel and UnbakedModel aren't really related

And ItemModel is not static geometry

So... uh... I still don't know what you actually want to do 😄

[tall glade]

No that's fair. I can back up and provide more context. ComputerCraft's turtles can be equipped with upgrades, whose model is dynamic rendered both on the item and block entity (I definitely don't want static geometry!). This is not data driven (maybe in the future, but not now), but instead works by mods registering a (TurtleUpgrade, TurtleSide, ...) -> BakedModel function for each upgrade type. This a) obviously won't work on 1.21.5 and b) was pretty clumsy to use anyway, as you had to register extra models separately, and then look them up again every frame.
So my plan here is to provide a model class like:

interface TurtleUpgradeModel {
  void renderForItem(TurtleUpgrade, TurtleSide, ItemStackRenderState, ...);
  void renderForLevel(TurtleUpgrade, TurtleSide, PoseStack, MultiBufferSource, ...);
  interface Unbaked extend ResolvableModel { TurtleUpgradeModel bake(ModelBaker bake); }
}

This is hopefully less rigid than what was there before, and avoids the whole awkward "register an extra model and then look it up later" pattern.

[tall glade]

So the generic T corresponds to the consumer's ItemModel-alike, and UnbakedModel<T> to ItemModel.Unbaked. Sorry, that was not clear.

[random pebble]

i dont think that's a good fit

[tall glade]

Oh, how come?

[random pebble]

well, it's for requiring static geometry

If you see what that does internally, it still goes through the ModelBakers path for block models

Item models and blockstate models are not in there

[hardy talon]

It's not a reload listener but the best you can probably do is a preparable model loading plugin from the model loading plugin PR I'm working on. The preparable plugins are guaranteed to be ready when model dependency resolution starts which should be sufficient for your use case unless I'm missing a crucial detail

[random pebble]

Well the model manager is a reload listener though

So, my current attempt is to just add a dependency from that to my own stuff

[hardy talon]

The dependency is only applicable to the apply phase though, in the prepare phase all bets are off.

[random pebble]

Hmmmm, good point, that would be very bad

[random pebble]

idk doesn't that make the dependencies kinda pointless though

(and yes, that is exactly what is happening) 😄

[hardy talon]

¯_(ツ)_/¯

[random pebble]

Well. Uh.... So Mixin time, I guess? Fuck 😄

[hardy talon]

At least until the model loading plugins are done, yeah

[random pebble]

I checked your plugin PR but I don't think it does what I need it to do

I didn't check the code though, just the PR description

What I am doing is essentially adding a third column besides blockstate models and item models, but with a dependency from a blockstate model to those (it's part models for one or more multipart blocks)

So loading of mine has to finish before blockstate models are baked

[hardy talon]

It's basically like Fabric's model loading plugins: there are "dumb" plugins and preparable plugins. The latter provides you the ResourceManager and an Executor and expects you to provide a future which loads the additional data you're interested in (sort of like a reload listener). Both the model loading and blockstate loading futures are thenCombine()ed with the plugin future and the dependency discovery depends on both of those futures, so the plugins are guaranteed to be done loading when dependency discovery happens, which is before baking

[random pebble]

Ah so it may actually do what I need it to do, I was unaware of how the Fabric plugins work

So it read to me just like wrapping was possible, not this

[hardy talon]

Yeah, I need to expand the PR description to include preparable plugins

[random pebble]

Assuming I have a custom block state model that acts as a switch/if for other block state models

To avoid baking repated sub-models, I use ModelBaker.compute?

Is there any "contract" that unbaked models actually implement equals/hashCode?

[spare bloom]

your sub-model in this case is the "switch" model?

you can pass a cache key to ModelBaker to compute something only once

[random pebble]

Imagine for AE2 parts....

{ "type": "indicator_switch",

"on": { ... unbaked model for when the part is on .... },
"off": "{ ... unbaked model for when the part is off .... },
"missing_channels": { .... unbaked model for when the part has no channels .... }
}

[uncut dock]

GOTY!

[spare bloom]

{ "type": "indicator_switch",

"on": { ... unbaked model for when the part is on .... },
"off": "{ ... unbaked model for when the part is off .... },
"missing_channels": { .... unbaked model for when the part has no channels .... }
}

not sure what you mean here?

[hardy talon]

One word of warning with respect to ModelBaker#compute(): if your sub model may use that as well in its bake implementation, then you cannot use it, otherwise you will get an exception due to recursive updates

[random pebble]

heh

[spare bloom]

we should maybe patch that

[random pebble]

A naive implementation would unconditionally bake on, off and missing_channels

even if on and missing_channels are identical

you dont have that problem for simplebakedgeometry (i think, I'd actually have to check!)

[spare bloom]

ResolvedModel has a cache

see ModelWrapper#bakeTopGeometry

[random pebble]

That's sensible since the input's kinda just the resource location + modelstate anyway

[hardy talon]

We could, but we'll need to be extremely careful if we do. Baking is highly parallelized now (significantly more than in previous versions), so we'd need to ensure that whatever replacement we implement for the computeIfAbsent() call still guarantees that the computation still only happens exactly once

[random pebble]

I should just check what multi-variant does 🤔

Or if it just assumes baking "deduplication" will happen at the geometry level anyway

That's probably true for Vanilla, and for most mods too. I just run into this when I have a custom block state model that dynamically produces geometry, but might have a cache or pre-built quads at the instance level

Then it'd really not be great to bake that multiple times with the same model state

imagine the example from above, but for AE2 annihilation planes. The geometry for those is fully constructed at runtime, the only thing the blockstatemodel takes as input is a texture sprite.

[spare bloom]

that is not a guarantee, and never was?

[random pebble]

But if you bake that multiple times, it'd multiply the memory usage for its internal cache 🤔

[hardy talon]

Both SingleVariant.Unbaked and WeightedVariants.Unbaked end up as a BlockStateModel.SimpleCachedUnbakedRoot which also uses ModelBaker#compute() to bake the underlying BlockStateModel.Unbaked

[hardy talon]

ConcurrentHashMap#computeIfAbsent() guarantees that

[spare bloom]

oh right

I didn't know that 😄

[random pebble]

Ok granted, I think I dont understand the UnbakedRoot caching system yet for blockstate models, and I'll just ignore it until embedded yells at me for using too much memory

[rocky vector]

I gave up on compacting memory usage of models a long time ago, why do you think I invest so much effort in dynamic loading/unloading 😛

[random pebble]

i have no idea what you even do in that space

this is the first time I hear that 😄

[spare bloom]

Well per part AO got merged before I could have a look

[quick cradle]

Minecraft Snapshot 25w15a

Snapshot on a Tuesday? Yes! From now on we will be moving snapshots to the same weekday as full releases, Tuesdays.This week we are releasing the first snapshot of the second game drop of 2025, including the happy ghast, ghastling, dried ghast and ghast ha...

Apr 8, 2025, 2:00 PM

[rocky vector]

uh what

[spare bloom]

Wtf 😄

[modern vapor]

I think it's because this thread has the Porting tag...?

[random pebble]

@hardy talon So why dont we just patch in BlockStateModel.Unbaked with(VariantMutator mutator); into BlockStateModel.Unbaked?

This is a pretty serious foot gun since the datagen now silently drops these mutators

[modern vapor]

seems to be so -- I'll remove the tag

[hardy talon]

It does not silently drop them, the method is abstract (unless you deliberately decide to use CustomBlockStateModelBuilder.Simple).
The composite builder does drop them because I saw no reasonable way to implement it and I didn't pursue it further because I questioned the usefulness of applying the same modifier to all sub-models

[random pebble]

well yes it's not quite clear what to do, and I did use CustomBlockStateModelBuilder.Simple 😄

So ultimately, both drop them. I wonder if they should at least throw then since as I said above, it's a footgun

Although throwing might make it even less useful, it's not great right now

[hardy talon]

The main problem is that datagen operates to a half-assed degree on unbaked models with the whole MultiVariant thing that is not actually an unbaked blockstate model

[random pebble]

you mean how vanilla does it, not our extension?

[hardy talon]

Yes, vanilla

[random pebble]

Yeah, then Variant is a record so you can't really extend it either

Because I actually face two problems here: some of my custom models might use Variant too, which at least makes it possible to apply a VariantMutator to them

But others need to rotate around the z-axis as well which means I have a custom SpinnableVariant which due to Variant being a record, can't just extend it and add Z

So while composition over inheritance is generally nice, for this particular stuff it's a nightmare 😄

The next issue is that even if we did add a "BlockStateModelMutator" or whatever, that could be applied to all supported custom blockstate models in the multi variant, the property dispatch is keyed to be VariantMutator only as far as I can tell

[hardy talon]

Yup

It's a pain in the ass to weave custom blockstate models into datagen and I currently don't see any way out of this mess that doesn't either involve huge amounts of patches or duplicating vanilla code

[random pebble]

@hardy talon https://github.com/neoforged/NeoForge/pull/2121

Here's what I am thinking: we don't solve this completely but at least add the plumbing to MultiVariant and MultiVariantGenerator to allow people to apply mutators to unbaked models, because that is currently not possible

(this is a prototype so please don't worry about method and parameter names)

[hardy talon]

Looks reasonable at a glance

[rocky vector]

drive-by comment: can we turn it into a class without breaking bincompat?

[random pebble]

I am checking the PR build in my datagen to verify it allows what it's supposed to

[random pebble]

I don't think so, for datagen in particular it's also probably better to just do UnbakedModifiers

since you might want to modify arbitary properties via blockstate properties

That I personally want to add z to Variant is probably just me 😄

[spare bloom]

For the composite unbaked model, one option is to work with Either<Variant, CustomBlockStateModelBuilder>

[random pebble]

The composite is easy

If all we're doing is offering a system to apply <T extends BlockStateModel.Unbaked> T apply(T) recursively

For MultiVariant that are not custom, I just temporarily wrap the Variant in a SingleVariant, apply the mutator and then unwrap it again.

The modder will have to decide what to do with each type of unbaked their blockstate model may have overall when they apply a property dispatch

I know an instanceof soup is not exactly great or nice, but what are you gonna do 😅

[random pebble]

Yes, I believe this works and is relatively comfortable to use. Downside is: requires unchecked casts.

Example of what I do:

protected static UnbakedMutator applyRotation(int angleX, int angleY, int angleZ) {
    return new UnbakedMutator() {
        @SuppressWarnings("unchecked")
        @Override
        public <T extends BlockStateModel.Unbaked> T apply(T t) {
            if (t instanceof SingleSpinnableVariant.Unbaked(SpinnableVariant variant)) {
                return (T) new SingleSpinnableVariant.Unbaked(
                        applyOrientation(variant, angleX, angleY, angleZ)
                );
            } else if (t instanceof DriveModel.Unbaked(SpinnableVariant variant)) {
                return (T) new DriveModel.Unbaked(
                        applyOrientation(variant, angleX, angleY, angleZ)
                );
            } else {
                throw new UnsupportedOperationException("Unsupported unbaked model: " + t.getClass());
            }
        }
    };
}

This works with a PropertyDispatch:

protected static PropertyDispatch<UnbakedMutator> createFacingSpinDispatch(int baseRotX, int baseRotY) {
    return PropertyDispatch.modifyUnbaked(BlockStateProperties.FACING, IOrientationStrategy.SPIN)
            .generate((facing, spin) -> {
                var orientation = BlockOrientation.get(facing, spin);
                return applyRotation(
                        orientation.getAngleX() + baseRotX,
                        orientation.getAngleY() + baseRotY,
                        orientation.getAngleZ());
            });
}

Which then can just be applied normally with the new withUnbaked method:

blockStateOutput.accept(
        MultiVariantGenerator.dispatch(AEBlocks.DRIVE.block(), driveModel)
                .withUnbaked(createFacingSpinDispatch())
);

@hardy talon if you're on board, I'd polish the PR up, add javadocs and get it into a reviewable state

[random pebble]

Okay after adding a builder for these unbaked modifiers, it's not actually that bad:

return UnbakedMutator.builder()
        .add(SingleSpinnableVariant.Unbaked.class, unbaked -> new SingleSpinnableVariant.Unbaked(
                applyOrientation(unbaked.variant(), angleX, angleY, angleZ)
        ))
        .add(DriveModel.Unbaked.class, unbaked -> new DriveModel.Unbaked(
                applyOrientation(unbaked.variant(), angleX, angleY, angleZ)
        ))
        .build();

[spare bloom]

The generic prevents usage of a lambda btw

And the (T) cast is quite gross heh

[random pebble]

Sure but there's no alternative

You need a signature that maps T -> T, not Object -> Object

[spare bloom]

That's what people said with <T> LazyOptional<T> getCapability

[random pebble]

This is not a global registry like capabilities are though 😛

I am not going to introduce type tokens for unbaked models

Not to mention, getCapability was not T getCapability(T)

The problem is that the caller needs the guarantee that the type is not changing

[spare bloom]

Well sure but that's quite unsafe

[random pebble]

Well no, not in the builder implementation 😛

[spare bloom]

In fact the builder could accept a supertype and that would lead to heap poisoning

[random pebble]

That does make that check explicitly

It doesn't accept a supertype though

[spare bloom]

Ah it has to be the exact type?

[random pebble]

Yes, specifically due to the (T) cast at the end

[spare bloom]

Ok good

[random pebble]

I was considering making the UnbakedTransform a class with a private CTOR so you had to go through the builder

But there might be some weird unwrap cases where you'd want to do the instanceof checks for your own weird hierarchy of unbaked models

but in that case you're responsible for the cast safety

[spare bloom]

Yeah that sounds reasonable

The builder is a simple wrapper on top of the lower-level API

[random pebble]

And a lot nicer to use than the lower-level API 😅

(and safer)

The key to the PR is really adding the plumbing to MultiVariant / MultiVariantGenerator / PropertyDispatch / CustomBlockStateModelBuilder as those classes are otherwise completely opaque and impossible to extend.

It's a bit like a Visitor, lol

[spare bloom]

Still a shame that this doesn't work with the datagen types

[random pebble]

I mean I did add a slight workaround/hook for it to work with Variants at least in the context of the builder

Which is this default:

public interface UnbakedMutator {
    default Variant apply(Variant variant) {
        return apply(new SingleVariant.Unbaked(variant)).variant();
    }

[spare bloom]

Applying a variant transform on the inner model should in principle be handled at the level of the DriveModelBuilder

[random pebble]

There is no DriveModelBuilder

And I bet you most mods aren't going to bother with making custom subclasses of the CustomBlockStateModelBuilder since it's most of the time just not needed

Especially since the composite builder internally operates on the Unbaked objects directly

So to apply mutators to that, you have to make the mutators apply to unbaked models

(not builders)

[spare bloom]

Arguably there should be, and we might want to change some things so that CustomBlockStateModelBuilders are kept until the last moment

[random pebble]

Why though?

It's extra code for very little benefit

[spare bloom]

It's the natural place to apply variant mutators

[random pebble]

Mapping an unbaked model from A -> B is not any harder in most cases

[spare bloom]

Otherwise each builder has to be special cased inside the mutator itself which is not great

[random pebble]

At least not compare to writing a completely new builder class on top

You're not going to get around writing custom mutators ultimately

Your custom models properties are arbitrary

There's no common system to address or express those

So yes, obviously you are going to either special case the mutator in your CustomBlockStateModelBuilder

or you are going to special-case the builder in the mutator

I don't see a way around that

(You're also again not special-casing the builder, you are special-casing the unbaked model)

[spare bloom]

We could operate at the level of an Either<Variant, CustomBlockStateModelBuilder>

[random pebble]

I mean the mutator already has two methods

and you can override the apply(Variant) method

i dont see what introducing an Either is going to do

Also: how would that even work... a mutator is just supposed to mutate properties, not the type

[spare bloom]

I would really like to see a prototype doing exactly this

[random pebble]

I don't though 😄

[hardy talon]

The builders are already kept until the very end, the MultiVariant stores the builders and not the CustomUnbakedBlockStateModel

[random pebble]

It's not going to change how this looks or works in the slightest

[spare bloom]

Then how can you know your design is better? Clearly having to special case each model inside the mutator is not great

[random pebble]

I don't understand how you can't see that you have to write the code to manipulate custom properties somewhere

It's not going to be less code just because you write it somewhere else

[spare bloom]

At least for the common case of basically doing a variant mutation but on a custom model

[random pebble]

You can already do that

If you absolutly want to write a custom builder for your unbaked model, you have a with(VariantMutator) method on that

It is not going to work with composite models though

[spare bloom]

So why are you not doing that since all you're doing is rotating an inner variant? 😛

[random pebble]

I am not?

Did you not read the code 😄

You didn't read the code

.... /sigh

[spare bloom]

applyOrientation seems to just modify a variant in your example

[random pebble]

No it modifies a SpinnableVariant

Since Variant is an unextensible record, we cannot add a z rotation to it

Meaning we have to do an entirely distinct variant type, which makes it impossible to mutate that with a PropertyDispatch

[spare bloom]

Ah that wasn't clear to me at all

[random pebble]

Well the core is: you cannot mutate any custom unbaked model property via PropertyDispatch right now

None.

It's all typed to VariantMutator and Variant which are unextensible

Ostensibly I also think it's bad how the two built-in versions of CustomBlockStateModelBuilder work, even if you use VariantMutators

Because of:

• The Simple version takes an unbaked model, and simply ignores any VariantMutators you try to apply (silently)
• The composite version also takes a list of unbaked models, and also simply ignores any VariantMutators

[spare bloom]

Shouldn't we do a UnaryOperator<T> mutator instead?

[random pebble]

We could, ig

[spare bloom]

It can mutate whatever it wants (and defaults to not mutating unknown args)

[random pebble]

Hm?

[spare bloom]

A return x instanceof SpecialVariant sv ? mutate(sv) : x kind of thing

[random pebble]

That just leads to silent failures

variants and unbaked models are stored in distinct fields, I think mixing these mutators is not a good idea

[spare bloom]

It means that it can be passed as-is to child models.

[random pebble]

You still can't pass a VariantMutator

Which is also an UnaryOperator<T>

[spare bloom]

VariantMutator is a special case of that

[random pebble]

That will just crash if you try to apply it to a non-variant

[spare bloom]

You can turn a VariantMutator into this more general object which will mutate variants and not change other types

No, just instanceof

[random pebble]

I am not sure what you aim to achieve with this, honestly. Every Variant is also just an intermediate representation of an unbaked model

SingleVariant.Unbaked(Variant) to be precise

[spare bloom]

I am trying to achieve composition

[random pebble]

composition of what

[spare bloom]

Of generated models

It makes more sense for the hierarchy traversal to be handled by the model builder

[random pebble]

Look, there is no common root or interface here to know which custom models actually have a Variant embeded in them or not

you cannot generically apply a VariantMutator to them

[spare bloom]

You can apply an AnythingMutator 😛

[random pebble]

Which then silently does nothing

and leaves you wondering why your model doesnt rotate

[spare bloom]

Yes. That's why you'd check the generated files

[random pebble]

You're really not helping with disarming the footguns here 😛

If you're in custom unbaked model territory, thanks to there not being a property object model like before, you cannot realistically address recursively applying mutators without casting at some point

If an unbaked model is a composite of other unbaked models, you can recursively apply an UnbakedMutator, but not a VariantMutator

You can see that in CompositeBlockStateModelBuilder, where I do apply the UnbakedMutator to each embedded composite as well

[spare bloom]

Ah yes right

[random pebble]

But the same cannot be done for VariantMutators

[spare bloom]

In fact one could wrap the custom variant into a new kind of UnbakedModel

Then the mutator would only need to be aware of that model type

Basically SingleVariant.Unbaked but for a different type than Variant

[random pebble]

Ahem no? that is the actual custom unbaked model the mod already implements

Variants are just model properties, nothing special, really

unbaked model properties

What we could do however to make this a hell of a lot smoother for custom models that do reuse the Variant set of properties

We could patch in a default method in BlockStateModel.Unbaked

Oh wait, no, we can't really since it's non-generic and couldn't guarantee to keep the same return type argh

return the same type as this, I mean

[spare bloom]

Imagine if there was a drive model builder that could do this.variant = mutator.apply(new SpecialVariantWrapper(this.variant)).unwrap()

[random pebble]

I suppose you could lessen that requirement? I am not sure....

Then it'd be

default BlockStateModel.Unbaked with(VariantMutator m) {
   // throw by default
}

[random pebble]

Ok, what does that do exactly? we don't have a Variant in the drive builder (which also doesn't exist)

Ooops gotta leave. Let's continue this l8er

[spare bloom]

Yeah I gotta work 😄

[vernal parrot]

do we need variantmutators?

what do they do that just looping over blockstate properties doesn't

[random pebble]

variantmutators do not even touch blockstate properties

[vernal parrot]

what are they for then

[random pebble]

they encode a subset of changes to the properties of an unbaked model

[vernal parrot]

I was under they assumption they were for rotating the parts

[random pebble]

yes, but that is not a blockstate property

[vernal parrot]

???

[random pebble]

a blockstate property is facing=down

[vernal parrot]

yeah, and your rotation depends on the blockstate property!

[random pebble]

x=0,y=90 are unbaked model properties

the variantmutator only contains x=0,y=90

no reference to facing=down

[vernal parrot]

yeah but if you don't know what property the blockstate has, how do you know which xy to assign to the unbaked model

[random pebble]

PropertyDispatch

• MultivariantGenerator

[vernal parrot]

and what makes that better than just iterating over the values of the blockstate property

[random pebble]

Your answer doesn't even make any sense

"just iterating over blockstate properties"

does not actually set any model properties

[vernal parrot]

look, I'm just iterating over the Directions and constructing a BlockStateModel.Unbaked for each direction

[random pebble]

Dude you can't just say "haha just for loops"

and begin to copy paste hundreds of lines of old neo helpers first

Really. No.

[spare bloom]

the goal is to make it possible for modders to use the vanilla system

using the old NeoForge system that we explicitly removed is completely off-topic

[vernal parrot]

I never used the old neoforge system or vanilla's, both felt overengineered and incomprehensible to me

look, this is the mindset that I have for datagenerating blockstate files:

[random pebble]

BlockStateBuilder, Variants, PropertyValue, are certainly not things that "are just there"

[vernal parrot]

those are all just helpers I have for constructing unbakeds

[spare bloom]

yeah we're not gonna ask every modder to copy/paste the random helpers you wrote

[random pebble]

Yeah and they're not generalized and thus don't solve the problems posed here

[vernal parrot]

they're totally generalized

[random pebble]

The vanilla system allows mix&match composition between different dimensions of property changes

and it will assemble it into one set

[vernal parrot]

but, look, this is the mindset that I approach datageneration of blockstates with:

I start with what the blockstate file looks like

[random pebble]

For example:

blockStateOutput.accept(
        MultiVariantGenerator.dispatch(AEBlocks.GROWTH_ACCELERATOR.block())
                .with(PropertyDispatch.initial(GrowthAcceleratorBlock.POWERED)
                        .select(false, plainVariant(unpoweredModel))
                        .select(true, plainVariant(poweredModel)))
                .with(createFacingDispatch(90, 0))
);

[random pebble]

This separates defining what a facing property does from what a powered property does and allows composition of the two

without repeating this over and over and over

[vernal parrot]

I look at this and go, "okay, we have combinations of blockstate property-values, and we're assigning things to them"

[random pebble]

Look Commoble, there's really no point in dragging this out. We're not going to recommend modders copy paste your utilities.
The recommendation is to use the Vanilla system.

[vernal parrot]

"so I should be able to loop over the relevant properties, like an outer loop for facing, an inner loop for rotation, and for each combination, construct a whatever for that combination"

y'all can do whatever you want but I'm gonna keep giving people my blockstate builders when they get confused by vanilla's system being weird and overengineered

been doing it for years

[random pebble]

@spare bloom So, getting back to our previous discussion

What was

Imagine if there was a drive model builder that could do this.variant = mutator.apply(new SpecialVariantWrapper(this.variant)).unwrap()
this supposed to accomplish?

[spare bloom]

so here in your builder you would only need to handle that wrapper model

i.e. SpecialVariantWrapper

[random pebble]

okay but why? if it's your builder it's already capable of pulling out the variant (if there is one, there isn't one for drives), applying the mutator and creating a new unbaked

And what would your mutator actually be defined on(?)

I dont understand your goal with this yet

[spare bloom]

so the thought is

traversing the models to find the thing you actually want to mutate is a bit awkward to do inside the mutator

return UnbakedMutator.builder()
        .add(SingleSpinnableVariant.Unbaked.class, unbaked -> new SingleSpinnableVariant.Unbaked(
                applyOrientation(unbaked.variant(), angleX, angleY, angleZ)
        ))
        .add(DriveModel.Unbaked.class, unbaked -> new DriveModel.Unbaked(
                applyOrientation(unbaked.variant(), angleX, angleY, angleZ)
        ))
        .build();

in this example the actual mutation happens in applyOrientation; so this whole thing is just glue to modify the SingleSpinnableVariant inside multiple models

[random pebble]

Well yes, but that is primarily due to me not wanting to write a SpinnableVariantMutator

And yes, precisely, it's a mutator for the unbaked models 😛

Since that's the part that's otherwise opaque

But I don't see what your system actually solves? The unbaked mutator is a "generic" interface in that it can't have a method apply(SpecialVariantWrapper)

Since SpecialVariantWrapper is already a class specific to a mod

Also please consider this: the variant is just a utility here, it's not even necessary to have that.

The properties found in SpinnableVariant could just as well be inlined directly into the unbaked model (as they actually are in the JSON as well)

[spare bloom]

yeah I think we'll go for what you have

you are right that this should really be operating on the unbaked models

[random pebble]

So I just wonder if we do want to make it a class+private ctor for now to prevent people from accidentally using it without the builder 🤔

I still think that the with(VariantMutator) methods silently not doing anything is bad, but other than just making them throw, I don't really have a super good way to fix it hrmpf.

[spare bloom]

We can try

[spare bloom]

Ok let's make it throw now that we can offer a proper alternative

[random pebble]

@spare bloom What do you think about this change? https://github.com/neoforged/NeoForge/pull/2121/commits/d33b9857aeffdf72103d150543d697d70c558491

That would change it so models that do not support VariantMutators will throw, but at the same time it adds support for variant mutators to be applied to composite models. (CC: @hardy talon )

It could be seen as redundant to have that with(VariantMutator) at both the builder and unbaked model level, but otherwise I think it's not possible to really support it

[spare bloom]

I don't like adding datagen helpers into the model classes

[random pebble]

Well I thought about that, but this is in line with Vanilla

They put VariantMutator into net.minecraft.client.renderer.block.model,
and they also added Variant#with(VariantMutator) which is essentially the same thing.

I'll still revert this for now and repopen it as a separate PR later

[spare bloom]

Hmm

[spare bloom]

That's true

Wait

Isn't it a different Variant?

[random pebble]

No

What do you mean by different variant 😄

Variant is the unbaked blockmodel part

Alright I think I polished up the unbaked mutator PR https://github.com/neoforged/NeoForge/pull/2121
If we wanted to add support for applying VariantMutators we can do that in a follow up PR it doesnt really interact with UnbakedMutators

[random pebble]

@hardy talon you sure we should adapt the parameter names if those are just 1:1 copies of the vanilla methods?

I had hoped this would make it easier to compare them to the vanilla versions in the future

[hardy talon]

I would prefer adapting the parameter names simply for readability. As far as I'm aware, JST won't apply Parchment names to these

[random pebble]

Mhmmm true since they're not just adding params

Urgh, well ok, that sucks for that long block that's copy pasta

Oh ffs, I actually wrote javadoc for more but forgot to genpatches and it's lost 😄

Oh no, thank god for local history!

[spare bloom]

@hardy talon do you want to look at https://github.com/neoforged/NeoForge/pull/2135 or should I merge it already?

[random pebble]

Is there even a point to keep StandaloneModelBaker if UnbakedStandaloneModel exists?

[tall glade]

I think it's still worth having some utilities for baking single models, especially for all the pre-existing formats like SimpleModelWrapper.
Though you could do that different way — the approach we went for in FAPI was to have something like:

class SimpleUnbakedStandaloneModel<T> implements UnbakedStandaloneModel<T> {
  public SimpleUnbakedStandaloneModel(ResourceLocation modelId, BiFunction<BakedSimpleModel, ModelBakery, T> baker);
  static SimpleUnbakedStandaloneModel<SimpleModelWrapper> simpleModelWrapper(ResourceLocation modelId);
  // ...
}

IMO it's not worth the breaking change right now, but happy to discuss.

[hardy talon]

Go for it

[random pebble]

We can just move the utilities to the new intergace

[random pebble]

That also works, but we're early in 21.5 and can break if we want to, or mark it as deprecated for removal in 22

But I don't like the two separate event methods. People then have to grok that it's basically the same thing and one is just a convenient wrapper around the other

[spare bloom]

There's a good chance that we rework this once model loading plugins land anyway

Anyway @random pebble your call. I'm happy to merge the PR as is

[random pebble]

eh go ahead but you know how it is, this may still be there in a year 😄

[spare bloom]

🤷

[tall glade]

Oh, I will be ready with a scalpel when 1.21.6/1.22 comes round.

[tired rock]

model loading plugins?

[hardy talon]

@warped sluice I'm finally getting back to https://github.com/neoforged/NeoForge/pull/1884 (model loading plugins) and I've got two questions:

• Do you have/know of any example use cases of the various modifiers, primarily the item model ones and the UnbakedModel on-load modifier but also the split between on-load and pre-bake for blockstate models?
• Since, at least to my understanding, you wrote most, if not all, of the model loading plugin system in Fabric, I'd like your approval to implement this into NeoForge since I'm effectively cloning your work from Fabric

[warped sluice]

OnLoad was ported forward from the initial version of the API and I can think of theoretical uses but I don't have anything concrete. It's probably not that useful, especially because the UnbakedModel only returns self attributes instead of self-and-inherited attributes like ResolvedModel

The item model events just mirror the block events for the most part - OnLoadItem is missing, and it is better suited for some hypothetical cases than BeforeBakeItem (much like for blocks - see the message below), namely being able to change model dependencies in resolve. I also don't have real use cases for these events, but I think they are more useful than OnLoad.

The block events are the simplest to justify for me. During the 1.21.4 port, both bake events were removed and only the on load event was kept. Purely in terms of functionality, on load covers both bake events as well, but not having the bake events is extremely inconvenient and somewhat inefficient in some cases. However, on load is also necessary, as wrapping during this event instead of during before bake allows changing model dependencies and model groups. The reasonable conclusion seemed to be that all three events are necessary and make all possible use cases possible and convenient.

See the second bullet point here https://github.com/FabricMC/fabric/pull/4320

In case it's relevant, Continuity only uses after bake block nowadays out of all events, but I haven't reimplemented emissive items in any version 1.21.5+ due to item model changes. It's likely that this will work by wrapping or otherwise interacting with VertexConsumers rather than by wrapping UnbakedModels or ItemModels, however. Wrapping UnbakedModels might work to some extent but would ignore dynamic geometry created inside ItemModels, while wrapping ItemModels requires fully inspecting the ItemStackRenderState, which isn't convenient and isn't even possible without using Fabric internals at the moment due to FRAPI not exposing a layer's non-vanilla geometry.

[hardy talon]

That sounds sensible to me. I'm gonna try getting another opinion on potential use cases and then I might remove the OnLoad modifier. We can always reintroduce modifiers when someone brings up a use case

[hardy talon]

@warped sluice do you still have plans to remove BlockStateResolvers from the Fabric model loading API or did you ditch that plan?

[warped sluice]

I would still prefer to have definition types instead, yes

[hardy talon]

The reason I'm asking is that I'm discussing use cases with someone and they are running into the problem of none of the modifiers being truly suitable for their use case (assigning models to dynamically generated blocks based on the presence of other mods' blocks) and definition types being an annoying solution due to requiring an in-memory resource pack to provide the blockstate files using them

[warped sluice]

Something similar was brought up in Fabric API discussion but for items, and my verdict is that using a virtual pack is the correct solution because a modloader API cannot be expected to provide a way to register every possible type of parsed resource in-code, and if that's only exposed for some things, like block and item models, then it sets an unrealistic precedent and is inconsistent

[hardy talon]

That's a fair point, yeah

[hardy talon]

I've thought some more about the blockstate resolver topic and I've come to the conclusion that I would prefer it to be solved in one of two ways:

• Add blockstate resolvers in the way they currently exist in FAPI. They have valid use cases (such as the dynamically generated blocks mentioned above), their maintenance overhead is zero since the on-load modifier for block models exists anyway and virtual RPs are frankly a complete pain in the ass to deal with.
• Change the on-load modifier for block models to iterate over all possible blockstates, retrieve the associated model from the map of loaded blockstate models (which is null if no blockstate file was loaded for the state's block), run the modifier on that and place the new model back in the map if it's non-null and not reference-equal to the existing model, if any, instead of running the modifiers only on the loaded models. If I'm not mistaken, this is how the on-load modifier for block models worked in older versions, so there's some precedent for this behaviour.
  I would appreciate more input on this, ideally from people who would make use of this.
  I would also be very curious what the opinion of people currently using blockstate resolvers on Fabric is on the topic of removing them in favor of blockstate definitions instead of having both.

[rocky vector]

virtual RPs are frankly a complete pain in the butt to deal with
this is mostly because everyone has to reimplement PackResources themselves, right?

[warped sluice]

In 1.21.4 on load for static models did accept and return a nullable model but that was only because this hook was moved to when a model was requested during resolution instead of when it was actually loaded, and that was a mistake on my part. The only other case of nullable models were null BakedModels when vanilla still accepted those and mapped them to a missing model. AFAIK nothing else ever interacted with nullable models. The 1.21.8 implementation of both on load events uses replaceAll on the appropriate model map, which can be considered a form of iteration.

[hearty nova]

eh, it's not that bad

[forest glade]

Partially; it's more that you have to build whatever the model is for the resource for your thing, then serialize it to JSON, only for it to be pulled back from JSON later. And you can't even use the normal datagen system at runtime without... certain issues (I mean, it's doable, it's just a bit painful). And you have to get your PackResources properly registered (once again, doable, you just have to be careful and make sure you understand how the event works) and... it's just a lot of moving parts, many of which are easy to screw up

[random pebble]

Isn't it also slow as fuck to first serialize stuff into a JSON byte blob for it to be then deserialized again via codec?

[spare bloom]

Do we have a use case for resolvers that is not served by a custom blockstate definition?

[rocky vector]

haven't measured but it wouldn't surprise me if it ends up being about the same as the overhead of going to zipfs for it

[random pebble]

Well yes, isn't that additive? 😅

Or what do you mean by that?

I more or less meant in comparison to constructing the blockstates directly as java objects

[rocky vector]

It wouldn't surprise me if the time spent generating the JSON byte blob in memory is the same as the time spent reading an equivalent JSON out of the jar

Of course being able to construct them directly will be significantly faster

But as Pepper mentioned we don't natively offer that functionality for other resources that are regularly customized (e.g. recipes)

[random pebble]

MAYBE WE SHOULD?!?!?!? 😄

[rocky vector]

I agree but there were concerns about it being impossible for datapacks to override the last time that was brought up

IMO, I'm not sure it's worth being concerned about, because the ecosystem is already doing it (cf: KubeJS)

[forest glade]

If you cache it it's pretty cheap after the first time. But then you have to implement caching. I literally have a mod for this

[forest glade]

This is a huge worry imo -- offering a non-datapack path for any of that stuff is worrying on two counts -- (a) datapacks overriding it wouldn't work out of the box, and (b) (of more direct concern to myself) people doing dynamic resource/data generation can't look at the exiting resources if they're made that way.

[queen elm]

doing this would prompt people to not do datapcks at all an purely register stuff via code which makes changing something via datapack at least very difficult if not impossible

[random pebble]

doesn that highly depend on how you wire this stuff into the datapack registry loading?

[forest glade]

Not really in general; or, rather, to behave sensibly with respect to all the ordering you end up having to have something to link it to a packresources somewhere so that it has a place in the ordering, which I suppose is technically possible, though... less so the more you want to avoid serializing it to JSON abd back. But even if you do that it's still a royal pain for folks like me who do dynamic generation based on existing resources, since I basically have to figure out how to get at the relevant stuff ahead-of-time and pull it to JSON on-demand anyways and that's just a pain

[rocky vector]

Ability to introspect and minimal patch surface are the main advantages of requiring a round-trip to JSON even for objects created at runtime

[hardy talon]

I can understand wanting to keep the amount of data-driven things where the files can be bypassed to a minimum but there are some cases where doing this with a virtual pack requires completely insane workarounds. The case I ran into were loot tables but with more and more things becoming data-driven, this will affect more and more things as time goes on. This is the kind of nonsense I had to resort to in order to make loot tables work (incidentally with exactly the library luke mentioned a couple times above): https://github.com/XFactHD/BuddingCrystals/blob/1.21/src/main/java/xfacthd/buddingcrystals/common/dynpack/generators/BlockLootTableGenerator.java. The worst part of it is this garbage: https://github.com/XFactHD/BuddingCrystals/blob/1.21/src/main/java/xfacthd/buddingcrystals/common/data/UnownedReferenceHolder.java

[rocky vector]

I forgot that registries make datagen non-pure now

Ugh

[hardy talon]

As for blockstate files specifically: blockstate resolvers and/or custom blockstate definitions would usually be used for very custom behaviours which get completely broken if overridden by another pack whereas overriding a recipe or a loot table would usually still work as far as the player cares (happy to be proven wrong on that). Case-in-point: FramedBlocks uses a custom blockstate definition in 1.21.5+ to wrap the models and make the dynamic shenanigans work. If a pack were to override the blockstate files, the whole thing falls apart and all you get are a bunch of cubes with the base frame texture.

[hardy talon]

Yup, unless you do BS like that or create the JSON from a hard-coded template string, there's no way to put a loot table that depends on an enchantment in a virtual pack since the pack's contents need to be known before any dynamic registries are loaded but you'd need those registries to create the pack's contents in the first place

[blissful jacinth]

not really

create does runtime datagen, it barley takes any extra time

i was looking into if it's worth working on a cache, and seeing as it only took a few ms max i decided it wasn't worth it

[forest glade]

I had a way to do this I'd prototyped ages ago... ugh, I should dig that up. It contained crimes against holders

[forest glade]

That said: yes, some runtime generation becomes more painful due to how vanilla does this. But consider that one of the major cases of runtime generation is when you're doing generation based on existing resources (I know of at least a few other libraries that do this...). So if you make the actual runtime generation easier by letting you register stuff without going through resources... but then you make one of the major use cases of such runtime generation harder, if not impossible, in the process... well, you've not really gained that much

[hardy talon]

I'd be really curious what this looks like compared to the crimes I had to commit