#1.21.5+ Models

1 messages · Page 1 of 1 (latest)

queen elm
#

Models

split hollow
#

👀

bronze gorge
#

Finally, we're getting rid of models. I've always said we should be hardcoding vertices into raw opengl vertex arrays \s

split hollow
#

you are not wrong, we really should just use raw arrays of vertex data

bronze gorge
#

it's obviously where all of vanilla Java MC's optimization problems lie

dense mesa
#

please... what have they done this time...

#

they just renamed some classes and move some of them around as part of minor cleanup.. right... right???

stray fog
#

Of course, of course... *increases su5ed's copium dose*

#

(I don't actually know what the model changes in 1.21.5 snapshots are)

swift sierra
split hollow
dense mesa
#

finn

split hollow
#

s5used

elfin junco
whole aspen
#

-# note: gander is still on 21.1 because NOPE

modern vapor
#

-# the future is now, old man

tired rock
#

-# Oh well. Gander is quite impressive though.

naive lance
#

-# cant wait to gander

spare bloom
#

first thing I did was porting some of the BakedModel extensions to BlockStateModel

last wave
#

Like what?

spare bloom
#

in case you don't know, BakedModel does not exist anymore

last wave
#

I know

#

I was just wanting an overview

spare bloom
#

for blocks, BlockStateModel is a rather direct replacement

last wave
#

So I can get my bearing

#

I like that

#

That is a much better API

stray fog
#

can someone TL;DR what has changed?

last wave
#

I am still reading up on it

#

So I am not the right person yet

#

Maybe tech can write up a couple of lines

pulsar lantern
last wave
#

Oeh

spare bloom
#

let's just say the system was refactored more or less completely

last wave
#

That is not helpfull.....

#

What did they actually change

spare bloom
#

yes

last wave
#

opens up kits

stray fog
spare bloom
#

read model rework instead

stray fog
#

what is that?

last wave
stray fog
#

oh a section there

#

I'll get there I'm skimming downward

queen cradle
#

from 25w06a to 25w08a there were 41 reject files and 24 of them were in net.minecraft.client.renderer and net.minecraft.client.resources

spare bloom
#

I wonder how BlockStateModels are even loaded now 🤔

last wave
#

I assume through BlockStateModelLoader

#

Not sure yet where that is called from though

#

What I am wondering is whether we need to extend the model loader mechanic to each of the loader kinds

stray fog
#

am I correct in understanding that BlockStateModel is a dynamic model that selects between different Block models based on the provided state?

spare bloom
#

yes

last wave
#

Yep

spare bloom
#

it is ItemModel for blocks, if you want

last wave
#

That is at least how I read the code

spare bloom
#

you can also use it to return fully dynamic quads if you want to

#

so there's something called BlockModelDefinition now

last wave
stray fog
#

right, so BakedModel has become a more slender "QuadCollection"

#

the lengths mojang go to do not use the word "Mesh" :P

spare bloom
#

my question is: how can modders create their own block models

last wave
#

Well the line above it

#

That then instantiates the UnbakedBlockStateModel

#

We likely need to patch at least several locations this time

#

....

#

Ur´gh

spare bloom
#

I think it's impractical to modify the block state format

#

at least, it would be different compared to how the system worked before

#

depends if we want the system to work with model inheritance or not (i.e. "parent")

#

if we do, hacking on top of UnbakedModel is basically a requirement

last wave
#

Yeah

#

This all feels, sticky meh at best

#

Hmm

spare bloom
#

actually I think it can be reasonable clean

stray fog
#

it is a huge blergh

last wave
#

My question is, for complex models do we need UnbakedModel

#

Or can we implement ontop of BlockStateModel / ItemModel

#

And get away with that

spare bloom
#

if we add something like BlockStateModel bakeBlockModel(...) to UnbakedGeometry

last wave
#

That seems counter to what mojang is trying to do though

#

With them keeping the implementation for blockstate and items completely seperate

spare bloom
#

well, the problem is that we need a way to load block state models at all

last wave
#

But it would be a cleanish solution

last wave
#

Which should be fine

spare bloom
#

what gson?

last wave
#

Sorry

spare bloom
#

it's the blockstate json

last wave
#

I looked at BlockModelDefinition

#

My mistake

#

God damnit

#

These names are confusing as all hell

spare bloom
#

well same problem IMO; that thing would be really annoying to work with directly

stray fog
#

err

#

that kits link

last wave
#

Maybe unsure

spare bloom
#

you wouldn't want to inline all the json for a model inside the blockstates file

stray fog
#

is named 06a

#

is that up to date?

last wave
#

Yeah

spare bloom
#

no

#

we are on 08a

last wave
#

At least for this discussion it is

#

That is not pushed

#

No it is

#

Github is lying again

stray fog
#

well I can't find BlockStateModel

last wave
#

ARG

stray fog
#

and UnbakedBlockStateModel has a BakedModel return

last wave
#

That is why I thought it would be patchable in the GSIN

#

It is a codec now.....

stray fog
#

would be best to look at Snowman

spare bloom
#

if you look here, this is the perfect place to intercept the resolvedmodel and turn it into a more complex BlockStateModel IMO 😄

image.png
#

what's with ResolvedModel that has a single implementation yet it has some default methods that are overridden??

stray fog
#

oh god this is horrible for modding

#

BlockStateModelLoader makes BlockStateDefinitions, which either hold a MultiVariant, or a MultiPart

#

and both bake straight into SimpleModelWrapper

#

wait no multipart bakes into BlockStateModel

spare bloom
#

I don't think it's that bad

stray fog
#

MultiVariant is the bad one

#

I guess we can patch calls to bakeModel

#

wait I see what you mean with ResolvedModel

spare bloom
#

yes

#

that is the place where a ResolvedModel gets turned into a BlockStateModel

stray fog
#

right but that method returns SimpleModelWrapper

#

not BlockStateModel

spare bloom
#

easy fix though

stray fog
#

we have to at least make a new method with a more generic signature

spare bloom
#

sure yes something like that

stray fog
# 
    public static /*SimpleModelWrapper*/ BlockStateModel bake(ModelBaker $$0, ResourceLocation $$1, ModelState $$2) {
        ResolvedModel $$3 = $$0.getModel($$1);
        //TextureSlots $$4 = $$3.getTopTextureSlots();
        //boolean $$5 = $$3.getTopAmbientOcclusion();
        //TextureAtlasSprite $$6 = $$3.resolveParticleSprite($$4, $$0);
        //QuadCollection $$7 = $$3.bakeTopGeometry($$4, $$0, $$2);
        //return new SimpleModelWrapper($$7, $$5, $$6);
        return ModelLoadingHooks.bakeCustom($$3, $$0, $$1, $$2);
    }
#

conceptually of course

#

that assumes we don't want to mess with blockstate selectors because the traditional multivariant and multipart approaches both still work

spare bloom
#

yeah

#

IMO if you want to mess with selectors you do it in your custom model

#

at least that's how it was usually done in the past

stray fog
#

yeah

#

since BlockStateModel still gets you a BlockState

#

you can have a MultiVariant model with a single entry

#

and point it at your selector model

spare bloom
#

yup

stray fog
#

it could be nice to add API for custom loading blockstate jsons

#

but it isn't necessary

last wave
#

Seems reasonable

stray fog
#

so blocks are not too bad

last wave
stray fog
#

we just have 1 patch target for blockstate mapping

spare bloom
#

difficult to compose I think

stray fog
#
  • whatever we need for the models themselves
spare bloom
#

unless you can mix and match blockstate "subformats", there is no real benefit over just pointing to a custom model that can do whatever it wants

#

moving on, the first thing to restore I think is extra context for unbaked model

stray fog
#

well I was thinking like, in a blockstate json, instead of selectin based on the presence of "variants", vs "multipart", have an optional dispatch key "neoforge_selector_type"

#

but all we gain from this is that the selector gets put in the blockstate folder instead of models folder

#

it's more elegant conceptually, not in code

#

now itemmodels ... those I don't understand yet

#

do they render in the update method or something?

spare bloom
#

the model prepares the render state, then it gets rendered later

#

(that hasn't changed too much since 1.21.4)

stray fog
#

so the quads are cached for rendering

#

I'm not a fan of the addAll call specifically

#

it's doing more work than just setting a prepared list by reference

spare bloom
#

I'm not sure that composite should remain

#

vanilla already has a composite model for items

#

maybe for blocks, but it would be weird if it did not work for items

last wave
#

There was some reason it was kept....

#

Trying to find it again

stray fog
#

but uh

#

if we make a CompositeBlockModel

#

it will only work with block models by default

#

vanilla's CompositeModel is a bit meh though because it doesn't have names for the parts

#

that doesn't matter for the baked model though

#

it only matters for inheritance

spare bloom
#

the answer is that item models are not inherited kek

#

we could however wire up vanilla's composite item model with our part visibility extension, I think

stray fog
#

they removed "parent"?!

spare bloom
#

item models were never inherited afaik

#

ItemModel, the class

stray fog
#

oh you mean the client item stuff

#

yeah

#

oh ew I just realize this is already ugly in 1.21.4

#

okay so what they changed is that ItemStackRenderState now fills a list of quads, instead of passing a BakedModel by reference

#

... that seems worse to me

#

they made this specific piece of code worse

spare bloom
#

there is no bakedmodel anymore

stray fog
#

yeah but the BlockModelWrapper contains a List<BakedQuad>

#

and it could set the list instead of adding its contents

#

since this is a baked model the list should be immutable

#

so there should be no issue with passing the list by reference

spare bloom
#

ok I have restored the ContextMap-based additional properties

#

I think now is a good time to restore UnbakedModel loaders

#

I will also move net.neoforged.neoforge.client.model.UnbakedModelParser.init(); to Minecraft because I don't like doing it multiple times (looks like a mistake in 1.21.4)

random pebble
#

I am late to this party

#

the getModelData method you envision that the model itself snapshots the worldstate it needs to generate its quads later?

#

is that it?

spare bloom
#

that's already the case

#

BlockStateModel hasn't changed substantially

random pebble
#

Ah, yes ok. It's essentially snapshotting world-state though, that's what I was getting at. I hope the Javadoc reflects that 😄

spare bloom
#

what javadoc? 😄

random pebble
#

😄

spare bloom
#

not sure how we should port applyTransform

    /**
     * Applies a transform for the given {@link ItemTransforms.TransformType} and {@code applyLeftHandTransform}, and
     * returns the model to be rendered.
     */
    default void applyTransform(ItemDisplayContext transformType, PoseStack poseStack, boolean applyLeftHandTransform) {
        self().getTransforms().getTransform(transformType).apply(applyLeftHandTransform, poseStack);
    }
#

probably as an ad hoc customization point for item models

#

(if it's still necessary at all)

#

alright, restored data-driven render types

last wave
#

Nice work!

spare bloom
#

thanks 😄

#

I think with this we should be able to port the obj loader relatively easily already

last wave
#

I wondered about this

#

Do we need two model loaders now?

hardy talon
hardy talon
# last wave Do we need two model loaders now?

Not necessarily. I think it would be sufficient to provide an additional defaulted method in UnbakedGeometry which can bake to a BlockStateModel and defaults to creating a SimpleModelWrapper. That way model loaders should be able to spit out dynamic block models without splitting the whole thing into two systems

last wave
#

That should indeed be possible

#

And a good solution IMHO

spare bloom
#

Yeah that's what I had in mind as well

#

We could also have the method on the unbaked model directly but then it would be weird with inheritance

#

I think that putting it in UnbakedGeometry is ideal

hardy talon
#

@spare bloom are you currently touching any of the model stuff? I'm going through compile errors in that area at the moment and want to make sure I'm not stepping on your toes by doing so

spare bloom
#

not doing anything but was gonna resume work just now

hardy talon
#

Ok, then I'll push my changes in a second and then move to entity stuff

#

Pushed, have fun 😄

spare bloom
#

thanks 😄

hardy talon
#

Tech, you can make EmptyModel even simpler by using vanilla's UnbakedGeometry.EMPTY

spare bloom
#

oh nice

#

XFact do you remember why IModelBakerExtension#findSprite exists?

#

there is now SpriteGetter#resolveSlot that seems to replace it, at least

hardy talon
#

Yeah, that looks like a 1:1 replacement

spare bloom
#

great, thanks

#

alright, now UnbakedGeometry can return a custom BlockStateModel to use

#

I think that with this the bulk of the work for models is done

last wave
#

This looks very nice Tech!

#

Thanks for all the hard work

spare bloom
#

np 🙂

#

now I stop for today 😄

last wave
#

Also thanks for properly communicating in here

#

Major props!

spare bloom
last wave
#

Power of the community!

hardy talon
#

@spare bloom did you intentionally not wire up the render type in BlockModelWrapper or is that an oversight?

spare bloom
#

oversight

#

good catch, you can fix it 😄

hardy talon
#

Will do 👍

hardy talon
#

We've got four remaining rejects related to models. The BlockModel reject should be resolved, unless I'm missing something. The other three (ModelBakery, ModelDiscovery and the remaining part of ModelManager) are related to standalone models. Has anyone given those any thought yet? I'm not sure what the best way to resolve those is (dumbing them down to QuadCollections doesn't seem like a particularly great idea to me)?

spare bloom
#

yes the block model is resolved. I was keeping it around until I got to exposing custom BlockStateModels

#

Pepper has given some thought to standalone models, see discussion in the Fabric thread

#

the summary is that it's not clear what to give as a final (i.e. "baked") type

hardy talon
spare bloom
#

yup!

hardy talon
spare bloom
#

so the options are either:

  1. simply return a standard record that contains most/all of the relevant properties
  2. let the model be baked into any type. basically offering a new type of "baked model" that can be customized for each application
#

i.e. for 2 you basically get a ResolvedModel and a ModelBaker and you turn that into whatever you want

#

whereas for 1 we'd extract standard state (geometry, particle, etc... everything used either by block or item models) into a record

#

now that I have gotten more familiar with the system, I would be tempted to say that 2 is the better option

#
  1. does not allow retrieving non-standard properties, but arguably that is not a common use case
hardy talon
spare bloom
#

if you wanted this to be clean, you'd probably need something like StandaloneModelKey<T> though where T is arbitary

warped sluice
#

If you use a map at all

#

The user could assign things to their own static fields somewhere

spare bloom
#

could be done too but it's a little bit ugly

warped sluice
#

Then the user needs to store their own keys in static fields instead

hardy talon
warped sluice
#

Right, changes to visible state should happen in ModelManager.apply

#

Ok, I suppose a map is necessary then

#

But why not approach 1?

hardy talon
#

Unless I'm missing something, option 1 would require the models to be "dumb" and static once baked. This would make it impossible to do dynamic standalone models through model loaders

spare bloom
#

and it also makes it impossible to retrieve extra "baked" context

#

Pepper since last night I basically reimplemented almost all the model patches 😄

#

notably, additional properties following a very similar pattern: fillAdditionalProperties in UnbakedModel and getTopAdditionalProperties in ResolvedModel

warped sluice
spare bloom
#

well, you have BlockModelWrapper, SimpleModelWrapper and any custom BlockStateModel that each use their own "baked" subset

#

so we could follow suit and let each user decide which baked subset they care about; potentially they just call e.g. SimpleModelWrapper.bake if they want to use the standalone model as a BlockStateModel

stray fog
#

reading up, what is the purpose of the standalone model?

#

I'm not sure what the purpose is

warped sluice
hardy talon
stray fog
#

for "models" in BER I want to reintroduce the Renderable system eventually, which I only learned yesterday had been deleted

#

the goal for that is to have an object that can draw itself the way say, an entity model does without a list of BakedQuads being involved anywhere

hardy talon
#

The geometry for that still has to come from somewhere and using a baked model for that is one of the simplest options

stray fog
#

the OBJ model already encodes the geometry in its own way

#

I think my initial goal was missed when updating the renderable API

#

cos it ended up as Mesh objects containing lists of BakedQuad

#

which completely defeated the point of not constructing BakedQuads that I initially had in mind

#

but regardless

#

BakedQuad or not

#

there's then two distinct use cases for standalone models, then

#

ones in which the ultimate goal is to call render(BufferBuilder) and you don't care what the internal storage format is

#

and ones in which the ultimage goal is to call getQuads() and you'd rather the internal storage format was already BakedQuads so this call is as fast as possible

warped sluice
hardy talon
#

Yup

warped sluice
#

I would like to see more specifics of the system

#

I agree with it in principle but am unsure of how much it will actually be used

#

I suppose the same for standalone models approach 2

hardy talon
spare bloom
#

(Btw we might want to remove part visibility at some point if nothing uses it)

hardy talon
#

OBJ models use it and composite models do as well, if we can save that

spare bloom
#

For standalone models I would imagine that it could look like

public static final StandaloneModelKey<PipeModel> PIPE = StandaloneModelKey.of(resource location);

// Later:
event.registerStandaloneModel(PIPE, (resolvedModel, baker) -> /* do stuff and return a Pipe */);
#

And it requires a query method for later, as well

warped sluice
#

Does it need an RL?

spare bloom
#

Yes you need to know which UnbakedModel model it needs to load

#

Well actually that's not necessary

warped sluice
#

It could extend ResolvableModel

#

That would be most parallel to vanilla

spare bloom
#

I don't have the code in front of me anymore

#

You get the general idea at least. Details idk

#

The API surface should manage to remain very small

warped sluice
#

Yes

hardy talon
# warped sluice It could extend ResolvableModel

I'm not sure forcing everyone to implement that is a good idea. Having to do that for the "dumbest" of models feels like bad API design to me. Most, if not all, standalone models I have in FramedBlocks for example would be RL -> QuadCollection because I only care about the quads and nothing else.

warped sluice
#

Is it not feasible to provide a simple default?

#

Possibly like new SimpleStandaloneModel<>(rl, (resolver) -> {...})

rocky vector
#

do we really need standalone models as a concept?

warped sluice
#

Yes

rocky vector
#

in most cases they seem to either be bound to a block or an item

warped sluice
#

Create uses them exclusively instead of ModelParts

hardy talon
warped sluice
#

ResolvableModel, not ResolvedModel

hardy talon
#

Right facepalm

stray fog
#

I have 2 use cases for standalone models.
primary one is this. in this case (ender-rift 1.21.4) I am using a standalone baked model, and rendering it from a BER

image.png
#

second one was in elements of power, where I used the renderable API so I could draw independently without having those textures stitched in the block atlas

image.png
spare bloom
#

What is even this renderable API? Sounds a bit pointless to me tbh 😛

#

In both cases you only need the geometry

stray fog
#

well I may be the only person that uses it, cos it was deleted in 1.21.4 without anyone telling me

spare bloom
#

I didn't even know it existed lol

stray fog
#

the goal for the renderable API was specifically to have an API that avoids going through BakedModel

#

and all the implications for it

spare bloom
#

Might have been collateral damage

stray fog
#

and to allow animating individual parts of a model separately

#

spoke to Orion, apparently was removed because it depended on GeometryBakingContext

warped sluice
#

Create only uses BakedModels but it doesn't use Renderable at all so I don't really see the point either

stray fog
#

well the point was to AVOID using BakedQuads

#

cos in my mind it was too many levels of unnecessary indirection, when you have an OBJModel that already has the vertex and index lists

#

and can push them straight into the buffer

warped sluice
#

Create converts the BakedModel and all its quads to one big int[] that stores all quads and it caches that and uses it later

stray fog
#

apparently at some point that reason was forgotten and it ended up being a Map<String, List<BakedQuad>>

#

with more steps

#

given what the renderable system became, I agree that it was pointless

#

but I don't think the original idea is pointless

#

(and I dislike the whole "I don't use it in my mod so it is useless" mentality, so if I can find the time and effort to prove it's a good idea, I plan on doing so, even if I'm the only person using it :P)

#

I should note, there's value on having standalone models regardless of me doing that or not

#

so I'm not oposing that

#

I think there's use cases for both features, and standalone baked models are a reasonable substitute even if a more explicit "this is the best option for rendering custom models in an entity or blockentity" isn't present yet

random pebble
#

Can someone define standalone models for me?

stray fog
#

models that are loaded but don't belong to a block or item

random pebble
#

Ok, yes, AE2 needs those 😅

#

Like... hard

stray fog
#

these models are used as part of a special item renderer, blockentity renderer, or entity renderer

random pebble
#

No, we actually bake them into the chunk too

stray fog
#

well ... that just adds one more data point to the list

#

:P

random pebble
#

Use case is cable parts that can also be contributed by addons, those get statically meshed into the chunk, but the "special" cable-bus model that is actually associated with the "cable bus" block cannot know all of them apriori

stray fog
#

these models are used as part of a special item renderer, blockentity renderer, entity renderer, and other places that can include custom geometry, such as GUIs and GUI Overlays, level stage renderers, and additional chunk geometry

#

there expanded

#

:P

warped sluice
#

From the gist, I don't agree with bakeAsBlockModel. It seems like a half baked solution for use cases that should instead be covered by block model types.

restive badger
spare bloom
#

Block model types are basically what bakeAsBlockModel does

warped sluice
#

In a weird way but the use cases overlap as I said, sure

warped sluice
# spare bloom So how would you implement block model types? 😛

Change the blockstates json file codec to possibly accept an object instead of the vanilla model reference string; the object can specify a "type" which works like a model loader, except that it makes a BlockStateModel.Unbaked instead of an UnbakedModel and uses a Codec instead of accepting the JsonObject directly

#

The type can be specified at the variant level or the variant list level instead if specifying at the model ref level doesn't work or the other options sound better. I need to check the code again to see which options are the most feasible.

spare bloom
#

That's also an option, but the patches might be gnarly and I am not sure it provides much benefit

#

Inlining a lot of data inside of each block state file is not great

hardy talon
#

Yeah, not a fan of overburdening the blockstate file with even more data, the files are already huge in a lot of cases. It also feels really counter-intuitive to me to either have to load the entire format yourself (or rather in practice, invoking vanilla and then un-wrapping and re-wrapping everything) with a loader specified at the root or having to potentially duplicate the same exact data across several entries instead of specifying stuff once in a model file which gets re-used by a bunch of entries in a blockstate file

warped sluice
#

Can you give me a concrete use case of bakeAsBlockModel?

hardy talon
#

Custom model loaders spitting out a model that is intended to be baked into a dynamic block model. Example:
https://github.com/XFactHD/ConnectedTextures/blob/1.21.4/src/main/java/xfacthd/contex/client/loader/ConTexLoader.java
https://github.com/XFactHD/ConnectedTextures/blob/1.21.4/src/main/java/xfacthd/contex/client/loader/UnbakedConTexModel.java
https://github.com/XFactHD/ConnectedTextures/blob/1.21.4/src/main/java/xfacthd/contex/client/model/ConTexModel.java
The example above would for example also support being baked into both a dynamic block model and a QuadCollection for an item model.

warped sluice
#

I see. I still disagree but I can't think of an alternative that I expect you would agree with.

queen elm
#

oh yea we should probably add @stiff fossil here

hardy talon
elfin junco
whole aspen
#

-# oh lord not another CTM mod... that'd make at least 4

queen elm
#

-# insert xkcd about standards here

spare bloom
hardy talon
hardy talon
spare bloom
#

I have to look at the code again. Not sure what the conditions are for a model file to be loaded into an UnbakedModel

#

In principle, once that is taken care of any model can be accessed through the Baker

hardy talon
#

Being loaded into an UnbakedModel is as simple as the file existing. The problem (which, as a side-note, is also the make-or-break for the composite loader) is the parent resolution and that requires handing the ResolvableModel to the ModelDiscovery

spare bloom
#

So the other unbaked models are just sitting there unused until something triggers parent resolution hmm

warped sluice
#

Composite models would require a lot of extensions to work as before

hardy talon
#

Yeah, the composite model is a major pain in the ass

#

With that said, I have the main sourceset compiling locally with a broken composite model and the GlStateManager errors commented out

spare bloom
#

Well, composite is dead for items anyway

#

But it's nice for blocks IMO?

hardy talon
#

The item stuff is already gone from my local copy. Getting the block part to work again is still a pain though because an UnbakedModel can no longer do parent resolution on wrapped models

spare bloom
#

Hmm what?

#

Composite model shouldn't have its submodels in the same file

hardy talon
#

Doesn't matter whether they are inlined or references to external files, they still won't participate in parent resolution because nothing tells them to. As it stands, the only things that can initiate parent resolution for referenced models are blockstate files, client item files and the WIP standalone model system

vernal parrot
#

example, how would I implement this custom model? ```json
{
"edge": {
"parent": "morered:block/red_alloy_wire_edge"
},
"line": {
"parent": "morered:block/red_alloy_wire_line"
},
"loader": "morered:wire_parts"
}

spare bloom
#

Would it be unreasonable to let parent resolution be triggered from a ModelBaker?

#

I have to check what the concurrency problems might be though

hardy talon
#

IMO yes, parent resolution happens a lot earlier

spare bloom
#

Could have a pass where we let unbaked models register additional unbaked models to load?

#

That's more or less what you did for standalone models I think

#

Not home yet so I'm just throwing ideas around 😛

hardy talon
#

What I did for standalone models is the exact same thing blockstate files and client item files do

spare bloom
#

The problem with our BlockStateModel approach is that we don't have this unbaked stage where dependencies can be queried

hardy talon
#

The primary issue I'm worried about is that the recursive resolution would most likely happen in the mapping function of a Map#computeIfAbsent() call due to the structure of the surrounding vanilla code and I'm fairly certain that that's going to throw a CME (or rather, because that code uses a FastUtil map, it would potentially break in some other nonsensical way)

spare bloom
#

Depends how we do it. I think we can be smart about it

#

If we coordinate this over all models we can keep looping until no model has extra references to resolve? 🤔

#

i.e. query models added in the last cycle repeatedly

spare bloom
#

Imagine if we added bakeAsItemModel kek

#

So I just had a thought

#

We could add BlockStateModel jsons

#

As long as they are separate from UnbakedModel jsons thonk

warped sluice
#

I had that idea yesterday and am still considering it as an actual solution, at least for Fabric

vernal parrot
#

HalfBakedModel jsons harold

spare bloom
#

You'd need a way to differentiate block state models from unbaked models in the blockstate files

stray fog
#

more intermediate json files screm

wide oak
#

FriedModel

stray fog
#

is it problematic in some way, to have the "loader" stuff in the main model json now?

spare bloom
#

Loaders are used for UnbakedModels

#

We are discussing BlockStateModels

#

Currently I added a hook for UnbakedGeometry to turn itself into a BlockStateModel, but that is a questionable approach

stray fog
#

oh I thought this was being discussed as an alternative to unbaked models

#

like, no more unbaked model loaders, you can either register a custom ItemModel loader, or a custom BlockStateModel loader

warped sluice
#

You can't really use an arbitrary block state model inside a variant anyway

spare bloom
#

Well you kinda can

stray fog
#

item models have like { "type": "whatever" } in the json object

#

can we not do the same for blockstates?

spare bloom
#

It will just be restricted to some states and/or a subset of the instances

warped sluice
#

By variant I mean the object that specifies the model, rotation, and uvlock

#

How do you uvlock a model that's already baked and that can't be inspected?

spare bloom
#

uvlock is specified in the block state file?

stray fog
#

yes

warped sluice
#

Yes

stray fog
# 
{
        "model": "minecraft:block/acacia_fence_side",
        "uvlock": true,
        "y": 90
      }
spare bloom
#

It would be in the ModelState

stray fog
warped sluice
stray fog
#

yeah my question was, can we extend that?

warped sluice
#

I was thinking replace it with an arbitrary codec

stray fog
# 
{ "type": "my:custom_loader",
"uvlock": true,
"y": 180
}

is this feasible?

warped sluice
#

I think is it

#

But Tech's point from yesterday is that allowing only that is inconvenient

stray fog
#

yeah, but those are two different problems that are being solved

hardy talon
#

I hate the idea of having to duplicate the same data over several entries in the blockstate file when that data could be specified exactly once in the model file

warped sluice
#

That's where the idea of standalone block state model files comes in

spare bloom
#

Could be block_model instead of model

#

And refer to a block state model file

warped sluice
#

I don't think that works: #1342994966405845063 message

spare bloom
#

Does the bake method not receive the model state?

stray fog
#

but the logic for model selectors makes more sense in the client item json / blockstate json

warped sluice
spare bloom
#

My biggest issues with the current system (that I implemented yesterday) are

  • dependent models
  • that a model could be used either as a BlockState model or a QuadCollection and kinda needs to support both (or using the wrong one would lead to a silent failure)
#

And of course the asymmetry with ItemModels (ask yourself why don't we just add bakeAsItemModel), but that's more of secondary concern to me

stray fog
#

there's a little part of my brain wondering if it wouldn't be worth making a separate ser of file formats, mesh definition jsons, mesh processor jsons (can select / combine meshes), and in-code mesh adapters for turning them into blockstate models and item models as needed
I don't like that part of my brain though

spare bloom
#

So abstraction for the sake of abstraction?

stray fog
#

it's just the part of my brain that doesn't like working with mojang's limitations speaking

random pebble
#

Sometimes, all we need is just a little bit more AbstractFactory 🙂

vernal parrot
#

beats working at the concrete factory

random pebble
#

Yes, or in the hellish mines of Impl

hardy talon
#

Hmm, I have a feeling that my fix for composing the root transform into the ModelState isn't quite correct 😅

2025-02-24_22.15.36.png
#

(Yes, that's a technically working composite model with an experimental solution for parent resolution)

spare bloom
#

alright I'm finally back at my PC let's see

hardy talon
#

Hmm, that's not right either kek

2025-02-24_22.23.50.png
random pebble
#

LGTM. ship it

spare bloom
#

if we wanted to patch block model types, we would simply have to patch them in Variant

#

we can add a block_model key relatively easily

#

looks a bit like a nightmare with visualEqualityGroup though

hardy talon
spare bloom
#

a new format

#

the main advantage of such a thing is that you'd be able to easily nest BlockStateModels

warped sluice
spare bloom
#

essentially MultiVariant#bakeModel would have to bake the BlockStateModel referenced by the Variant

warped sluice
#

Do you intend on patching BlockStateModel.Unbaked#bake to receive a ModelState?

spare bloom
#

could be done yeah

warped sluice
#

I don't think it's necessary

spare bloom
#

honestly I'd probably introduce a different class than BlockStateModel.Unbaked since we have to provide a full dispatch codec for it and I wouldn't expose visualEqualityGroup

#

of course, moving things out of the block state files makes matters a bit more complicated

#

@hardy talon what kind of use case do you have for blockstate files + custom block models?

#

(i.e. giving a different custom block state model to various states or parts)

#

CTM?

hardy talon
#

Yup, the CT model is the primary one

vernal parrot
#

I have a case where I have a multipart blockstate where each state gets a different vanilla-style model but I also apply a dynamic model to all states

hardy talon
# hardy talon Yup, the CT model is the primary one

However, the CT model has at most one parent which would be returned by UnbakedModel#parent() and be handled transparently by vanilla. The problems start once you have multiple dependencies like the composite model, Commobles use case or this one:
https://github.com/XFactHD/RedstoneControllerUnit/blob/1.21.4/src/main/java/io/github/xfacthd/rsctrlunit/client/model/ControllerModelLoader.java
https://github.com/XFactHD/RedstoneControllerUnit/blob/1.21.4/src/main/java/io/github/xfacthd/rsctrlunit/client/model/ControllerGeometry.java
https://github.com/XFactHD/RedstoneControllerUnit/blob/1.21.4/src/main/java/io/github/xfacthd/rsctrlunit/client/model/ControllerModel.java

spare bloom
#

yeah, a BlockStateModel.Unbaked would have no issue dealing with that

hardy talon
#

Yeah

spare bloom
#

btw, do you know why vanilla adds the coloring properties to the group key?

#

it made sense at some point but now I don't understand it

#

the properties are the same for all block states of a block, and each block has either a different MultiVariant or MultiPart instance and therefore visual equality group? harold

hardy talon
#

To be honest, the entire model group system makes little sense to me

spare bloom
#

ah right now I remember

#

it allows avoiding chunk remeshes

#

this here

image.png
#

if you change between two block states that have the same "visual equality group" no remesh is requested

#

that's why it also needs to take into account the properties that affect the color of the blocks even if the models don't use them explicitly

hardy talon
#

Makes sense

#

Fixed the root transforms (the emerald block is already broken in earlier versions, still need to figure that out - I think Pepper mentioned something related to that at some point).

2025-02-24_22.50.32.png
spare bloom
#

good 😄

#

how do you mark dependencies for the composite model?

hardy talon
# 
Subject: [PATCH] ModelDependencies
---
Index: projects/neoforge/src/main/java/net/minecraft/client/resources/model/ModelDiscovery.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/projects/neoforge/src/main/java/net/minecraft/client/resources/model/ModelDiscovery.java b/projects/neoforge/src/main/java/net/minecraft/client/resources/model/ModelDiscovery.java
--- a/projects/neoforge/src/main/java/net/minecraft/client/resources/model/ModelDiscovery.java    (revision d2f29c0063794941428c3ee647b5ab960856279d)
+++ b/projects/neoforge/src/main/java/net/minecraft/client/resources/model/ModelDiscovery.java    (date 1740428360518)
@@ -63,6 +63,8 @@
         ModelDiscovery.ModelWrapper modeldiscovery$modelwrapper = new ModelDiscovery.ModelWrapper(p_405734_, p_404997_, flag);
         if (!flag) {
             this.parentDiscoveryQueue.add(modeldiscovery$modelwrapper);
+        } else if (p_404997_ instanceof ResolvableModel resolvableModel) {
+            resolvableModel.resolveDependencies(this.resolver);
         }
 
         return modeldiscovery$modelwrapper;
@@ -113,6 +115,9 @@
             } else {
                 p_405689_.add(modeldiscovery$modelwrapper);
             }
+            if (modeldiscovery$modelwrapper.wrapped instanceof ResolvableModel resolvableModel) {
+                resolvableModel.resolveDependencies(this.resolver);
+            }
         }
     }
#

The composite model then implements ResolvableModel:

@Override
public void resolveDependencies(ResolvableModel.Resolver resolver) {
    for (Either<ResourceLocation, UnbakedModel> child : children.values()) {
        child.ifLeft(resolver::markDependency);//.ifRight(model -> model.resolveDependencies(resolver));
    }
}

The way this works of course means that inline models in a composite model are dead once again (sorry Soaryn, deal with it)

spare bloom
#

is there anything in vanilla that prevents infinite parent cycles?

warped sluice
#

Yes

hardy talon
#

ModelDiscovery#resolve() has a check for it

spare bloom
#

ah yes I see

#

say we introduce block state model types and we make composite models a block state model

#

how will that work with part visibility?

hardy talon
#

It would have to load the part visibility from the blockstate model and ignore the one specified in the "standard" model

spare bloom
#

these separate block and item models kinda suck

#

we should really add bakeAsItemModel 😛

#

well at least item models can query additional properties from the ResolvedModel now

hardy talon
spare bloom
spare bloom
#

WTF is RegistryContextSwapper

pulsar lantern
#

it swaps a registry context

hardy talon
# spare bloom WTF is `RegistryContextSwapper`

As far as I understand, it's a cursed way of switching out the dynamic registries under the feet of item model properties without having to reload resources when connecting to a server such that the item model properties can check data components which hold dynamic registry entries

spare bloom
#

that sounds about right

#

so I guess we have two options now:

  1. roll with what you have when it comes to model dependencies and accept that unbaked models can't be nested in the same file
  2. introduce block state model types, move composite to it, remove visible parts additional property, don't allow adding additional unbakedmodel dependencies
#

(we don't have to remove visible parts but I think it makes more sense if we move composite to a block state model type)

hardy talon
spare bloom
#

yeah you are right; so we'd have to remove the visible parts additional property

#

well except that obj uses it? blergh

hardy talon
#

Yup

spare bloom
#

we would be removing visibility from the block state model format entirely

#

since it can't be overriden, it wouldn't make sense to keep it

#

in fact, similarly to how CompositeModel.Unbaked only has a List<ItemModel.Unbaked> models, the block state model equivalent should only have a List<BlockStateModel.Unbaked> models

#

you could have it so that

{ "model": "xxx" }

selects an unbaked model whereas

{ "type": "xxx" }

selects a block state model

#

(so a model key would actually be how you encode an "unbaked `SimpleModelWrapper")

#

so a composite model would be something like

{
  "type": "neoforge:composite",
  "models": [
    { "type": "ctm:whatever", /* extra data */ },
    { "model": "mymod:additional_piece" }
  ]
}
#

OTOH since you can create arbitrarily many model files, there is no need to inline the definitions of nested models in the same file, it's just convenient sometimes

#

alright I've made my decision: we keep bakeAsBlockModel

hardy talon
#

I may have a slightly cursed idea for how to make the composite loader work exactly as before

spare bloom
#

do tell

#

allow models to inject arbitrary (already parsed) UnbakedModels? (along with a ResourceLocation to retrieve them later of course)

hardy talon
#

I should fairly trivially be able to construct my own ResolvedModel for the inline model. With the experiment from earlier, I can mark the inline model's parent as a dependency and then retrieve that parent model as a ResolvedModel from the baker

spare bloom
#

yeah that should work lol

#

that's probably more or less how it worked in 1.21.4 anyway, since the UnbakedModel also acted as the ResolvedModel

hardy talon
#

Yup

spare bloom
#

ok why not

#

btw can you push what you have already? I think we can always make UnbakedModel implement ResolvableModel

#

it might make sense to add an extension method to the Baker or something to construct a ResolvedModel from an UnbakedModel

#

e.g. resolveFreestanding(UnbakedModel model, Supplier<String> debugName)

#

not to be confused with standalone kek

hardy talon
spare bloom
#

well, I would suggest that we default-implement markDependency for UnbakedModels

#

implementing a magic interface is not great API design IMO

hardy talon
#

Fair

#

Yup, works. Left is with references, right is inline

2025-02-24_23.47.13.png
#

Pushed. The way it's currently implemented means that items referencing a composite model will render nothing. If we do decide to introduce a bakeAsItemModel() extension, then this could be fixed by converting our composite model into the vanilla composite model

spare bloom
#

I wonder if we should at least make the normal bake error out when it is called

hardy talon
#

I had that thought as well, yeah

spare bloom
#

if we throw it will probably fail the whole reload though

hardy talon
#

It shouldn't, ModelBakery#bakeModels() try...catches model baking on a per-BlockStateModel and per-ClientItem basis

spare bloom
#

oh yeah, nice

hardy talon
#

Btw, for standalone models: should I push what I have for that currently (i.e. the patch I linked last night) and we potentially simplify it later or do you want to discuss it further first?

spare bloom
#

I think you can push it

hardy talon
#

👍

spare bloom
#

BTW, how much thought did you give to the cycle-finding algorithm and nested dependencies for UnbakedModel#resolveDependencies?

hardy talon
#

Just a tiny bit. It should work fine for most cases but there are likely cases where a dependency from UnbakedModel#resolveDependencies() can bypass it

spare bloom
#

nested computeIfAbsent calls seem to be a problem, no?

hardy talon
#

That's the one thing I'm not sure about. At least in my experiment with the composite loader it worked fine

spare bloom
#

it's only a problem if the table gets resized and/or the bucket is taken by something else

hardy talon
#

Yeah, that makes sense. Worst case we change ModelDiscovery#getOrCreateModel() to first do a containsKey() check and then do the recursive resolution on the return value of computeIfAbsent() if the key wasn't previously present

spare bloom
#

yeah

#

and regarding cycles, what can happen is that cycles are not detected in some cases as far as I can tell

hardy talon
#

Yeah, I would suspect that a composite model referencing itself as a part would be missed

spare bloom
#

btw what is the point of the resolveDependencies call in discoverDependencies?

#

shouldn't it already be covered by the call to getOrCreateModel?

hardy talon
#

Both getOrCreateModel() and discoverDependencies() calling resolveDependencies() is due to the distinction between root models and non-root models (the latter being ones that specify a parent). In hindsight I'm not sure that's even needed. I should be able to test that fairly easily though, one second

#

I'm gonna push the standalone models first though

spare bloom
#

another way to see is that there is a single instantiation site of ModelWrapper

hardy talon
#

That's the last three rejects from 25w08a gone btw

#

That's true, yeah

spare bloom
#

cursed formatting 😛

image.png
hardy talon
#

😄

spare bloom
#

not a fan but I'm not going to change it 🤷

#

another thought: we could probably fix the cycle detection if we had Set<ResourceLocation> getAdditionalDependencies() instead of void resolveDependencies(Resolver resolver)

hardy talon
#

That's true, yeah

spare bloom
#

basically we'd change the definition of root models to models that have no parent and no additional dependency

#

makes propagateValidity a bit annoying though

hardy talon
#

The ModelDiscovery.ModelWrapper will then also need a Set<ResolvedModel> additionalDependencies and the non-null parent check in discoverDependencies() needs to be relaxed

#

This is an interesting view kek

2025-02-25_00.26.07.png
spare bloom
#

WTF 😄

spare bloom
#

it would be a Map<ResourceLocation, ModelWrapper> I think

hardy talon
# spare bloom WTF 😄

The test run still crashes when trying to create a world, so I had to test somewhere else and in that environment it was easiest to replace vanilla models 😄

spare bloom
#

sure sure

#

😛

hardy talon
#

Presenting somewhere else kek

2025-02-25_00.31.25.png
#

Ok, moved the recursive dependency resolution to a single place and changed getOrCreateModel() to avoid issues with doing the resolution within the computeIfAbsent() call

spare bloom
#

lol I also did that already kek

hardy talon
#

lmao

spare bloom
#

slightly differently though

hardy talon
#

What does your solution look like?

spare bloom
hardy talon
#

Heh, funnily enough, I was thinking about doing that right as I pushed it kek

#

Feel free to change it

spare bloom
#

alright, changed 😄

hardy talon
#

👌

spare bloom
#

I don't think that we really need to care about cycles

hardy talon
#

We can always come back to it if it turns out to be an actual issue

spare bloom
#

yeah, for example by passing a custom resolver or sth

#

now standalone models

hardy talon
spare bloom
#

I'm really not a fan of the Supplier<UnbakedStandaloneModel<T>> modelSupplier

hardy talon
#

It really begs the question what the use case of providing a custom ResolvableModel for this is. If we decide that a standalone model should only have a single dependency, then we can simplify the key to StandaloneModelKey<T>(ResourceLocation modelId, StandaloneModelBaker<T> baker), store the registered keys as a set before baking, change the dependency resolution to standaloneModels.forEach(key -> modeldiscovery.addRoot(resolver -> resolver.markDependency(key.modelId()))); and invoke the custom baker directly in StandaloneModelLoader.bake()

spare bloom
#

after all, you can always do arbitrary magic with model loaders and additional dependencies

hardy talon
#

Yup

spare bloom
#

in any case what I had in mind was StandaloneModelKey<T>(ResourceLocation name)

#

and you'd register the key + the baker to the event

#

with that design we can also remove the StandaloneUnbakedModel for now and add it back later if there is demand

hardy talon
#

Any particular reason to keep the baker outside the key?

spare bloom
#

the fact that it's a key? 😛

hardy talon
#

Fair enough 😄

spare bloom
#

do you want to do it? should I?

hardy talon
#

I can do it

spare bloom
#

hmm, should we ensure that no two models share the same ID?

#

in principle multiple mods may want to load the same model

hardy talon
#

Yeah, two keys could realistically point at the same model file

spare bloom
#

yeah

#

we should document that, it's not obvious that the keys are compared by identity

#

btw, it's also worth considering that the event is called RegisterAdditional but the models are called "standalone"

hardy talon
#

I can rename the event if you want

spare bloom
#

idk if we should

#

probably?

hardy talon
hardy talon
# spare bloom probably?

I think it would make sense for consistency. Standalone models are a very distinct thing now, even more so than in 21.4

spare bloom
#

stupid question: why is it a record?

hardy talon
#

Because why not, that's it 😅

spare bloom
#

if we need identity it might be better to make it a simple final class

hardy talon
#

Yeah, that's what I just did

#

There we go, custom unbaked model gone

#

Now the only remaining things are the GlStateManager compile errors, the test framework and the tests and two fluid-related rejects

spare bloom
#

added a little bit of docs

#

I'm off for today 😄

hardy talon
#

Yup, me too

spare bloom
#

is it time for the users of the system to fix it? 😄

hardy talon
modern vapor
#

we need a willing sacri-- I mean, volunteer to fix fluids

queen elm
queen elm
#

ok only shader stuff left in testframework

hardy talon
#

@spare bloom I quickly ported my CT implementation last night and I'm currently fixing the model test mods. I'm considering (re)introducing a couple things:

  • DelegateUnbakedModel: useful for models that load and bake a single inline model as they need to forward all of them (applies to my CT mod and several of the tests)
  • DelegateBlockStateModel: nice to have since we introduce several more methods to BlockStateModel which require forwarding to wrapped models (was previously vanilla's DelegateBakedModel)
  • The helper you mentioned yesterday for getting a ResolvedModel from an inline UnbakedModel to ModelBaker: basically required for any unbaked model that wraps one or more inline models (applies to my CT mod, several of the tests and the composite model)
    Any comments or objections?
spare bloom
#

No objections 🙂

#

Regarding the third point, I guess make sure to document that the dependency has to be marked in the resolver earlier

hardy talon
#

That's the thing: it doesn't necessarily have to be, it depends on the case. In the composite model's case it does have to be marked as a dependency because there are multiple inline models and the outer model doesn't forward anything from any of them. If you have a single inner model though and forward that model's parent as the outer model's parent, then the dependency resolution is taken care of for you

spare bloom
#

True

spare bloom
#

IMO call it delegate not parent

#

Parent is confusing for unbaked models

#

Also I would suggest ModelBakerExtension, i.e. without the leading I

hardy talon
#

I called it IModelBakerExtension because the previously existing extension interface was called that. I can rename it if you want

hardy talon
stiff fossil
#

nice design you got there
would be a shame is somebody... merged some additional refactors

warped sluice
#

Refactor it all

stray fog
hearty nova
#

xfact, if models changed a lot, please help me with the connected texture model again kek

hardy talon
#

Hehehe, will do 😄

rocky vector
#

the refactors will continue until morale improves

warped sluice
#

Continue refactors until no mod loader needs to make any extensions to the system

stiff fossil
#

"just one refactor more and it will be alll perfect, trust me"

stray fog
#

boq wants to refactor our brains so they stop having too-high expectations

spare bloom
#

You refactor and we redesign, not problem 😛

spare bloom
whole aspen
#

Just use a lowercase i prefix instead, annoy both camps

#

-# (please don't)

bronze gorge
#

oh no, not this again harold

blissful jacinth
wide oak
#

Block models got the item treatment it seems

# 
>         this.singleThreadRandom.setSeed(blockState.getSeed(blockPos));
>         blockStateModel.collectParts(this.singleThreadRandom, this.singleThreadPartList);
>         this.modelRenderer.tesselateBlock(blockAndTintGetter, this.singleThreadPartList, blockState, blockPos, poseStack, vertexConsumer, true, OverlayTexture.NO_OVERLAY);
warped sluice
#

Aha they did switch to a collector approach like FRAPI uses

#

They are just collecting parts instead of quads directly

wide oak
#

yep

#

and then that has getQuads(Direction) (and ambient occlusion)

warped sluice
#

It doesn't matter too much as FRAPI can still patch emitBlockQuads onto BlockStateModel, but it's annoying that the vanilla model renderer doesn't accept the model directly

#

I suppose an alternative can be added for that too

warped sluice
wide oak
#

List

warped sluice
#

I mean the type that has that method not its return type

wide oak
#

uh

#

I don't know what you mean

#

oh

#

yes

#

SimpleModelWrapper holds a QuadCollection

warped sluice
#

I know about the SingleModelWrapper

#

The part being a QuadCollection makes sense

#

Well for vanilla this is a great change and maybe it gives Neo another reason to reconsider its getQuads

spare bloom
#

Models changed again? concern

warped sluice
#

Yes

stiff fossil
#

as was foretold

warped sluice
#

Boq basically said this would happen

stiff fossil
#

IT WAS SAID

#

anyway, probably done for now

rocky vector
#

the 1.8 paradigm of baked models is finally gone

warped sluice
#

Will Neo adapt?

last wave
#

To what?

violet elm
rocky vector
#

won't happen

rocky vector
warped sluice
rocky vector
#

the whole extended getQuads thing can probably be replaced by just extending the part collection logic

warped sluice
#

The extended getQuads is part of that paradigm

last wave
last wave
#

The question I have is where we will extend

rocky vector
#

IMO extending part collection makes way more sense

last wave
#

I would think it would now be on the part collection

last wave
warped sluice
rocky vector
#

as a part collection is now a collection of quads, and the configuration for those quads

last wave
#

Yeah

warped sluice
#

It's not the full configuration

#

Neo does not store the RenderType like other quad properties

last wave
#

IMHO RenderType should be part of the part collection

#

Not directly on the Quad....

#

But maybe not

warped sluice
#

Then again the model AO is also a separate property, but Neo's AO control is per-quad

rocky vector
#

It's more memory efficient to store it on the part collection than per-quad

stiff fossil
#

Anyway, in vanilla BlockStateModel is everything that does (BlockState, Random) -> something, and is unconcerned with rendering.
I was even considering just making parts generic
The renderable bits are just in Variant

wide oak
#

it makes more sense logically

warped sluice
#

Really what I want to avoid is having whatever extension method be called more than once

wide oak
#

BlockStatePart is essentially sub-models(?)

#

they should store state

#

quads should not store state

spare bloom
violet elm
#

How do item models and overrides work

rocky vector
#

my thought is we would give world/ModelData context to collectParts

wide oak
#

that seems reasonable

warped sluice
#

That seems like the simplest solution

last wave
#

Yep

#

It also seems to line up most with the general idea of the model system

warped sluice
#

A RenderType parameter will not allow that

last wave
#

We really need a rendertype though

#

Especially for things that mimick stuff

stiff fossil
warped sluice
last wave
rocky vector
#

I think we just move the render type field from SimpleBakedModel to BlockModelPart

last wave
#

And I am not sure if that is worth it

#

The call cost to that method are most likely much lower then the iteration costs of the relevant parts that get populated.....

#

But something I would need to see the numbers on

violet elm
last wave
#

I mean maybe....

#

I don't know

#

It is difficult to know what rendertypes are needed on a given model from the outside, unless we expose that information somehow (aka getRenderTypes on BlockStateModel)

#

So that mimicking models can pass that through

#

or somehow pipe that information around

#

And retrieve the right quads for the right render types from their respective inner models

warped sluice
last wave
#

The BlockState == RenderType bound has always been a bit tricky to handle

last wave
#

But difficult to handle

spare bloom
#

We just move the extension methods to BlockModelPart, no?

last wave
#

Because the level renderer needs to filter out all the parts it does not need in that stage

#

Which seems like a massive overhead/waste

rocky vector
last wave
last wave
warped sluice
rocky vector
#

Sure we can

last wave
#

Because for one, we fire an event per rendertype, and for two that is simply not possible with things like transparent etc

rocky vector
#

I'm referring to the logic in SectionCompiler

last wave
#

At least not in the way the level renderer was written in the past

spare bloom
last wave
last wave
#

Sorry part count

#

Not quad count

spare bloom
#

It doesn't. It depends on the part count and it probably is quite low in practice, especially compared to the cost of processing the quads

last wave
#

Yeah as I said

spare bloom
#

Ah yes part count

last wave
#

Wrong word

#

You might be right.....

#

It might be worth thinking about for a bit

#

And then go for this route none the less

#

As it seems to be the easiest option IMHO

#

Potentially with embeddeds idea to just render the entire model at once to all buffers in the compiler

spare bloom
#

It's not really different from a model with many submodels in the previous system

last wave
#

Which is why I feel like it could work

#

But it is a "feeling"

spare bloom
last wave
#

And you know

#

Those can be wrong

last wave
#

IMHO We could do that for now

#
  • Expose getRenderTypes on BlockStateModel (and maybe ItemModel?)
  • Rewrite SectionCompiler to get all rendertypes
#

The other alternative:

#
  • Don't expose the getRenderTypes()
spare bloom
#

getRenderTypes is not needed for item models

last wave
#
  • Adapt the SectionCompiler to not give a shit and handle it internally
spare bloom
#

(you already choose the render type for each item layer)

last wave
#

Yeah true

wide oak
#

can't each part just have one RenderType

spare bloom
wide oak
#

it just makes more logical sense to store state more statically

stray fog
#

ah I see boq's warning was real. tl;dr?

wide oak
#

where state only changes between parts

last wave
# wide oak Why does it need to be a list like that?

That is what I am trying to determine.....

Right now we can trivially say, as a wrapping model, in what rendertypes we render by asking the inner model.

That makes specific speed ups possible, by handling the render type variations properly.

The section compiler implementation is basically vanilla and that makes maintanance easy

#

The counter point is that if it is stored within the part then that makes models easier to implement but more annoying to optimize (as you have less data available to you)

And it would make the section compiler harder to maintain, as it now contains a significant patch to handle all the states at once, or hit a "massive" speed bump because it iterates over all the parts over and over again trying to find the parts that match

#

And we could potentially do both

#

Where getRenderTypes() should be considered a hint

warped sluice
spare bloom
#

Ok I see what was done. For sure requires a bit of thinking on our end

#

Keeping the parts "dumb" matches item models best

last wave
# warped sluice Why is it iterating multiple times?

The section compiler (as far as I read it right now) just grabs the render type from the static map, and considers all parts to have that type, and passes it to the right buffer, no?

We patched that so that it would iterate over all render types that the model told it to, and then call getQuads for that, if the SC was currently doing that RT.

Now that is not directly possible anymore.
There are now two options.

Either we keep the SC behaviour that it is responsible for one RT at a time, add RT to the Part, and then filter out all parts that do not have the RT at all. Which is wastefull.

warped sluice
#

The third option is getting the correct buffer for each part as the iteration happens a single time

last wave
#

Which is not inline with the design mojang has

#

As Tech already points out

#

Rigth now from reading the BlockStateModel and ItemModel code

#

I would prefer the "Dumb" solution

#

Where parts are dump and just carriers of the Quad+Config data

#

And pass, world/modeldata/rendertype to the getParts call

#

However there is an argument that rendertype is part of the part configuration

#

And so should reside there.......

warped sluice
last wave
#

Which is likely your argument no, pepper?

warped sluice
last wave
#

Maybe

#

It would be possible......

warped sluice
#

It is at least one of the reasons. Another is that removing getRenderTypes simplifies the work that model implementations have to do.

#

An analogous reason for why I prefer FRAPI instead of Neo's existing model extensions is that I only need to override a single method instead of 4 different methods

last wave
#

I can understand that completely

#

My biggest arguement against the FRAPI style is that it is not how Vanilla does it, and for many modders, that is generally the best resource they have on how to do and use stuff in the game

stray fog
#

I'm trying to catch up but I'm having trouble finding how the game now resolves blockstates into models

last wave
stray fog
#

oh BlockModelDefinition#instantiate

#

wow that's horrible

last wave
#

Yep

#

It is not great

#

But it works

stray fog
#

so models are now baked combinatorically? I can't see a cache at a glance

#

oh I assume collectParts is being called when processing chunks?

#

gah why does my scarce social life have to coincide with mojang's snapshot cycle so often

last wave
rocky vector
#

I don't think a section compiler patch would be very hard to maintain

last wave
#

We already have one to limit it to a given RT

spare bloom
last wave
#

Yep

#

Balanying a million scales at once (so to speak)

spare bloom
#

You saw the extensive discussion 2 days ago where we considered whether to implement modded unbaked block state models

last wave
#

We sadly don't have the luxury to rewrite systems every version, so API designs are carefully considered for all kinds of aspects.

Including but not limited to similarity with vanillas design, as we strive to follow it as much as we can

#

Even if that limits us sometimes

warped sluice
last wave
#

Or at least more weight then would seem to be obvious at first, especially since mojang it self does not consider our cases (nor should they) when they design systems

warped sluice
stray fog
#

I haven't had time to properly acquaint myself with the code (away from home again), but I see it as a tradeoff between maintainability (less patch points is better), usability (less complexity is better), and usefulness (more use cases is better)

last wave
warped sluice
#

Yes

last wave
#

And we might alter our opinion on bakeAsBlockModel as we now finalize the approach for this

stray fog
#

right now my worry is dynamic block models, but I assume Orion already has block bits in mind

last wave
#

We might not

#

Depends

last wave
#

Or it is passed into the getParts call

stray fog
#

as I said, I haven't had time to catch up with changes, so I don't know if producing dynamic quad lists is part of the problem or not

spare bloom
#

The problem with these refactors is usually not whether something is still possible but rather what the best way to make it possible is

last wave
#

Yep

#

I keep a hawk eye on whether dynamic models are possible with this

#

But given that like out of the 6ish rendering people we have on the team

#

Each and every one of them has some form of dynamic model

#

I am not really worried that anybody forgets it

last wave
spare bloom
#

These model parts are actually quite great because they solve a big UX issue with complex models which is the composition of multiple submodels

last wave
spare bloom
#

As a somewhat unrelated note, we should probably think about introducing some sort of MutableBakedQuad helper. That and composition are IMO the biggest selling points of FRAPI

stray fog
#

what is the purpose of that?

#

vs the BakedQuadBuilder we have/had already

stiff fossil
last wave
#

Introspecting and on the fly alteration without memory overhead

stray fog
#

ah so this would be for a "quad transformer" thing

#

to avoid constructing new quads

last wave
#

Yeah basically

stiff fossil
#

which, on vanilla brought a puny save of about 10MB, but it's still something

warped sluice
# last wave Dynamic models should not care whether the part has the rendertype

While the approaches are functionally equivalent it is interesting to think that if parts are collected exactly once per model, a significant advantage of getModelData is lost. It does precomputation using world data so that computation does not have to be redone every time the method to receive geometry is called, but if it is now only called once, then precomputation is no longer necessary. I don't deny that ModelData has other uses but I am just saying that if this advantage does disappear then perhaps it merits a rethinking of how ModelData works.

last wave
#

At least not on the meshing thread

spare bloom
#

Pepper means the getModelData method that receives the block and tint getter

warped sluice
#

It has a world parameter, so the model does have access to the world

spare bloom
#

(level in the chunk meshing copy sense)

warped sluice
#

In vanilla it's the RenderChunkRegion

last wave
#

Okey, I missed when that was added

#

Because the whole idea of ModelData

#

The core problem it is supposed to prevent

#

Is people accessing the world

#

Because like it or not, that caused so many issues in the past

#

With people accessing their BE

#

Or any other structure in the world that was not thread safe

#

And caused so many issues

spare bloom
#

I can't believe we're having this discussion again 😦

warped sluice
#

I'm confused because it was added a really long time ago

spare bloom
#

The existence of this function might not be obvious at first but it's the only way to do CTM

warped sluice
#

Right, it's an absolute requirement

#

If BE access is problematic, patch the BE getter to throw or something

spare bloom
#

(And CTM is just a simple example. In general any world state access that is not BE access should potentially use it)

hardy talon
# last wave Okey, I missed when that was added

That method has existed since the inception of ModelData. It's the only sensible way to allow things like connected textures without providing the level directly in getQuads(). Accessing the level in it is perfectly safe as the provided level is specifically designed to provide safe access to world state on meshing threads. Accessing a BE's internal state is the only case where you would realistically run into issues

vernal parrot
#

doesn't getBlockEntity return null if you call it offthread anyway

last wave
#

That we can collapse it away into the parts

#

I think.

#

Well maybe not

hardy talon
last wave
#

ModelData can be used as a cache key....

#

The point is: Accessing a BE from a custom model == bad

#

It always has been

#

But yeah we need a way for custom models to access the world to do CTM etc

spare bloom
hardy talon
spare bloom
#

Some mods such as AE2 shove the ChunkRenderRegion inside of it

last wave
hardy talon
#

I'm well aware that this was regularly done wrong (ignoring the fact that the pre-1.19 ModelDataManager required it when mods with fake levels came into play). In fact, I could link you an example where it's still being done and actively causes issues.

last wave
spare bloom
#

we discussed it and it might be a good idea

warped sluice
#

From what I remember a good choice is for the model to have a dedicated method that returns an arbitrary Object key given the same context that geometry retrieval receives

spare bloom
#

yup

#

that's exactly what I remember

last wave
#

Yep me too

#

If we are already going through this with a sledge hammer

#

Should we do it properly

#

And tackle these two aspects?

warped sluice
#

I would say yes

#

The API design is dead simple if we agree on it

last wave
#

For the cache key

#

Yes

#

For the thread safe world access

#

I am not sure

warped sluice
#

It is already thread safe

#

The only solution for BE access is to patch the vanilla method to throw

spare bloom
#

it should be done separately from the other changes IMO

last wave
#

But lets just at least theoretically discuss it now

last wave
#

Would that be a problem for any known model implementation currently?

warped sluice
#

Continuity harold

#

Yes, I access the BE to get its custom name

spare bloom
#

in principle if the BE is careful to be thread safe it's ok

warped sluice
#

I cannot use Fabric render data/Neo ModelData for that as I cannot mixin to every single BE, but even if I could, mixins should not be necessary to achieve a use case

last wave
spare bloom
#

I'm not interested in "modders are stupid" discussions

last wave
#

Except I am

#

Because we are supposed to properly design an API

#

That prevents people from shooting themselves and many others in the foot

#

And as XFact also pointed out, that foot shooting actually still happens

#

Eventhough it was the original design goal ModelData was suppose to fix, that that particular detail somehow got lost on the way, does not make it less relevant

#

Because if you leave that design part away, then modeldata has virtually no use, as you can just access the world, and through it the BE

#

And grab your data

spare bloom
#

what I mean is that we all know (or so I hope) that this is part of the design, and it is pointless to bring it up again

warped sluice
#

Yes

#

Assuming you mean BlockEntity.getModelData and BlockGetter.getModelData

Whether it needs to be a map is debatable but those methods must exist regardless

wide oak
# last wave Except I am

if you're going to make rules, you shouldn't be worrying about what happens when they're broken

#

that's why you made the rules

last wave
#

Sorry @wide oak But this feels entirely of topic or not matching the conversation. If you want to contribute as a sodium maintainer / iris maintainer on how to make custom model access to BE data thread safe, feel free to do so, but that comment is not helping any body forward

long aurora
#

Interesting

last wave
# spare bloom what I mean is that we all know (or so I hope) that this is part of the design, ...

Everybody in here knows, hence me saying that Pepper likely does his stinking best to make the name retrieval as safe as possible, but likely can't guarantee it fully (because he simply does not control the underlying implementation for 100%)

However that does not and should never mean, that we design an API in such a way that it requires reading a bunch of documentation and its minutia to create a simple model loader that accesses your data on your own BE.

#

It is not common knowledge for the modder that getModelData in the model it self is called from the meshing thread

spare bloom
#

in fact I added Called, typically on a meshing worker thread, to capture additional model data needed by the model. to the javadoc 2 days ago

#

previously there was no javadoc whatsoever 😛

last wave
#

Well that is a start..... And kind of makes my point at the same time

warped sluice
#

By the way, patching the BE getter to always throw won't work, because vanilla itself sometimes uses it in a way that is not necessarily thread safe

last wave
#

Yeah

#

Which is a whole problem in and of it self

long aurora
#

I've been skimming through the chat trying to get a rough idea of what model changes happened, and they seem pretty conveniently in line with what my instance-based geometry baking stuff in Gander is doing. In particular, the BlockModelPart stuff is more or less the API I was looking for

spare bloom
#

it's not instance-based and not really static

long aurora
#

The instancing stuff is just how I implemented it; the querying and baking of BlockModelPart-s seems to be pretty much exactly what I had in mind

#

Specifically, you collect all of the BlockModelPart-s once and bake them all, it appears

#

then the baked models just refer to those BlockModelPart-s

warped sluice
#

Dynamic parts from custom models are possible

#

The object instance may be different every time you call the method

long aurora
#

That is true, but it seems like the intent here is that the BlockModelPart instances could be shared

#

Or, at the very least, the actual mesh data behind each BlockModelPart could be shared

last wave
long aurora
#

and BlockModelPart itself could be a "this mesh data with this configuration" object

last wave
#

The idea is that the model can be shared

#

But the parts can not

#

They are still heavily specific to the context

#

And are fully dynamic

long aurora
#

The code I'm reading doesn't seem to suggest that at all

vernal parrot
#

why do we need BlockEntity#getModelData?

#

IIRC I just stick my model data in savedata and then have the other getModelData grab that

random pebble
#

wat

last wave
#

And that data is actually cached these days in the modeldatamanager

#

Only updated when need be

#

So that the model in and of it self does not need to touch potentially unsafe for multi-threading data in your BE

random pebble
#

you need a way to snapshot arbitrary world-state to safely use it off-thread

#

Whether that should be code living IN the block entity

last wave
#

That is indeed the concept of model data

#

But right now that is not really working