1.21.5+ Models

[random pebble]

or code being closer to the model -> debatable

[last wave]

Agreed shartte

[vernal parrot]

ah nvm I did refactor it to use BlockEntity#getModelData at some point

what if there's no blockentity though, where else would you create model data?

[last wave]

You get the empty model data in the model methods

[warped sluice]

It is EMPTY in that case

[vernal parrot]

no I mean, where would I add model data dependent on world context

[last wave]

That depends

In reality it should be in the block entity

[vernal parrot]

right but what if there's no blockentity

[last wave]

That is what I am getting to

Once second

If you model requires model data from the BE, you F'ed up........

[random pebble]

ahem

😄

[last wave]

However if you have a model that does not require data from a BE (like a CTM model, that only needs to know the neighbor)

Then you can use the getModelData() method in the model it self

But you need to care with accessing potentially unsafe data

[last wave]

I know it is a bit harsh

[random pebble]

you mean if a model got selected to be rendered

even though the BE it requires data from

[last wave]

Yes

[random pebble]

is not present?

[last wave]

Correct

[random pebble]

then yes

that sounds far more sensible 😛

but you still need to be robust

[last wave]

Yes

100%

You can still break it

But it is a lot harder

Then randomly accessing stuff

[vernal parrot]

okay, so if my model data is based on data that's being stored somewhere that's not threadsafe, I basically have to give the block a blockentity and have the blockentity provide it via getModelData?

[hardy talon]

Yes

[last wave]

Yes

The problem are the exception cases

[vernal parrot]

like "model data is based on savedata"

[last wave]

Namely models that need world access to work....

But not the be data to be specific

Like connected textures

Etc

[long aurora]

Mkay, so looking at this code, what we really need is some way to switch/alternate/etc between a BlockState and a ModelData, because with the system as of 25w09a, both are effectively the same data, just passed in incompatible ways

The idea being that if a block has a BlockEntity it would instead dispatch based on ModelData using the same APIs as a static block would using BlockState

[last wave]

No not necessarily

That is the problem

It might be on both

Or on none

Or the model might be wrapped via the api

And require world access

But not really model data access

[vernal parrot]

what if you have a multipart model that mixes static blockstate-based models with dynamic modeldata-based models

[long aurora]

if it dispatches on none, it would use the SingleVariant model

[last wave]

That is fine. But model data can and is used for dynamic model structures

[long aurora]

Yes, that's what I'm saying

[last wave]

Where precomputation for the dispatching logic is not possible

[long aurora]

I'm just saying we would dispatch it using the same APIs as BlockState

You don't need to precompute the "dispatching logic"?

[warped sluice]

What problem are you trying to solve?

[spare bloom]

ah! I was asking myself the same question 😄

[warped sluice]

[spare bloom]

some discussions are more serious than others hehe

[long aurora]

What I'm suggesting is that there would be a 1:1 correlation between any block and a model, and that model would take the (BlockState | ModelData) and return the right parts

If it, itself, dispatches to multiple models, that (BlockState | ModelData) would need to contain the "nested" data too, as it does currently

(Or, it could hard-code it in the collectParts body, as an example)

[last wave]

That would be the case if the model data would solely be supplied by the BE

[long aurora]

The idea is that a BlockState would be cached when the models are baked, because the model bakery knows about block states

[last wave]

However as you can see from the discussion above about CTM that is simply not the case

[long aurora]

But it would say "I don't know about ModelData" and then let it get passed along at runtime

[last wave]

To what though?

You still need something that interprets the model data

[long aurora]

The model, in the collectParts method

[last wave]

And you still need to deal with the fact that models might need access to data not from the BE

[last wave]

But what model?

[long aurora]

the model for the block

[last wave]

So it would still dispatch on the blockstate?

[long aurora]

Yesn't. It dispatches based on ModelResourceLocation, which itself is based on the registered blocks in BuiltInRegistries.BLOCK

Specifically in ModelManager#createBlockStateToModelDispatch

[last wave]

That does not work

[hardy talon]

MRL no longer exists

[long aurora]

Yeah I'm looking at older code, admittedly

[last wave]

I understand the idea

But the concept simply falls short

In many ways

[long aurora]

I couldn't easily find the ModelManager code so I looked at the existing release code

[last wave]

It does not deal with the dispatching logic properly

And it also does not deal with the fact that models in and of themselves want access

[long aurora]

It totally does

[last wave]

How?

[long aurora]

Mkay, so. I found the actual code and I'm seeing a little more of the issue now.

It looks like that based on how this is structured, the (BlockState | ModelData) union/abstraction would be incredibly expensive to implement: the idea was that you'd have a Map<ResourceKey<Block>, BlockModel>, where there would be a 1:1 mapping of items in BuiltInRegistries.BLOCK to a BlockModel, a new type which would be a parent of BlockStateModel (indicating that it dispatches on BlockState) or ModelDataModel (indicating that it dispatches on ModelData). Because the system already knows about BlockState, it could compute that lookup ahead-of-time as an optimization, but because it doesn't know about ModelData it would fall-back to dispatching at runtime (that is, a patch somewhere in ModelBlockRenderer). Part of the idea was that the two systems could nest/wrap each other cleanly, so when collectParts was called, it could correctly traverse the layers of wrapping as necessary, using the "at runtime" knowledge baked into the system for ModelData

It seems like, in this case, the best approach would be to just add ModelData as a parameter to collectParts

[rocky vector]

Did we reach a conclusion on whether models will still get world context or not?

[warped sluice]

I don't see how you can remove that without sacrificing a ton of use cases or why that's being considered

[rocky vector]

I don't know if it's being considered, but I saw some debate in the conversation above when I was skimming

Unfortunately don't have time to read through each message

[spare bloom]

Removal is not being considered for sure

[spare bloom]

Codec<BlockStateModel.Unbaked> CODEC

datagen of the blockstates files is using the same data structure as the model loading code

    public static class Unbaked implements BlockStateModel.UnbakedRoot {
        final List<MultiPartModel.Selector<BlockStateModel.Unbaked>> selectors;
        private final ModelBaker.SharedOperationKey<MultiPartModel.SharedBakedState> sharedStateKey = new ModelBaker.SharedOperationKey<MultiPartModel.SharedBakedState>(
            
        ) {

what is this weird vineflower output

@queen cradle have you reported this yet?

the blank line is really strange 😄

it's weird how the particleIcon of each BlockModelPart is barely being used

(most call sites call BlockStateModel#particleIcon() instead)

[queen cradle]

What class is that in?

[hardy talon]

Similar thing with the AO flag, only the one from the first part is used, the others are ignored

[spare bloom]

Why have it in the part at all then

[stiff fossil]

because I'm a lazy mojang dev and wanted this PR in, so I did not spend extra time patching all callsites to use actual parts

eventually, only parts should be used - if you break a block, you should see particles from actually rendered parts and not just whatever is first in multipart/multivariant

[pulsar lantern]

it's official, mojang is lazy

[tired rock]

Mojank :>

[spare bloom]

I see, thanks a lot

Do you think you will get to that refactor soonish? (Otherwise we'll probably do it already on our end)

[spare bloom]

It's in MultiPartModel.java

With these new changes we might want to consider custom unbaked block state models again 🤔

I just realized that the BlockState is gone from BlockStateModel#collectParts

I like the SharedOperationKey and in general the new multipart implementation

[hardy talon]

Yes, but the BlockStateModels that care about the specific state (in vanilla that's only the multipart model) capture their associated state during baking. As far as I understand the code, for a block with a multipart model, there is a separate instance of MultiPartModel for every state but the actually heavy part (the part selectors and cache of resolved subsets) is shared between all of those

[spare bloom]

Exactly

That is my understanding as well

[hardy talon]

I actually quite like it, especially because it should make some of my shenanigans even simpler than 25w08a already did 😄

[spare bloom]

I think we can kill bakeAsBlockModel

We might potentially introduce custom BlockModelDefinitions as well

I'll be busy for a while, but I am looking forward to this 😄

[spare bloom]

I just noticed that there are unbaked block model parts!

[hardy talon]

Yup 😄

[spare bloom]

feels a bit unnecessary

I don't see why we'd expose the ability to register custom unbaked model parts

it feels more practical to directly expose BlockStateModel.Unbakeds

[hardy talon]

Probably, yeah

I've been playing around a bit with the buffering side of models and I have some ideas for the issue of multiple render types in a single model. The basic idea it boils down to is that we'd patch the ModelBlockRenderer methods to additionally take a nullable Function<RenderType, VertexConsumer> (can't be a MultiBufferSource because of the buffer pack stuff used by chunk meshing, at least for the methods relevant to chunk meshing), make the vanilla VertexConsumer parameter nullable in the overloads and then use the function, if present, to get a rendertype-specific consumer and otherwise use the vanilla VertexConsumer paramter with a null check.

One thing I know for sure though is that the whole BlockModelPart stuff will make part collection in the FramedBlocks models an absolute allocation hell

[spare bloom]

So for sure I think we keep the parts static

And add a render type to them

Or actually

I hate these RenderTypes, we should just have a BlendMode

(Probably a discussion for another time though?)

The temptation to return a bullshit type is too high IMO

(bullshit i.e. not chunk)

[hardy talon]

I was about to say that, yeah

I'm honestly not sure though whether a single rendertype per part or support for potentially multiple is better.

[spare bloom]

A single is definitely better I think

Multiple means you also need to filter by render type in getQuads

[hardy talon]

That's true, yeah

[spare bloom]

Which also means that you're probably storing them separately anyway, so might as well have used different parts 😄

[spare bloom]

Another patch strategy is to filter the List<BlockModelPart> higher up in the stack trace

A single type per part is I think a good compromise between keeping the renderer simple and keeping model implementations simple (also when it comes to wrapping)

E.g. in MI I need to render a submodel transparently and just changing the render type makes it easy

[hardy talon]

That's going to make dynamic models even more expensive though when caching is not an option or not desirable because they'd be reconstructing the whole set of parts up to five times instead of once per position being rendered. The patches are actually surprisingly clean for this approach.

[hardy talon]

That's true, yeah

[spare bloom]

Could we store the part list once and loop over each chunk render type?

[hardy talon]

We could but the filtering would still require at least one additional list to transfer the relevant parts to. I don't know, filtering the part list feels kinda dirty to me

[spare bloom]

which call site are you looking at?

[hardy talon]

SectionCompiler#compile()

[spare bloom]

ah yes

you realize that collectParts is already collecting the parts into a reused list, right?

so filtering the list would be very clean imo 😄

[hardy talon]

The list is re-used, yes, but if you want to only collect parts once, then you need a second list (can be re-used as well) and iterate the first one five times to copy relevant parts to the second one

[spare bloom]

yes exactly

might be possible to special case the 0 part and 1 part cases to avoid iterating too much in those cases

I don't think it's a concern though, outputting the bakedquads is certainly a lot more expensive

on a completely unrelated note, we should not patch in our TriState ambientOcclusion, rather I would simply put the TriState in BakedQuad instead of the boolean that we currently have there

on another completely unrelated note, we should PR a MutableBakedQuad helper in NeoForge as soon as 1.21.5 is out because we have waited way too long for that already 😄

[hardy talon]

Another thing worth remembering is that requiring external filtering not only impacts the SectionCompiler, it applies to several other call-sites (primarily entity and BER rendering) and also makes certain use cases in mods even uglier (having the render type loop everywhere is already quite ugly IMO).

[spare bloom]

how did these call sites work previously? I'm sure a lot of them were simply using whatever vertexconsumer was provided, and we could continue to do the same

[hardy talon]

They where almost all patched to perform the iteration over BakedModelExtension#getRenderTypes(), causing them to end up basically like this: https://github.com/XFactHD/FramedBlocks/blob/1.21.4/src/main/java/xfacthd/framedblocks/client/render/special/GhostBlockRenderer.java#L210-L244

[spare bloom]

yeah i see

could we maybe have a helper for the iteration? I feel like that would be quite clean

[hardy talon]

We could, but that feels like yet another workaround for an annoying API. If we instead add a parameter to pass a function through, then that loop plus the helper method get condensed down to this:

List<BlockModelPart> modelParts = model.collectParts(RANDOM, modelData);
mc().getBlockRenderer().renderBatched(renderState, renderPos, level, poseStack, builder, false, modelParts, null, modelData);

[spare bloom]

Iterable<List<BakedModelPart>> groupByRenderType(List<BakedModelPart> parts, List<BakedModelPart> scratch) { ... }

keep in mind you still need to allocate a list for collectParts

[spare bloom]

where is the Function<BlendMode, VertexConsumer> there?

[hardy talon]

Almost all cases outside of SectionCompiler run on the main thread, so you could re-use a list stored in a static field

[spare bloom]

actually, another big advantage of passing in a Function<BlendMode, VertexConsumer> is that all the intermediate data structures that vanilla tries to reuse across parts will be reused

things like the bitset and blockpos$mutableblockpos in tesselateWithoutAO for example

[hardy talon]

The parameter being passed null because my specific use case here uses a consumer with a custom render type and passes that as the vanilla VertexConsumer parameter

[rocky vector]

can we replace the VertexConsumer parameter entirely instead of just augmenting the function?

[warped sluice]

I wanted to add such a param in Fabric because right now separating FRAPI geometry by BlendMode is possible but really inconvenient

[spare bloom]

can't you pass blendMode -> constant?

[hardy talon]

I could, yes

[spare bloom]

I don't think we'll make things more granular than one BlendMode per part (in NeoForge)

[hardy talon]

Actually, yeah, I can make that work without breaking vanilla bin compat

[rocky vector]

It should be fine to introduce a capturing lambda for the vanilla bin compat helper, as long as we never use it in hot paths in patched code

[hardy talon]

Yup

[warped sluice]

That's fine with me

I would only like the part to store its own RenderType so BakedModel.getRenderTypes is removed

[rocky vector]

Btw for this shouldn't we just move it up to the part level?

[spare bloom]

of course yes

[spare bloom]

also an option, but it is already a BakedQuad field as a boolean

[warped sluice]

I think it is fine to keep in the quad

[spare bloom]

(force-enable is quite rare anyway)

[rocky vector]

Even in vanilla?

[spare bloom]

no

[hardy talon]

Nope, we add that field

[hardy talon]

Disagree. FramedBlocks makes use of the model-level tristate method to force AO even for light-emitting blocks because the whole point behind using these blocks as light sources is to have them visually vanish in their surroundings

[warped sluice]

FramedBlocks is only one mod

[spare bloom]

I think that light-emitting blocks are basically the only use case

[warped sluice]

The option is useful for sure

[spare bloom]

I'm not saying that we should remove it, but it should be fine to have it as a per-quad option only?

[hardy talon]

Per-part would be better until we have a way to transform quads in-flight without allocations, otherwise I'll end up having to shallow-copy every single quad in those cases

[spare bloom]

per part is also acceptable I think

[rocky vector]

What are we still patching into baked quads as of right now

[hardy talon]

Only the AO flag

[warped sluice]

This will require changes to the json format

I think the option could be on both the quad and the part

However, if XFact wants to fully override what the quad says, this option doesn't really make sense

The quad would get the final say (if it's not using DEFAULT)

[hardy talon]

That's already the case now and it matches my intentions. Basically, what currently happens is the following:

• If a camo is present and its model requests specific AO (i.e. it returns something other than TriState.DEFAULT), then that value takes precedence
• If a camo is present and it emits light or is marked as emissive, then AO is force-disabled
• If the config says that blocks with the light emission modifier applied should force AO, then AO is force-enabled
• Otherwise it defaults to vanilla behaviour

[spare bloom]

starting to look into custom BlockStateModels...

to make this strategy work I need to perform open heart surgery on the codecs 😛

[warped sluice]

Could BakedModel.getModelData be removed?

[spare bloom]

there is no BakedModel anymore

[warped sluice]

I meant BlockStateModel

The Yarn name is in my mind

[spare bloom]

I don't even dare to imagine the yarn names for 25w09a

BlockModelPart is probably AmbientGeometry or something horrible like that 😄

[warped sluice]

More specifically, have its parameters inlined into collectParts, with the possibility of removing the BE ModelData param as it can be retrieved through the passed level using the passed pos.

[stray fog]

does yarn still try to remain clean-room?

[warped sluice]

Yes

[spare bloom]

looks like it's still class_10889

[stray fog]

I can't imagine why anyone would still want to guess names blind

for very little to no gain

but anyhow

I don't know if this still applies

but in the past, getModelData was separate from getQuads because getModelData was called in-thread instead of in the workers

[spare bloom]

I'm happy we kept NeoForgeExtraCodecs#dispatchMapOrElse 😄

[warped sluice]

That must have been a really long time ago as that hasn't been the case since at least 1.18

[stray fog]

it was the whole reason for it existing

instead of having a Level param in the getQuads method

to avoid reaching across threads

[hardy talon]

You're confusing BlockEntity#getModelData() and BakedModel#getModelData(). The former is called on-thread, the latter was always called off-thread

[stray fog]

well I could have misunderstood it

but I could have sworn getModelData was also called on the main thread

[spare bloom]

@hardy talon idk what to do about not inlining block state models inside the block state file

I'm not sure there's much of a choice

possibly we could allow models to be referenced in a top-level field of the BlockStateDefinition

[hardy talon]

Kinda sucks, but I guess that's just going to be the way forward

[spare bloom]

(similarly to texture slots)

[stray fog]

if it was off-thread, then I have to wonder why we had the method at all instead of passing a mutable (internally) state object into getQuads that includes the Level, BlockPos, etc.

🤷‍♂️

[hardy talon]

ModelData is also used for the particle texture getter

[warped sluice]

I thought it was for performance, because getQuads was called up to 7 times per call to getModelData

[hardy talon]

That too

[warped sluice]

getQuads and the particle sprite getter never share the ModelData from a single getModelData call as far as I know

[hardy talon]

Obviously, yeah, they are two completely separate code paths

[stray fog]

I wonder why we share the model data call, then

instead of having getParticleData separated

[hardy talon]

Legacy stuff, probably

[warped sluice]

But is data necessary at all anymore? I don't see a reason to keep it

[stray fog]

getModelData is how a custom model created via a model loader can handle world context

if there's a way to do this by other means now

then it may not be necessary

[warped sluice]

Yes, passing that context to collectParts and the particle sprite getter directly

[stray fog]

collectParts is outside the per-side lists, right? that's inside the part?

[hardy talon]

The parts encapsulate the per-side lists

[stray fog]

so 1 simple block has 1 part with 6 side-lists, with 1 quad each

[hardy talon]

7 lists, but yes

[stray fog]

well the no-side would be empty

[warped sluice]

Yes, collectParts is only called once now to get all geometry, so modeldata no longer provides any performance benefit

[stray fog]

yeah

so, assuming it is safe for a dynamic model to construct parts on the fly based on the provided context

I would guess getModelData would be unnecessary now

[warped sluice]

In 1.21.4 and as far back as I can remember, BakedModel.getModelData is called almost directly before BakedModel.getQuads on the same thread, so it is safe

To be clear, I'm suggesting void collectParts(BlockAndTintGetter, BlockPos, BlockState, RandomSource, List<BlockModelPart>)

[spare bloom]

maybe not BlockState

[warped sluice]

It may have a use for submodels in facade models, but I'm not sure

I would also suggest Predicate<@Nullable Direction> cullTest for use in models that perform expensive transformations and want to skip them if it is known that a certain face is culled

[hardy talon]

The blockstate parameter is very useful for facade models if you need to have the camo model to call the appearance API on a very specific state. I'm just realising though that combining data collection and part collection will make this significantly more painful because I wouldn't be able to pass a somewhat unexpected state to models which don't call the appearance API.

[warped sluice]

Why not?

I mean, if the parameter says

What's the reason for passing the unexpected state in the first place? You probably explained this before but I forget

[hardy talon]

Because it would blow up as soon as they try to get a blockstate property from it (which is already a problem right now with Create's CT implementation).

[hardy talon]

If I encounter a connected textures model, then that model has to call the appearance API on the framed block blockstate instead of the camo blockstate, otherwise the shape-sensitive connection gating doesn't work

[warped sluice]

Ah, yeah. Could that not be in the contract of the BlockState param, to not assume that it is always what is expected?

[hardy talon]

Sure, that would be an option

[warped sluice]

It's not like getModelData/getQuads have those contracts right now, so separation of data collection/geometry collection doesn't really help and leads to Create causing an error anyway

[hardy talon]

That's true, yeah

[warped sluice]

Are there any other issues with the suggested signature?

[spare bloom]

I think that adding back the block state is really weird given that mojang removed it

I am not necessarily against adding it back, but it needs a bit of thought

[warped sluice]

The model can retrieve it from the level and pos anyway, but if a supermodel wants to replace a submodel's blockstate that it uses for appearance getting, it would have to wrap the whole level

Possible, but certainly not convenient, and also requires an allocation

[hardy talon]

Yeah, I would have to wrap the level because of the double models

[hardy talon]

The only additional thing we'd need to think about is the cacheable data stuff we talked about. We could add that as an additional nullable parameter.

[spare bloom]

cacheable data would be a separate function

receiving the same context as collectParts, but returning a cache key

[hardy talon]

Yes, of course. What I'm talking about is passing that data back to the model it came from to avoid having to re-compute that data for part collection

[spare bloom]

quite ugly I think

if you are even working with the cache key, it means that you expect the transformations to be highly expensive

Codec<Weighted<Either<net.neoforged.neoforge.client.model.block.CustomUnbakedBlockStateModel, SingleVariant.Unbaked>>>

what could go wrong

I get ingredient type flashbacks 😄

[hardy talon]

Hehehe

[spare bloom]

idk if CustomUnbakedBlockStateModel is the best name, it's a bit lengthy

[rocky vector]

I think merging collectParts and BakedModel#getModelData makes sense now that there's a canonical method that's called once for the whole model

20 minutes late to that convo but I was driving

[hardy talon]

Btw, I noticed earlier that restricting BlockModelPart to a single render type means that multi-rendertype registration in ItemBlockRenderTypes becomes useless since the part looking up the render type from ItemBlockRenderTypes wouldn't be able to return more than one. This would also significantly reduce the patches in that class and ChunkRenderTypeSet can be removed

[spare bloom]

ChunkRenderTypeSet can be removed
where do I sign up? 😄

[rocky vector]

actually render types alone require us to pass the BlockState to collectParts so the vanilla models can default to resolving the render type through ItemBlockRenderTypes

unless we intend to attach the render type to the model during baking

edit: actually can't do that because of leaves

which I wouldn't be against, since it would avoid a hot map lookup during meshing, it's just not how we did it before

[spare bloom]

if we have a BlendMode we can simply have a DEFAULT

[rocky vector]

what is BlendMode

[hardy talon]

The only slight caveat is that we'd need to convert from RenderType to BlendMode during lookup or preemptively with an additional map if we decide to store a BlendMode in the part instead of a RenderType

[hardy talon]

An enum of blending modes, i.e. solid, cutout, cutout mipped and translucent (and maybe also tripwire)

[rocky vector]

That doesn't sound very extensible

[spare bloom]

it's not, by design 😛

[spare bloom]

well, the RenderTypeGroup would have a BlendMode for block essentially

[rocky vector]

We don't officially support custom chunk render types but everything before worked with them

[spare bloom]

did a first pass on BlockStateModel.Unbaked extension

⚠️ codec surgery ⚠️

https://gist.github.com/Technici4n/8130c7238012929a8062f250c7dec0bf

for @warped sluice 😄

[hardy talon]

That's not what I mean. If there is no RenderTypeGroup because the JSON didn't specify a render type, then we need to either convert on the fly to BlendMode from the RenderType we get from ItemBlockRenderTypes or we need to store an additional pre-converted map in ItemBlockRenderTypes

[spare bloom]

ah yes

for FRAPI this is stored inside the mutable state of the renderer

[spare bloom]

even ChunkRenderTypeSet?

[hardy talon]

Yes

[rocky vector]

As long as they had IDs assigned, it should've worked

I think we should continue to not deliberately break them even if they're not possible to make with an API

[spare bloom]

well, part of the problem is that people return all sorts of wrong render types

[rocky vector]

That throws basically immediately due to the missing ID

[hardy talon]

If ChunkRenderTypeSet goes, then the ID goes as well

[rocky vector]

ah

well, it should still be possible to throw in the Function<RenderType, VertexConsumer>

[spare bloom]

in general if we add RenderType renderType() to BlockModelPart and use it blindly, we will allow custom render types to "work" (but not really in practice)

[spare bloom]

sure but that's not much more different from adding an enum

what we can do is not use an enum, but rather a class with a private constructor?

(kind of enums-at-home)

[hardy talon]

SectionCompiler#getOrBeginLayer() will NPE if the SectionBufferBuilderPack does not contain the render type

[spare bloom]

if you want to hack a custom chunk render type in, surely you can use a mixin to add the enum value? 😄

(maybe we should have made enum extender run after coremods/mixins after all 😛 )

[hardy talon]

Making it run after would break some of the guarantees it provides. Apart from that, you're already throwing mixins everywhere to make a custom chunk render type work, so one more certainly won't hurt

[spare bloom]

yeah

this sort of mixin is really not the end of the world IMO if you're adding custom render types: https://github.com/SpongePowered/Mixin/issues/387#issuecomment-888408556
@rocky vector still possible really

[hardy talon]

Another question: to what degree do we want to support dynamic data in BlockModelPart (i.e. for quads, the render type, AO and the particle texture)?

[spare bloom]

ideally no dynamism whatsoever

[warped sluice]

I agree with that

[hardy talon]

Ok

[warped sluice]

Thanks, I'll take a look. Ideally I would add something similar to Fabric.

[hardy talon]

Hmm, what should I call the tristate version of useAmbientOcclusion()? Can't use the same name since both would be no-arg

[spare bloom]

in which class?

[hardy talon]

BlockModelPartExtension

[spare bloom]

how cursed is ambientOcclusion? 😄

[hardy talon]

Works for me

[rocky vector]

ew

I guess

[spare bloom]

How's the progress, XFact? 😄

[hardy talon]

I'm currently goofing around with the model part stuff in a mod dev environment (I commented some stuff out to make it compile). I'm not sure whether I like it yet or not, especially for highly dynamic models

[spare bloom]

What are the pain points?

(and how do they compare to similar situations in item models?)

[hardy talon]

Primarily having to constantly copy the parts

[hardy talon]

It's not comparable. The BlockModelPart interface exposes everything a part holds, you're not sitting in front of an abstract thing that may hide quite literally anything. Item models are the complete opposite in that regard

[hardy talon]

But maybe I'm also going about this the wrong way and trying too hard to stick to old ways of doing things

[spare bloom]

Well, depends why you are copying thel

If you are collecting the geometry and modifying it you might as well gather all parts of the submodel(s), and then turn that into one part per render type?

[hardy talon]

That's effectively of what I'm doing, except that I'm copying the parts 1:1 instead of N:1 per render type because it makes the code simpler.

[spare bloom]

My take is that previously you'd have needed to allocate one bakedquad list per rendertype anyway 😄

[hardy talon]

In previous versions, the uncached path in FramedBlocks worked as follows:

• Get cullable quads for the requested face
• If empty, attempt to extract technically cullable quads from the list of uncullable quads (works around mods with broken models, happens far too often unfortunately)
• If the camo is emissive, copy the quads list and bake lighting into the quads
• If further quads need to be added, the list is copied
• Collect additional untransformed quads if present (relies on the previous copy)
• Add reinforcement quads if necessary (relies on the previous copy)
• Perform late uncached post-processing (relies on the previous copy)

Now it looks like this (cached and uncached path combined):

• Collect all parts of the camo model
• Iterate the camo parts, deep-copy them with a side filter, apply uncached post-processing and the emissive baked lighting to the quad list
• Get or create cached part list
• Collect additional untransformed parts (need to be shallow-copied to apply AO, won't be caught by the uncached post-processor anymore)
• Add a pre-computed part for the reinforcement, deep-copy it if an uncached post-processor is present and apply the post-processor

To be honest, I might be making this unnecessarily hard on myself by trying to avoid capturing lambdas in the uncached paths at all costs

Here's the changes I'm playing around with as a patch: https://gist.github.com/XFactHD/c93ce5a9a200c74fbf64bd1092f50f58

[spare bloom]

Didn't look at the details but it looks reasonable

[spare bloom]

Both look very complex 😄

[hardy talon]

They are, yes. But also very powerful

[spare bloom]

the composite_block test is so screwed with unbaked block state models 😄

[warped sluice]

In terms of API it's the simplest possible so I agree with it (though I will consider making the codec getter part of interface injected FabricUnbakedBlockStateModel instead). The codec patches are quite bad but it should still be possible on Fabric.

[spare bloom]

unless one uses some hack to move the bulk of the definition to an unbaked model

(after all additional properties can do any magic)

[hardy talon]

Making the files for block models with custom loaders not a complete pain in the ass will in general be an issue

[spare bloom]

well I expect 99% of the files to look like

{
  "variants": {
    "": {
      "type": "mymod:model"
    }
  }
}

where it gets messy IMO is when you combine it with vanilla's state-based selectors

e.g. this:

{
  "variants": {
    "facing=east": { "model": "composite_model_test:block/composite_block", "y": 90 },
    "facing=west": { "model": "composite_model_test:block/composite_block", "y": 270 },
    "facing=north": { "model": "composite_model_test:block/composite_block", "y": 0 },
    "facing=south": { "model": "composite_model_test:block/composite_block", "y": 180 }
  }
}

where the "composite_model_test:block/composite_block" model is a huge thing 😄

[hardy talon]

I suspect that this will end up requiring a combination of a blockstate loader and an unbaked model loader or something janky like that

[spare bloom]

if the children of the composite get split into separate files it gets easier to manage

does the rotation of the composite even work?

[hardy talon]

Which one? Rotation coming from the blockstate file or rotation coming from the root transform?

[spare bloom]

both seem to be implemented

[hardy talon]

Indeed

[spare bloom]

but it won't rotate dynamic models unless those were careful to implement the ModelState

(which they usually don't care about... at least mine 😄 )

[hardy talon]

I've always had my model loaders take that into account

[warped sluice]

Was the root transform TODO fixed?

[hardy talon]

This one ^?

[spare bloom]

MI's pipes clearly won't rotate if you add a rotation in the pipe blockstates file 😄

[hardy talon]

I'd consider that a special case where it doesn't make sense to take it into account

[warped sluice]

Probably not: https://github.com/neoforged/NeoForge/blob/1.21.x/src/main/java/net/neoforged/neoforge/common/util/TransformationHelper.java#L142

[hardy talon]

Ah, no, that's still present

[spare bloom]

just added a trivial composite blockstatemodel (not using our rendering extensions yet since those are still TBD)

[warped sluice]

Can it be fixed in this snapshot?

[spare bloom]

this is not rendering anything, right?

[hardy talon]

Correct

[spare bloom]

in this case it should be possible to composite the geometry

[hardy talon]

I'm not sure what you mean by that

[spare bloom]

I mean that for the specific case of this composite_block, it should be possible to use a custom unbaked model loader that combines the geometry of multiple submodels

[hardy talon]

How would that work though? If we keep bakeAsBlockModel(), then an unbaked model loader can at most spit out one BlockModelPart and if we remove it, then we only get a single QuadCollection, neither of which is sufficient for one of the primary use cases of composite models which is to combine multiple render types in a single model

[spare bloom]

bakeAsBlockModel() is for sure going away imo

it doesn't make sense to have that and custom unbaked blockstatemodels

[hardy talon]

True

For the composite model it really doesn't matter what we do with that method, both cases are insufficient to make this work in an unbaked model loader

[spare bloom]

why?

if you can produce an arbitrary BlockStateModel you can do anything no?

[hardy talon]

Yes, but an unbaked model loader cannot produce a BlockStateModel on its own

[spare bloom]

ah right, in your patches you changed it to a block model part?

we could make it bake to a block state model if we really wanted to

but I think it's cleaner to not do that 😄

[hardy talon]

Yes, because that's what the patched call-site expects, Variant is a BlockModelPart.Unbaked

[spare bloom]

hmm yeah

we could patch it even deeper but heh

[hardy talon]

That's going to get really ugly

Or maybe not, hmm

[spare bloom]

in principle just SingleVariant.Unbaked#bake

[hardy talon]

Yeah

[spare bloom]

again I'm not saying it's a good idea 😛

[hardy talon]

Yeah, I'm kinda torn on it. On the one hand a blockstate model loader/type is cleaner in terms of implementation, but on the other hand it makes some custom models a complete pain in the ass.

With your current implementation, at what level(s) of the blockstate file can a type be specified?

[spare bloom]

only where you would normally have a "model": "xxx" instead

[spare bloom]

this example basically translates everywhere

I am seriously considering allowing a list of models as a top-level field though

but idk how much it would help in practice (you could also just tweak the selector)

like

{
  "submodels": {
    "m1": {
      "type": "mymod:model"
    }
  },
  "variants": {
    "whatever=true": "#m1",
    "whatever=false": "#m1"
  }
}

[rocky vector]

are we inventing the forge blockstate format again

[hardy talon]

I think the option to entirely take over the format might be more useful. Then you could handle it the way Athena does (https://wiki.terrarium.earth/athena/loader-introduction) and avoid having to duplicate the same data in every selector (this would also make the workaround of combining a blockstate model type and an unbaked model loader unnecessary for almost all cases)

[spare bloom]

yeah definitely

I think it's a bit of a different use case though

as in, there should be a use case for unbaked blockstatemodels AND a use case for taking over the whole format, no? 🤔

[hardy talon]

Definitely. I was more questioning the usefulness of a top-level model list

[spare bloom]

okay I'll look into custom block model definitions

let's already remove bakeAsBlockModel

[hardy talon]

From a quick glance that should be as simple as adding another Optional to BlockModelDefinition, checking for it in the validation and making BlockModelDefinition#instantiate() defer to the custom thing if present. I would also split the vanilla implementation off into a BlockModelDefinition#instantiateVanilla() method such that a custom definition can invoke the standard vanilla handling and then post-process the vanilla result if desired

[spare bloom]

indeed

I was also thinking of such an optional

good idea to split the vanilla instantiation

[warped sluice]

Taking over the whole format isn't possible because you can only deserialize to Unbaked, not UnbakedRoot, right?

Oh and the definition returns models for multiple states at once, which might be convenient/necessary for some custom formats

[hardy talon]

BlockStateModel.Unbaked has a asRoot() method to "elevate" it to a BlockStateModel.UnbakedRoot

[warped sluice]

Ah right

The utility of replacing the whole file does seem somewhat slim then

The BlockState would be received during baking, so you would be able to delegate to anything else there

[spare bloom]

there is still one UnbakedRoot per variant selector

[warped sluice]

You can just use

{
  "variants": {
    "": {
      "type": "whatever",
      "custom_properties": ...
    }
  }
}

and have the necessary logic in the type

[spare bloom]

yeah in most cases I think that will be sufficient

[warped sluice]

I honestly don't see in which cases it wouldn't be

The only benefit I can see is that it would be slightly more compact

[hardy talon]

When you have the exact same data for every single selector and want to avoid duplicating it over and over again

[warped sluice]

Why would you need multiple selectors in that case?

Or perhaps you mean that for example two selectors use some model A and two other selectors use some model B and you don't want to define A and B twice each?

[hardy talon]

In order to still support the vanilla parts of the format. As a simple example: if I have a pillar block with an axis property and want to have connected textures on it, then I still need three variants for the orientation but I don't want to duplicate the CT metadata across all three variants if I can avoid it

[warped sluice]

BlockStateModel.Unbaked doesn't receive ModelState, so how would you implement that?

[hardy talon]

I would call back into the vanilla BlockModelDefinition#instantiate() and then wrap the models

[warped sluice]

It seems like this local model reference list idea #1342994966405845063 message should cover this use case

You can configure the CT model once in the list, then for each axis you use a type that combines an arbitrary BlockStateModel.Unbaked with a ModelState, and use a reference to the CT model

Combining BlockStateModel.Unbaked with ModelState is impossible to do efficiently though because the Unbaked may produce a custom BlockStateModel that depends on world state

[spare bloom]

that does not exist

Combining BlockStateModel.Unbaked with ModelState is impossible
indeed

[warped sluice]

It's possible by always transforming the geometry from the submodel in collectParts but that is extremely slow

[spare bloom]

yeah that's not even up for consideration 😄

[warped sluice]

It seems like what XFact was suggesting he would do though

[warped sluice]

What doesn't exist? The reference list? I know, I am talking about it as a possible addition to the format instead of top-level definition types, which don't exist right now either (unless they were already added to the repo)

[spare bloom]

I meant the ability to pass a ModelState to bake an unbaked blockstatemodel

[warped sluice]

Ah yes

[hardy talon]

I can't see a way to wire such a reference into the BlockModelDefinition, the whole thing is hard-wired to deal with direct references to a model file.
It also wouldn't solve my particular (arguably slightly niche) use case of a format that can be loaded in vanilla (and ideally in Neo without the type being present)

[spare bloom]

so much effort for an unpublished mod

you should probably just publish it 😛

[hardy talon]

I might actually do that, yes

[warped sluice]

Yeah, all the codecs use BlockStateModel.Unbaked.CODEC which makes sense but makes references difficult

[spare bloom]

it would be possible with custom ops but I don't think it's worth the complexity

[hardy talon]

No, what I would be doing at a file level is this:

{
  "variants": [
    // Standard vanilla declarations
  ],
  "type": "contex:main",
  "contex:meta": {
    // Special metadata
  }
}

That way the file can load in vanilla without any changes. With the current format at the model file level I have that worked flawlessly, including loading like vanilla on Neo when the loader is absent

[warped sluice]

Your axis model example, though

Either each of the 3 selectors are distinct by using different ModelStates (x and y rotations) or they are all the same and the use case doesn't apply. Maybe I am confused

Either way, that contex meta should work with arbitrary BlockStateModel.Unbakeds, right?

Wait I think I understand

[hardy talon]

The metadata doesn't care about the ModelStates specifically, I would just be wrapping the unbaked model coming from the vanilla path, consider it a black box for the most part and then resolve the parts/quads from the baked model later

[warped sluice]

I see

The reference list wouldn't even help then because the submodel of each selector would not be the same, even if the contex meta is

[hardy talon]

For the most part, yes, with the caveat that dynamic models (i.e. ones depending on world state) wouldn't work. That's already the case now and I'm fine with that

[hardy talon]

Yup

[warped sluice]

I am not exactly against custom definitions but I am hesitant because it creates a third custom registry related to model deserialization

Then again, item models alone have like 6 registries

[hardy talon]

7 vanilla registries plus our unbaked model loaders

[warped sluice]

That's the first custom registry

Perhaps another option would be to make the contex meta a reference to another file that holds it, but baking only receives ResolvedModels so it wouldn't work

It is a consideration though because that's kind of what preparable plugins do, except that their data is only exposed during modifier event registration

[hardy talon]

Combining a custom blockstate model type and an unbaked model loader, keeping the metadata in the model file like is the case now and then instanceof-checking the ResolvedModel's wrapped UnbakedModel would work but it's kinda awkward

[spare bloom]

it's also an option indeed, but it's not great 😄

it's basically bakeAsBlockModel-at-home 😄

[hardy talon]

Yup

[spare bloom]

ok, I did rework the composite unbaked model to match my vision for it

(i.e. static geometry)

essentially it is just replacing bakeAsBlockModel by bake, and merging all the quad collections

[hardy talon]

Please document that multi-rendertype models must use the blockstate-level CompositeBlockModel or vanilla's composite item model instead, depending on the use case

[spare bloom]

I think now is a good time to restore the block state model hools

[spare bloom]

i.e. this

[hardy talon]

Sounds good to me, I'll push what I have now and we can iterate on it (i.e. remove model-level getModelData() in favor of directly providing context, etc.)

Pushed, have fun 😄

[spare bloom]

😄

[spare bloom]

changes look good, I have nothing to add really

(except for a getModelData override in composite block model unless we decide to get rid of that method?)

[hardy talon]

If we keep it, then we'll have to do something similar to MultipartModelData for the composite model

[spare bloom]

yeah... it is available a few commits ago, so it could just be copy/pasted in

however it is exactly when composing models that you see just how annoying getModelData is

[hardy talon]

Yeah. Could probably even re-use it since it's just a Map<Model, Data>

[spare bloom]

yup

maybe I'll already add it back and worst case we remove it again

[hardy talon]

Sounds good to me

If you're touching that anyway, do you mind fixing the multipart model's part collection? I just noticed that I fucked that up and forgot to unpack the MultipartModelData for each part

[spare bloom]

fixed the composite using a much simpler data key

[hardy talon]

👌

[spare bloom]

you mean the missing resolve call?

[hardy talon]

Yup

[spare bloom]

fixed

[hardy talon]

Thanks

[spare bloom]

    public static boolean isBlockInSolidLayer(BlockState state) {
        var model = Minecraft.getInstance().getBlockRenderer().getBlockModel(state);
        return model.getRenderTypes(state, RandomSource.create(), ModelData.EMPTY).contains(RenderType.solid());
    }

such a cursed thing 😄

[hardy talon]

I want to nuke that thing

It would be simpler (and IMO more correct) to check BlockState#isSolidRender()

It also removes the reach from common into client code

[spare bloom]

is this "just" to stop culling the face if a resource pack changes the render type? does that even make any sense?

[hardy talon]

The intention is to prevent a neighboring block from telling the block being checked to not render a face if the block doesn't want it. The default implementation of "doesn't want it" is whether it's in the solid layer

[tired rock]

Removes the reach?

[spare bloom]

sorry I can't parse this 😄

[hardy talon]

The method Tech quoted above is in ClientHooks and called from IBlockExtension. It calls client-only code from common code

[spare bloom]

it allows the block to prevent faces of adjacent blocks from being culled iirc

[hardy talon]

You have block A (the block being checked for face occlusion) and block B (the neighboring block on the side being checked). In vanilla, block A can decide whether it wants to skip rendering a face in presence of block B by overriding Block#skipRendering(). We add an extension that allows block B to decide that block A's face should not render in the presence of block B (useful for facade blocks that want to make the real block skip a face when a facade block with the same block as camo is adjacent). IBlockExtension#supportsExternalFaceHiding() (which calls the code you quoted) allows disabling that extension and does so by default for blocks rendering in the solid layer

[spare bloom]

so if we are solid, we don't let hidesNeighborFace cull the face

[hardy talon]

Yup

[spare bloom]

does that make any sense?

[hardy talon]

Debatable

[spare bloom]

if you are next to a solid block, your faces should be culled regardless of their render type, no?

[hardy talon]

Well, it would be a case of the occluded block being the solid block, not the occluding block

supportsExternalFaceHiding() is called on the occluded block and hidesNeighborFace() is called on the occluding block

[spare bloom]

IMO the render type of the occluding block matters, but not the render type of the occluded block

[hardy talon]

IMO neither render type matters, render types don't decide about occlusion. If we keep this default, then it should be based on the occluded block being considered fully solid in terms of its occlusion shape (which is easiest to check with BlockState#isSolidRender()).
In all honesty though, this is a Lexism, it was introduced because "blah blah xray blah"

[tired rock]

Gotcha

[spare bloom]

Xray really?

[hardy talon]

It was some weird justification like that, yes

[spare bloom]

There: #coordination message

Not from Lex but still a weird and vague argument

I would just change it to default to true

[hardy talon]

Fine by me

[rocky vector]

https://github.com/neoforged/Kits/blob/bd064bdf1af674c88c5e4354cb0b98058471c1dc/src/main/java/net/neoforged/neoforge/client/extensions/BlockStateModelExtension.java#L37-L38

IMO this should not be non-extendable

Models with a static immutable list could avoid the allocation here, no?

also unrelated: I thought we wanted to just give the world context to collectParts instead of having a separate getModelData

[hardy talon]

Both WeightedVariants and MultiPartModel call the three-arg method on the inner model(s), so that's not an option

[rocky vector]

Hmm ok, guess the recommended approach for hot paths will be to just use the three-arg method and manage the list yourself then

[hardy talon]

Yup

[spare bloom]

Doesn't the hottest path reuse the list anyway?

[rocky vector]

sounds like it

still curious about getModelData

it looks redundant

[spare bloom]

What? It's been there like that forever

[rocky vector]

The original reason for that was getQuads being called multiple times per render type/direction

That's not the case with collectParts

[spare bloom]

The option we are considering is inlining the BlockAndTintGetter, BlockPos and BlockState into collectParts

[rocky vector]

Yeah that's what I'm asking about

[hardy talon]

I hadn't done that change locally yet, I just pushed what I had

[spare bloom]

(It needs to be inlined into the particle too)

[rocky vector]

gotcha

[warped sluice]

I am all for the removal

If it is done, Neo's API will basically be FRAPI at home which will still be significantly better than what existed previously

I'll try to add the level context to the particle sprite getter on Fabric but IIRC it's not trivial

[spare bloom]

One issue with passing Level directly is that it won't allow AE2 part models to retrieve the model data corresponding to the part in a nice way

But maybe part models should simply be a new completely separate interface rather than reusing BlockStateModel

[rocky vector]

can you elaborate

shouldn't it work with the same system as multipart models?

[spare bloom]

so, assume we have this signature: void collectParts(RandomSource random, List<BakedModelPart> parts, BlockAndTintGetter level, BlockPos pos, BlockState state)

To access model data from a BE, you'd call level.getModelData(pos). This works completely fine for a multipart model, but it is a problem when you need to pass a subset of the data to a submodel

imagine for AE2, the model data would have a Map<Direction, ModelData> property (simplified), which contains a map of the side where there is a facade to its model data

previously it could have been something like this:

Map<Direction, ModelData> partsData = ...

for (var part : parts) {
    collectParts(..., partsData.get(part.direction()));
}

of course this doesn't work anymore in the new system

one of the options would then be to let the part model handle this directly, e.g. by moving the model property to API, but it might be a bit awkward?

[hardy talon]

Wasn't this already an issue in the past when AE2 was still available on Fabric?

[spare bloom]

it was, and our solution was to check for a specific subclass of BakedModel

(which broke when continuity wrapped the model... fun times)

[hardy talon]

Oof

[spare bloom]

"annihilation plane invisible with continuity" should find some issues

[spare bloom]

I think the cleanest solution would be for parts to have their own model type

The question is, how tf should I load them 😄

Could hardcore them in code but it's not too great 😦

[rocky vector]

I'm still not understanding, shouldn't your model data be structured such that it encodes what data belongs to what part?

oh nvm I see what you mean

to solve this it seems like you'd need to construct the BlockModelPart on the fly, holding an internal ID, that it uses to know what part of the model data it should be looking at

but that's still inefficient since every part has to query level.getModelData(pos)

What if you move creating the list of parts to the BE, and just embed the final list in its model data?

[warped sluice]

This makes the most sense because in one way or another the part ultimately needs more context than what collectParts would normally provide

[spare bloom]

Not sure what you mean. Don't confuse AE2 parts with block model parts 😄

[spare bloom]

How to load them though? A preparable model loading plugin but on NeoForge? 🤔

[warped sluice]

Or actually parts don't receive that context anyway, nor should parts really ever be wrapped, so it should be fine

[warped sluice]

What are they loaded from right now?

[spare bloom]

I think they're just BakedModels, but idk how they are being loaded

[random pebble]

AE2 part models?

We collect them all and pass them to the CableBusModel as dependencies / via ctor

and we also pass them to ModelEvent.RegisterAdditional

[spare bloom]

They're not really data driven right?

(as in no type dispatch)

[random pebble]

Type dispatch? I don't follow. We scan for explicitly listed part models on part classes and use those as standalone models

[spare bloom]

We scan for IDs? That won't work anymore now that unbaked models are purely static 😄

[random pebble]

Yeaaaaah fun times ahead of us

What does that mean exactly?

[spare bloom]

Idk I guess I'll see when I do the port 😛

[random pebble]

No, I meant what does "unbaked models are purely static" mean 😄

[spare bloom]

UnbakedModels only resolve to static geometry

Static geometry is all you get from unbaked models

[spare bloom]

collectParts(RandomSource random, List<BlockModelPart> parts, BlockAndTintGetter level, BlockPos pos, BlockState state)

truly an awful parameter order IMO 😄

shouldn't we do BlockAndTintGetter level, BlockPos pos, BlockState state, RandomSource random, List<BlockModelPart> parts?

[warped sluice]

I agree with the latter

[spare bloom]

thoughts on this? is there any point in forwarding to the overload with more context?

we can't really provide a block state, and it's the only parameter for which I wouldn't want to pass a garbage value: it is used to select the model and should therefore always be correct

[warped sluice]

The state is not guaranteed to be what the model expects

I think it is fine to pass AIR as the state

Plus, if that's not sufficient for the model, it can override the method and use whatever params it needs

[hardy talon]

Yeah, I agree with just using air

[spare bloom]

Really?

I would blindly query a state property tbh

[warped sluice]

We discussed this with XFact in this channel

The model can be used as a submodel for a facade

[spare bloom]

I feel like I would then pass the block state of the facade

In the world the state might not match, but it should IMO match as a parameter?

[warped sluice]

That is what's passed, because the submodel can call getAppearance on it, but the submodel is for an arbitrary block. The getAppearance may be used for CTM

[hardy talon]

Blindly querying a state property is how you end up with this: https://github.com/XFactHD/FramedBlocks/blob/1.21.4/src/main/java/xfacthd/framedblocks/client/model/FramedBlockModel.java#L399-L411. It would be nice to standardize being able to pass the state of the outer model instead of the camo model. If the outer model is forced to pass in the camo's state, the appearance API falls apart because it means that the block checking for connections can no longer take control over the outward connections (example: https://github.com/Creators-of-Create/Create/issues/5167)

[spare bloom]

I find it weird that passing the wrong state might lead to better results

[spare bloom]

Ah right

The submodel should call getAppearance to resolve the correct block state before any property check?

[hardy talon]

Yes

[warped sluice]

Passing the expected state is useless anyway. If you need it, store it as a field whose value is equal to the BlockState passed when baking. I suppose that doesn't work for non-roots but they are probably less likely to need the state.

[spare bloom]

how about this for the documentation then?

[hardy talon]

Not adding any part is almost never correct, it should fall back to some default set of parts (i.e. completely unconnected textures in the case of a CT model). Other than that, the BE note is missing a word: Avoid accessing or manipulating the block entities directly. I would also make the last paragraph more specifically about the provided BlockState as BE-provided ModelData being absent is effectively (even if extremely briefly) mentioned by IBlockGetterExtension#getModelData().

[spare bloom]

Thanks! Better?

[hardy talon]

Looks good to me 👍

[spare bloom]

can be removed, right?

[hardy talon]

Probably can be, yeah. I meant to check where that was actually used but completely forgot, let me do that real quick

[spare bloom]

it was used in RenderType getRenderType(ItemStack itemStack) in BakedModel

which was in turn called from BlockModelWrapper in 1.21.4

[hardy talon]

Yeah, then it can just be removed

[rocky vector]

I would suggest not telling them it's usually a RenderChunkRegion

That encourages if(getter instanceof RenderChunkRegion region) getter = region.level; nonsense

which is almost always the wrong approach

[spare bloom]

oops, pushed another file by mistake heh

🤷

[spare bloom]

just removed that note then, it was pointless anyway

thanks

pushed

now we need to think about a method to return all of the state used by a model

[rocky vector]

you mean the ModelData keys that can be cached?

[last wave]

Yes and no

Basically so that BakedModel can return a cacheable key

Not neccesarily from ModelData

[rocky vector]

If it's not from ModelData, what is it computed from

[last wave]

Undetermined as of yet 😄

That is basically what we are trying to determine

[rocky vector]

The model needs to then receive the same key as context in collectParts, to avoid retrieving the same data twice (once for key computation, once for actually using the information)

which seems redundant

[last wave]

I would say that seems logical

[warped sluice]

I don't think it's necessary

[spare bloom]

I don't think so either; the premise is that the geometry transformations will be so expensive, that computing the cache key will be worth it on average

working draft:

    /**
     * Collects a snapshot of the data used by the model to produce rendered parts.
     *
     * <p>This allows the geometry produced by this model to be cached,
     * provided that the snapshot matches.
     *
     * @return a snapshot of the data used by this model; can be any object as long as
     *         it implements {@link Object#hashCode()} and {@link Object#equals(Object)} correctly
     */
    // TODO: default implementation?
    // TODO: is null allowed?
    // TODO: optional, or required for all models?
    // TODO: name?
    // TODO: what about the random source?
    // TODO: add an example of checking the snapshot in the javadoc
    Object snapshotUsedData(BlockAndTintGetter level, BlockPos pos, BlockState state);

[rocky vector]

It should ideally be a generic type so that collectParts doesn't need to cast

I don't know if that's doable with extension interfaces though

[spare bloom]

collectParts should not receive it

[warped sluice]

The only case where it might be useful is when the key user gets the geometry. If the key contains some precomputed values, some performance may be saved, but I don't think the average user will care about this. Always forcing key computation is inefficient in the general case.

[rocky vector]

Oh I see, the key isn't always used

[spare bloom]

that too yeah; in some cases (e.g. when faces are culled) parts of the key might not be used

[rocky vector]

then I think the problem will be that not everyone implements this

[hardy talon]

Realistically this only affects a handful of mods, primarily CT mods, so I'd say that's fine

[warped sluice]

They will have to once someone reports their mod doesn't work with Framed Blocks

Standardizing this is still useful

[spare bloom]

what is the default behavior? always cache or never cache? 😛

[hardy talon]

I would say default to returning null

[vernal parrot]

rand.nextInt(2)

[warped sluice]

I'd say the random is necessary. The key user can provide it easily because it only requires the pos and state to create, which the key user already has.

Imagine the case where the random is used to offset vertex positions

[spare bloom]

well, the alternative is that the key user can only cache if the random matches

which I guess is highly problematic 😄

[warped sluice]

Hm that's true

If the model requires it though then that should be up to the key user to handle, not the model

[spare bloom]

the logic is that the model should extract the minimum amount of data needed from the context

passing the random means that the cache can be more effective

and when it's not used it will simply not be put in the key

[spare bloom]

sure; what you do with that then is up to you

(e.g. print a warning + cache anyway)

[warped sluice]

Yeah. The random itself should not be in the key but whatever it computes should be in the key.

I think it's important to establish that none of the passed params except state should be directly included in the key

[spare bloom]

in any case I'll let this sit overnight 😉

[hardy talon]

The most likely outcome will be that I'll pass EmptyBlockAndTintGetter.INSTANCE and BlockPos.ZERO to models which don't provide this key to ensure with high certainty that they provide fallback parts (i.e. unconnected quads from a CT model). That's basically what I'm already doing by passing ModelData.EMPTY to getQuads() when I cannot extract a known CT data object from the ModelData or CT support is disabled

[spare bloom]

ah yeah that's quite clever actually, I like that

[warped sluice]

I think null should not be allowed because its only purpose is a unique value. The same can be achieved with static final Object UNIQUE = new Object();

Ah you're trying to use null as determine that the model does not implement the method

[hardy talon]

Yes

[warped sluice]

It doesn't make sense for one invocation to return null and another to not return null, right?

[hardy talon]

Depends. I wouldn't consider it unreasonable for a model to return null if the provided context would compute the same result as no context. In practice I expect that implementors of this will either always return a non-null result or always return null.

[spare bloom]

should dependency on the block state be put into the state snapshot?

[warped sluice]

I don't see why not

It's up to the model to decide

The main purpose of the key is to say that certain different inputs map to the same geometry output

[spare bloom]

(depends if the user has to check for block states on top of checking for the key)

v2:

    /**
     * Collects a snapshot of the data used by the model to
     * {@linkplain #collectParts(BlockAndTintGetter, BlockPos, BlockState, RandomSource, List) produce renderable parts}.
     * The returned snapshot encapsulates which parts of the world state the model depends on.
     *
     * <p>This allows the geometry produced previously by this model to be reused,
     * provided that the snapshot matches.
     *
     * <p>The passed in {@code level}, {@code pos} and {@code random} parameters should not be
     * put directly into the snapshot. Only the relevant information should be extracted
     * so that the same snapshot is yielded as often as possible.
     *
     * <p>The default implementation returns {@code null}, meaning the model does not implement this method.
     * A model that wishes to override this method must therefore not return null,
     * even if the model does not use any passed in state.
     *
     * @return a snapshot of the data used by this model; can be any object as long as
     *         it implements {@link Object#hashCode()} and {@link Object#equals(Object)} correctly.
     */
    @Nullable
    default Object snapshotUsedData(BlockAndTintGetter level, BlockPos pos, BlockState state, RandomSource random) {
        return null;
    }

[hardy talon]

The method name sounds weird to me but other than that this looks very good. I currently can't think of anything better either though 😅

[spare bloom]

yeah it's not a great name

we will need to think a little about the default implementations (especially MultiPart)

as a semi-unrelated note, I also noticed that the composite model does not reseed the random for every submodel

that sounds bad

as it currently is, if the first model uses the random, it might affect subsequent models

[hardy talon]

We can just copy the solution used by MultiPartModel where it queries the random source and then uses that value to re-seed the random source before collecting each of its part models' parts

[hardy talon]

The easiest solution would probably be to do something similar to what MultipartModelData previously did and collect the results from the part models into a list or map

[warped sluice]

I agree

[spare bloom]

yeah

we need to put the selector BitSet in the snapshot too

[hardy talon]

True

Should still return null though if none of the part models return a non-null result

[spare bloom]

if any submodel returns null we have to return null

[rocky vector]

ah yeah otherwise it caches when it shouldn't potentially

[hardy talon]

Hmm, true

[spare bloom]

I have this for composite:

    @Override
    @Nullable
    public Object snapshotUsedData(BlockAndTintGetter level, BlockPos pos, BlockState state, RandomSource random) {
        List<Object> snapshot = new ArrayList<>(models.size());
        for (var model : models) {
            var subSnapshot = model.snapshotUsedData(level, pos, state, random);
            if (subSnapshot == null) {
                return null;
            }
            snapshot.add(subSnapshot);
        }
        return snapshot;
    }

(this is how I noticed the random seed issue)

[rocky vector]

what prevents someone from throwing the level into the snapshot

[hardy talon]

The docs

[rocky vector]

[hardy talon]

If someone does, then the ATM memory leak police will arrest them

[spare bloom]

consider that lazy devs will hopefully just not implement the method

(and then do it correctly if they realize that they need to 🤞)

[hardy talon]

Yeah

[warped sluice]

Now I need to add this method to Fabric too since it's actually happening

I'm a bit iffy on the Nullable and the name but otherwise it's perfect

Actually no, I forgot about one thing

I mentioned the cullTest before but I think everyone missed it. Can it be considered for addition as a collectParts and snapshotUsedData parameter?

[spare bloom]

IMO it's weird and we already have too many arguments in that method

in principle you might be able to defer processing of some transforms until BakedModelPart#getQuads is actually called for a specific direction

[warped sluice]

I was just about to say that. I suppose that's good enough since dynamic geometry requires creating dynamic parts anyway.

[spare bloom]

yeah; with FRAPI it's a different story

[warped sluice]

The param is still necessary on Fabric so I'll add it as a param to the key getter

[spare bloom]

the name is really not great and should be improved

[warped sluice]

I'd like createGeometryKey better

[spare bloom]

I think that key is not great

geometry is not ideal but I could live with it

well at least it is better than "data" which is very vague

[warped sluice]

That is really the returned object's only purpose so I don't see why

[hardy talon]

That or createGeometryCacheKey() sounds good to me

[warped sluice]

I finally started porting FRAPI and the new vanilla model pipeline has an issue: all cull checks are re-run per part instead of per model like before. Running them per part is not necessary as the result does not depend on the part.

FRAPI does not care about this as it replaces the pipeline but it might be an issue for Neo

[warped sluice]

Also this actually doesn't work because the list is retrieved before the cull check, unlike in previous versions

Neo could probably patch the vanilla logic relatively easily to solve both of these issues

[hardy talon]

Yeah, I noticed that as well

[random pebble]

You could report this to Dinnerbone

Since that has perf-implications for them too, doesn't it?

[warped sluice]

I pinged Boq about it in the Fabric server already, but they have not responded as of now

[random pebble]

ok

[rocky vector]

We should definitely patch that if Mojang does not

[rocky vector]

Are you sure that wasn't the order in previous versions too?

[hardy talon]

You'd be correct, that's how it was previously as well

[warped sluice]

Hm I feel like Neo should have patched that

That could be pretty bad for performance of dynamic models

[spare bloom]

For trivial models the cull check is more expensive

[rocky vector]

Trivial models generally have quads on all 6 sides anyway

excluding non-fullblock models with no cullfaces, but I think those models are statistically much rarer

[spare bloom]

Yeah it's a bit weird

[rocky vector]

Tbf for trivial models the cost of getQuads is basically zero (a map lookup/field read) so changing the order shouldn't impact performance

[warped sluice]

I guess FRAPI's lazy cull checking is practically a perfect solution then

Yeah

[spare bloom]

There's a small hit for each quad but it's not too bad yeah

[warped sluice]

Will Neo add a utility method that accepts a BlockStateModel and Function<RenderType, VertexConsumer> to buffer it? The vanilla method now requires a list of parts. I plan to add such a method to FRAPI.

[spare bloom]

we add some methods here and there, not sure if such a method is missing

[warped sluice]

At least a version with a buffer func instead of a single buffer is necessary

I'd check Kits but it's private

[last wave]

I can understand the appeal, but my question then is, necessary for what?

[warped sluice]

Even for the chunk builder

Users certainly would like to easily buffer any BlockStateModel into the correct, separate VertexConsumers outside of chunk building

Neo has the same use case for things like moving and falling blocks

[spare bloom]

In ModelBlockRenderer we add this overload:

    public void renderModel(
        PoseStack.Pose p_111068_,
        net.minecraft.client.renderer.MultiBufferSource bufferSource,
        BlockStateModel p_405848_,
        float p_111072_,
        float p_111073_,
        float p_111074_,
        int p_111075_,
        int p_111076_,
        net.minecraft.world.level.BlockAndTintGetter level,
        BlockPos pos,
        BlockState state
    )

And in BlockRenderDispatcher:

public void renderSingleBlock(BlockState p_110913_, PoseStack p_110914_, MultiBufferSource p_110915_, int p_110916_, int p_110917_, BlockAndTintGetter level, BlockPos pos) {

[last wave]

A multibuffersource would completely suffice in that case

[warped sluice]

Yeah that's basically equivalent to the function

The only possible difference is that it may be desirable to make the returned VertexConsumer Nullable to signify that quads with this render type should not be buffered

[warped sluice]

Alright thanks

[last wave]

I thought of that, but that is not really, at least in my opinion, something the model should care for.

In my opinion it is up to the renderer to handle that case gracefully.

[warped sluice]

The model never receives the VertexConsumer so I don't understand what you mean

The model will still output all geometry, so what to buffer would indeed be up to the renderer

[spare bloom]

Is that usual? Only rendering a subset of the quads?

[warped sluice]

Personally I don't think I have a use case but it is trivial to support in the implementation

[warped sluice]

Boq might have the Fabric server muted or something

[spare bloom]

@silent Hey @stiff fossil are you aware of this yet?

Grr silent doesn't work on my phone sorry

[stiff fossil]

ping? banned from Minecraft

[spare bloom]

I should update the app, probably 😄

[rocky vector]

next snapshot will explicitly block your UUID from joining servers /s

[spare bloom]

the geometry key is only valid when compared against geometry keys from the same model, right?

in other words, geometry keys from different models with different geometry might be equal

[hardy talon]

Yes

[spare bloom]

alright, pushed the first iteration

the diff is here for @warped sluice: https://gist.github.com/Technici4n/bc70438783928b1bf1a6e99eb8f7127c

an implementation for WeightedVariants is still missing!

I thought it was gonna be very annoying, but tbh maybe it should just reference the selected model directly 🤔

[warped sluice]

What's WeightedVariants?

[spare bloom]

it's the weighted model

    @Override
    public void collectParts(net.minecraft.world.level.BlockAndTintGetter level, net.minecraft.core.BlockPos pos, net.minecraft.world.level.block.state.BlockState state, RandomSource p_409649_, List<BlockModelPart> p_410123_) {
        this.list.getRandomOrThrow(p_409649_).collectParts(level, pos, state, p_409649_, p_410123_);
    }

[warped sluice]

Oh ok

I mean the impl should be list.getRandomOrThrow(random).getGeometryKey(...) right

[spare bloom]

it should also capture which model was selected I think

since the current impl of SingleVariant#createGeometryKey is return Boolean.TRUE;, you cannot distinguish between different variations being selected for example

this might be an option but I'm not the biggest fan

the equals and hashCode overrides to force identity comparison are probably overkill

(also, it might be a bad idea to put an entire model in the cache key? )

[hardy talon]

I don't think SingleVariant should implement that method since it's not sensitive to any dynamic state (i.e. level, position, model data or random)

[warped sluice]

That seems fine

[spare bloom]

then it means that any model using SingleVariant will return a null geometry key?

not implementing the method means "we don't know if the model is sensitive to dynamic state"

returning the same object all the time means "we know that the model is not sensitive to dynamic state"

[hardy talon]

It also means that callers of this method cannot have any actually useful fast paths for the case of not caring about dynamic state. Currently, when I encounter a camo model for which no CT data is available, I use the CamoContent directly as the cache key which avoids the key allocation in a significant amount of cases.

[spare bloom]

not sure what you mean, I am not familiar with the inner workings of FB

[hardy talon]

If a camo's model provides no connected textures data that I can extract, then the CamoContent (for simplicity's sake think of it as an abstraction over BlockState to allow non-block camos) is used as the cache key directly instead of constructing a key with the CamoContent and null CT data

[spare bloom]

we can always define a specific object that should be used for static geometry, but it starts to complicate this function (and potentially the default implementations) even more

[rocky vector]

allocation of subKeys should be deferred till after you see the first non-null subkey, otherwise createGeometryKey is guaranteed to allocate when called on a multipart/composite model

[spare bloom]

True but in general the model should be able to compute a key. All vanilla models will return a nonnull key

[hardy talon]

The problem with distinguishing between static geometry and models we're not sure about is that it makes the method basically required to implement which should ideally not be the case considering that this is a niche feature

[spare bloom]

So what do we do? Accept the allocation for MultiPartModel and WeightedVariants?

[rocky vector]

What if we strengthen the conditions on the returned key by requiring it be globally unique (and not just unique to that model)

My thought is to allow multipart to detect all the submodels returning the same simple static key and just return it itself, instead of having to allocate a list to hold all the static keys

[warped sluice]

If the key is globally unique then can each SingleVariant just return itself as its key?

[spare bloom]

Multipart always has to allocate since the result depends on the block state

Well maybe it could return its submodel list if they're all simple 🤔

[warped sluice]

I don't think it's common for a multipart to have multiple submodels return the same global/class-global key because that would mean it's duplicating the exact same geometry

[warped sluice]

If it selects only one model then it doesn't. With global keys, returning that single submodel's key directly would improve the quality of the key as it would now work for multiple models.

[warped sluice]

If each SimpleVariant returns itself as its key then this would be the case anyway

[spare bloom]

Yeah

Right now they all return Boolean.TRUE 😛

[warped sluice]

It seems global keys are a good idea but writing clear documentation about what is allowed and expected may be a bit tricky

Mostly for the reason that even though the keys are globally unique, they are not expected to have perfect quality globally, but are expected to have perfect quality across the same class of model

[warped sluice]

At the same time this may not be a good idea because there is no contract that says that a List<BlockStateModel> key requires a simple composition. A custom model may return such a key but do more than a simple composition, which could conflict with multipart models' keys.

It is necessarily the case then that a multipart model that returns more than one submodel for a certain context would need to include a unique object (like its class object) in its key for that context

[hardy talon]

I don't think enforcing globally unique keys is realistic or even particularly useful. If you're caching, then there will always be a BlockState (or BlockState-like object) involved in the final key since that's how models are looked up, so the computed key will always be combined with something that already uniquely identifies the model

[warped sluice]

To establish some terms and definitions:
Instance-local geometry keys: Geometry keys that can only be compared with geometry keys produced by the same model instance.
Class-local geometry keys: Geometry keys that can only be compared with geometry keys produced by any model instance of the same java Class.
Global geometry keys: Geometry keys that can be compared with geometry keys produced by any model.

[hardy talon]

I think instance-local is sufficient

[warped sluice]

That was the original idea but we got to global somehow; maybe class-local would be a good middle ground

I agree with you that global keys shouldn't be implemented because while they allow forwarding a single submodel's key, saving an allocation and improving the quality of the key, that may not be that useful and correctly implementing global keys is tricky, as seen with the multipart example above.

[hardy talon]

I don't think class-local gains much over instance-local because the caller already needs to combine the potentially absent key with something that uniquely identifies the model instance to create the final internal cache key

It's not useful for the model whose geometry is being cached to take over that task because it has no idea what requirements the caching model has for the final cache key

[warped sluice]

Is that necessarily the case or is that just what Framed Blocks does?

If I were to implement caching in Continuity using this same geometry key idea which will be added to FRAPI, I only care about the geometry, so I don't think I would need to add the model instance to the internal key (unless the key is instance-local by design, which would force me to do that)

[hardy talon]

It is necessarily the case because there will inevitably be models that return a null key unless we make the method abstract and therefore make its implementation required. But that's basically guaranteed to cause an outrage.

[warped sluice]

Hm, I don't think this applies to my Continuity example because when geometry is requested, the model would get the key of the singular submodel, and if it's null, invoke the transformation logic dynamically, or if it's not, use it in a cache with a computeIfAbsent-like getter. If keys are instance-local, the cache would be stored in the model wrapper; if keys are class-local, the cache would be shared and retrieved through some global map that uses class objects as keys; if keys are global, there would be a single shared cache.

[spare bloom]

Not sure what you mean here

You shouldn't need to add the block state to the cache key

[craggy hazel]

and also that won't stop anyone from returning null

[warped sluice]

With these changes to models, is there any reason to keep BE model data as specifically a map? If it only comes from one place now (the BE), then using Nullable Object like Fabric should work too.

[hardy talon]

The composition is still useful when both the BE and the consuming model can be extended with additional behavior. In FramedBlocks for example, the "base" BE adds an object holding the primary data (camo, culling data and a few flags) and a few special BEs add an additional object that's specific to their model (i.e. the pot adds its contained plant, the collapsible blocks add their "offset" data, etc.).
Also, with all due respect, dumbing something that's explicitly intended to be introspectable by the caller down to Object is awful API.

[spare bloom]

Object is actually fine API IMO

The biggest issue is indeed around composition, which I also intend to make use of

[last wave]

Yeah lets keep this a map. Much better for introspectibility and composition

[rocky vector]

There's also no longer a strong performance argument to make it not a map since it's only constructed by BEs now

[last wave]

Yes 100%

But the point is: the map has many other advantages, and if you really want to store an arbitrary object in it, you still can.....

[stray fog]

make it data components /hj

[warped sluice]

That makes sense

I see the utility, even if it is somewhat niche, because you control the model and BE subclasses, you can plan out the type of the object to support the composition use case. The only case I see where it wouldn't work is when the model and BE are extended by a third party but the model data type is not composable.

I am fine with how it is now

[hardy talon]

In my case both the model (there's an asterisk here but that's not relevant to this "problem") and the BE are intended to be extended by a third party. Due to the existence of certain double blocks, it's also not really feasible to do the composition at the level of the primary data object

[warped sluice]

What do you mean by the second part?

[hardy talon]

There are a few double blocks that consist of single blocks which need additional data. Providing arbitrary additional data from both a single-block's BE and a double-block's BE to the single-block's model without having generic composition via a map becomes a case of either jamming the additional context into the primary object with generics and unchecked casting or it becomes inheritance hell.

[warped sluice]

Alright

Does Neo do anything in LevelRenderer in the method that renders a block outline about the call to get the blockstate's render type?

[warped sluice]

It would be very nice if Mojang could move PistonHeadRenderer.renderBlock into BlockRenderDispatcher or ModelBlockRenderer and reuse that method in FallingBlockRenderer as well as SectionRenderDispatcher if an overload that accepts a List is made

[spare bloom]

We patch them to call the right overload at least

[warped sluice]

That's more difficult to do on Fabric as the render type retrieval, random creation, seed retrieval, random seeding, and model part retrieval (and model retrieval, if it's not assigned to a local) all should be cancelled for performance

My suggestion would still help Neo by reducing the amount of patches necessary, and would help mods by not requiring them to rewrite that logic

[spare bloom]

I doubt these things matter all that much

[warped sluice]

Not really but it seems like a trivial change

[hardy talon]

ChunkRenderTypeSet be gone

[warped sluice]

At last

[warped sluice]

In the impl of this method, do you get the default layer from the passed blockstate? How does the default layer work in this new Neo system anyway?

[hardy talon]

The impl calls BlockModelPartExtension#getRenderType() with the provided BlockState and uses the result for the buffer lookup. The default impl of that extension method calls into ItemBlockRenderTypes.

[warped sluice]

Does each part have to re-retrieve the default layer if that's what it uses?

[hardy talon]

Currently it does, yes

I'm not sure there's any good way to optimize away that repeated lookup, for two primary reasons:

• LeavesBlock special-casing for fast vs. fancy
• Doing so would almost certainly be cleanest during model baking but mods which specify render types in code almost certainly do so in FMLClientSetupEvent which currently fires kind of awkwardly in the async phase of the first resource reload

[warped sluice]

BlendMode would also fix it but would change the API

[hardy talon]

I don't see how BlendMode would resolve either of those blockers. The registration timing would still be an issue and leaves would still have to fall back to ItemBlockRenderTypes to get their actual render type depending on the fast/fancy flag

[warped sluice]

BlendMode allows the caller of collectParts to get the default render type just once

[hardy talon]

Fair enough, I guess. If we wanted to put this burden on the caller, then that's trivial to do with our extension as well by just making it nullable and returning null by default

[warped sluice]

True

The caller is already burdened more than in previous versions as seen here #1342994966405845063 message

[rocky vector]

I don't know if we strictly have to fix this, it seems to me like yet another reason for modders to use data-driven render types instead of ItemBlockRenderTypes.setRenderLayer

[warped sluice]

It affects the performance of vanilla models

[stiff fossil]

hi

[warped sluice]

Hi

[stiff fossil]

bye

[warped sluice]

I'll take a look at the full source in a minute

[last wave]

Why do I feel like models changed again..........

[rocky vector]

ModelBlockRenderer became partially static

don't think that's a huge deal, just noting it

otherwise I don't really see any changes except to its implementation, which shouldn't affect our patches much

[pulsar lantern]

mhh yes just casually setting off some tnt and moving on

[warped sluice]

WRT models, only the internals of ModelBlockRenderer received changes

[stiff fossil]

fizzle

[warped sluice]

I was hoping most results of calculateShape would be stored in BakedQuad because it's not necessary to redo all those calculations every time a quad goes through the pipeline (except the ones based on BlockState, but that can be moved to inside the main AO calculation and combined with the static flags from the quad)

The tint cache is interesting; I suppose it would help a lot with grass blocks

Though the tint color is actually cached in the ClientLevel anyway so I'm not sure

[modern vapor]

as one does, of course

[hardy talon]

Looking through the ModelBlockRenderer changes, they solved the issue of checking face occlusion per part instead of per block with two bitmasks, one for "was face checked" and one for "is face visible"

[spare bloom]

Ok yeah that's also how Indigo does it

[warped sluice]

Do they still get quads before doing the check?

[hardy talon]

Yes

[warped sluice]

Mistyped, meant quads of parts

[hardy talon]

The quads are still retrieved before checking occlusion unless the face has already been checked

[warped sluice]

Neo should probably still patch that

[hardy talon]

Why? Getting the list of quads from the part is almost guaranteed to be faster than calling Block.shouldRenderFace()

[warped sluice]

Maybe in vanilla cases but as discussed before a dynamic part may want to wait for the getQuads call in case it does a lot of expensive transformation

[warped sluice]

Also check isn't performed only if the list is empty, which is rare

Or maybe it's always performed anyway; I can't remember

[hardy talon]

What the code is doing now is basically this:

for parts {
  for sides {
    if checked && !visible { continue }
    
    quads = part.getQuads()
    if quads.empty() { continue }

    if !checked {
      visible = shouldRenderFace()
      checked = true
    }
    if visible {
      renderQuads()
    }
  }
}

[warped sluice]

The only case where the patch would make performance worse is when all quad lists for a specific face across all parts are empty

Maybe for cross models that wouldn't be great

[stiff fossil]

it's per direction, so you increase a cost of slab by 1/6th
a cost of a flower pot by 5/6th

argument that "some models might potentially do heavy calculations" ignores that vast majority of models are "pretty dumb"

[warped sluice]

True

I suppose passing the cullTest to models/parts would be the only perfect solution

[spare bloom]

It should probably be passed to parts however this means allocating a combined list or something like that

[restive badger]

Will this be finished in the initial release of 21.5.0? Asking for documentation reasons

[hardy talon]

It kinda has to be 😅

[queen elm]

It has to, and we should prioritise this over a same day release

[spare bloom]

I think the only things that still need doing are:

• figuring out the remaining details of this geometry key
• custom block state definitions but that can also wait

[warped sluice]

What does Neo's extended BlockStateModel.getParticleIcon method look like? Does it receive the state and if so, does that parameter have the same utility as the state param in collectParts? Does it receive a ClientLevel instead of a BlockAndTintGetter?

[violet elm]

At least historically I think it was mainly to receive the model data (I forget if it has one or two parameters as the overload though)

[warped sluice]

Historically it's only received the ModelData produced by the model but that concept no longer exists in 1.21.5 so I am wondering what is on Kits

[violet elm]

I believe TextureAtlasSprite particleIcon(BlockAndTintGetter level, BlockPos pos, BlockState state)

But browsing the code on my phone is annoying

[spare bloom]

Sounds about right yes

[warped sluice]

That is exactly what I expected so my later questions here apply #1342994966405845063 message

[spare bloom]

Not sure what to answer really

There isn't a reason not to provide the state

[warped sluice]

What about providing a ClientLevel or Level instead of BlockAndTintGetter?

[last wave]

I don't really see a practical reason to do so

I would keep it to XYZGetters if we don't have a practical reason

[spare bloom]

BlockAndTintGetter is consistent with collectParts

[last wave]

I would leave it at the Getter level for now for sure

Unless pepper can come up with a good reason as to why it needs to be anything lower in the type tree

[warped sluice]

I do not have a specific use case in mind that couldn't be covered by the getter

[warped sluice]

However, I think it's worth considering moving this API to get the value from the model instead (probably the tint index, where -1 means that particles shouldn't be tinted), as I think that makes more sense.
https://github.com/neoforged/NeoForge/blob/1.21.x/patches/net/minecraft/client/particle/TerrainParticle.java.patch#L8

[spare bloom]

Probably yeah

That hook is quite ancient

[warped sluice]

Should exactly should the API work?

Is it sufficient to add a param-less boolean getter to the model?

[spare bloom]

Hmm

This controls tinting which models only have limited control over

[warped sluice]

Actually not param-less but accepting a state so that the default check can be put there

Really the API seems of limited use to me because a modder can simply choose to reserve tint index 0 for the particle only and control whether the actual tint is returned or -1 is returned based on the context that the block color provider receives

Custom subclasses of the grass block seem the most likely to need the API and that's already really niche

[wide oak]

haven't really been updating myself on what's going on with 1.21.5

is ModelData just gone?

[last wave]

Yes and no

It's scope is reduced to block entity data only

And it can be retrieved from the batgetter

[warped sluice]

That scope is equivalent to the scope of Fabric's BE render data now

[last wave]

Exactly

The idea is still that people use it because BEs are completely not thread safe

[hardy talon]

@spare bloom I just noticed a rather annoying detail about the custom blockstate model stuff: custom definitions (i.e. taking over the entire blockstate file) would be trivial to datagen by implementing a custom BlockModelDefinitionGenerator. Custom unbaked blockstate model types however can currently not be datagenned because both MultiPartGenerator and MultiVariantGenerator (the two vanilla implementations of BlockModelDefinitionGenerator) are deeply hardcoded to deal with Variant/MultiVariant

[spare bloom]

pfff I thought that the datagen was finally using the same code as the actual model loading

too bad I guess, we'll have to find a reasonable way to datagen those

[hardy talon]

It does for the most part except for some builders and those are particularly hardcoded

[spare bloom]

PropertyDispatch is horrendous

we could always subclass Variant, might be an option

[hardy talon]

That's a record

[spare bloom]

wait I was looking at 1.21.4 code lol

[hardy talon]

It is, but it's miles better to actually use than our old custom blockstate datagen, at least IMO

[spare bloom]

ok so first thought is that we can patch in an additional field in MultiVariant

not sure what to do about the VariantMutator though

ok yeah patching in an additional field in MultiVariant should be enough for MultiPartGenerator

[hardy talon]

The custom thing could have a method which takes the mutator and then decides what it wants to do with it. That way a custom thing that is basically a multi-variant with additional metadata can handle the wrapped variants properly

[spare bloom]

yeah maybe something like that

MultiVariantGenerator doesn't seem too hardcoded to VariantMutator which is nice

we can always add with overloads with a different "mutator" interface that will work with custom models

[hardy talon]

Sounds reasonable to me

[spare bloom]

happy to have a look but I don't want to do it 😇

[hardy talon]

Heh. I might take a gander at implementing it later.

[spare bloom]

@rocky vector @warped sluice @hardy talon I think I will make the geometry cache key global (i.e. unique across models unless they render the same). And document that returning a model gives by convention a completely static model

I wonder if the AO of the model should be put in the key. That would complicate things even more

[rocky vector]

imo no, models that rely on that much world state are probably not worth caching and should opt out of caching

[warped sluice]

There is no top-level AO getter that accepts world state so I don't see the point

[spare bloom]

Is there none? Hmm

[warped sluice]

Also as we discussed with XFact I don't think global keys are a good idea because that kind of expects a model impl to know about every other model impl which is not realistic and the only reliable way to create a global key is to automatically bundle this with whatever data because there is no (and probably should be no) standardized format for what a key means if it's of a certain type

[warped sluice]

The first part determines AO but that is determined by collectParts which is what the key is already for, basically

[warped sluice]

Class-local keys seem like a better idea at first but fall apart when you introduce model wrappers, so I think just using instance-local keys is fine. Another argument for instance-local keys is that they are simplest, which is good because this whole concept is already likely a bit difficult to understand and correctly implement if you're seeing it for the first time.

[spare bloom]

Class local means that MultiPart has to allocate an object

Same for Weighted

Whereas global means that they can just return whatever key the selected submodel returns

(MultiPart with multiple active parts of course still has to allocate)

[warped sluice]

If the keys are global, an allocation might be saved, but it means that keys of type List mean something specific and if a custom model doesn't know that it might return a key of that type thinking it means something else. It's exceptionally easy to make that mistake, and it requires somehow standardizing what certain types mean.

[hardy talon]

I would argue that weighted models are transparent as far as the key consumer is concerned (assuming of course that the consumer can even deal with the randomness at all cough cough)

[warped sluice]

Really all of these approaches have issues

Global keys can result in the highest key quality and are easiest for model impls to deal with when they have submodels (whose geometry isn't transformed) but the key type problem is really an issue that I don't see a solution to

[spare bloom]

No it's not transparent

Different submodels might return the same key for different geometries

A simple example is the basic part wrapper returning Boolean.TRUE currently

And it is completely valid to return that because the key is assumed to be model-local

[hardy talon]

Fair enough

[spare bloom]

That's why I don't like this approach

I feel like it's even easier to get it wrong than with global keys and it means more allocations

We should just pick either approach and run with it

[warped sluice]

Ok let's look at the exact impl then

With global keys, simple models would return themselves, variant models would return the key of the current delegate, and multipart models would return the key of the delegate if there is exactly one delegate, otherwise a unique type that wraps or subclasses a list of all delegate model keys with appropriate equals/hashCode impls. If the list is returned directly, that can cause issues, as I previously said.

[spare bloom]

OK I made the key reusable across models

I think it's cleaner

should we remove the implementation of the method in DelegateBlockStateModel? I think it's very easy to forget to override it