1.21.4 Snapshots

[quick cairn]

Sounds good, was just making sure who to bother when I put up the docs freeze to address the changes

[rich rune]

Re: docs freeze, I hope I can get the entity Pr in before that

[quick cairn]

It’s possible

[teal patrol]

New version detected: 1.21.4-rc2.

[ashen cedar]

<@&1067092163520909374>

[gleaming apex]

only one bug fix

[remote fractal]

hwat

[junior epoch]

[swift linden]

Changelog: https://www.minecraft.net/en-us/article/minecraft-1-21-4-release-candidate-2
SnowMan: https://github.com/neoforged/Snowman/commit/ca73c73e0ae31e772b69b1d6657ca61c1475c2a9
Primer: N/A

Slicedlimes Videos:

• Main: https://www.youtube.com/watch?v=34hnq35hRgI
• Data/Resource Pack Changes: N/A

[opaque cliff]

https://www.minecraft.net/en-us/article/minecraft-1-21-4-release-candidate-2

[remote fractal]

early one

[rich rune]

oh no

[swift linden]

snowman linked in pin above

[teal patrol]

1.21.4 Release Candidate 2

Today we are releasing 1.21.4 Release Candidate 2 to activate Blending around the Pale Garden Biome.If no further critical issues surface, this is the version we are planning to ship as Minecraft: Java Edition 1.21.4.Happy Mining!Fixed bugs in 1.21.4 Relea...

Nov 29, 2024, 11:58 AM

[thin dawn]

How many patches are missing?

[hidden ravine]

33 patch rejects left

30/33 of them are for client-side classes

[royal solar]

we need to make it compile more than anything

[royal solar]

nice coeh made it compile overnight 😄

[thin dawn]

HOw are 30 patches then missing?

Ah okey I get it 😄

needs more tea

[royal solar]

did you see what I did last night

[thin dawn]

Not yet

[royal solar]

here's a little hint

[thin dawn]

Currently trying to figure out how to get a DependencyCollector attached as an extension to a collector?

[thin dawn]

Oh yeah I saw that

Meh needs refactoring

We already saw that comming a mile away

Because of the new render state

[royal solar]

yeah

re item models I wouldn't touch them for now, vanilla's stuff seems reasonably extensible

it would be nice to restore the baked model hooks though

[thin dawn]

I would at best refactor them to have the same functionality

[thin dawn]

The ones for multiple rendertypes and models per render type

Yeah

[royal solar]

my thought was to make them work as closely as possible to what we had before

[thin dawn]

Those are really nice

Probably

[royal solar]

no time for a larger refactor, and it might change again soon

[thin dawn]

100%

[royal solar]

I'm happy if you do it and I can just review it lazily later

[thin dawn]

LOL

Maybe

I made the original ones with ama

Depends on how fast I can get this implemented into Tableau

[royal solar]

in principle that should be achievable through BlockModelWrapper

i.e. BlockModelWrapper should support our extra hooks

and map them to the new item rendering system

[thin dawn]

Probably

they where in the past mapped in LevelRenderer

So it should be pretty simple to find the prototype location of that call

[thin dawn]

@hidden ravine Did you push?

@royal solar What is the branch you guys use?
I tried 1.21.4-pre1 and I get 180 rejects

NVM

I am being an idiot

[royal solar]

did you try to run setup?

[swift linden]

he did yes, ran out of habbit of opening normal repo

[royal solar]

ah yes

happened to me too 😅

you might want to run :base:setup to get vanilla sources though

(and you'll need it to be able to run vanilla)

[ashen cedar]

seems like we'll need to delete our getPickResult entity extension with the hit result

[royal solar]

the block one is also compromised

[ashen cedar]

picking is now a packet and vanilla doesn't transmit the hit result to the client

the block one isn't?

[royal solar]

you don't get a hitresult anymore for block

[ashen cedar]

oh sure

i removed the hitresult

[royal solar]

👍

[ashen cedar]

still needed for player context

[royal solar]

yeah

[vagrant saffron]

does the thing where the client does a raytrace every frame and store the hit result clientside still exist? or is that gone

[rich ferry]

I would imagine. It was used for f3 debug info and for rendering the punch cooldown indicator iirc

[fleet hatch]

That still exists but it doesn't help you there because picking is server-side now

[vagrant saffron]

nah I was using the clientside hitresult to handle "break specific segment of a wire block" so I'm good

[quick thunder]

Does this mean multiple clientside mods all need to do ray tracing themselves?

[fleet hatch]

No, the field on the client still exists

[vagrant saffron]

and I was already manually doing raytracing on the serverside because as far as I knew it wasn't actually stored anywhere on the server and I had to do it myself

[fleet hatch]

Yeah, on the server there was never a raytrace result available, you always needed to do that yourself

[ashen cedar]

are you sure?

[thin dawn]

It compiles for me

[fleet hatch]

Did you build or run?

[royal solar]

it compiles with intellij for me

I haven't tried gradle 😄

make sure you sync the project again if you haven't yet

@thin dawn why new java.util.concurrent.ConcurrentHashMap<>(); there's no need for a concurrent map

[fleet hatch]

My guess is that maty pressed build instead of run, in which case it also compiles tests

[royal solar]

but it fails in :neoforge:compileJava which does not compile tests 😛

[fleet hatch]

Right, nevermind

[ashen cedar]

the gradle run doesn't work either..

[royal solar]

what is even the compiler error?

[ashen cedar]

it's not one error

it's more than 100

[royal solar]

ah you probably have leftover files from another MC version

git clean -f -n -d src/projects/neoforge/src/ should show some files

if that looks correct run it again without the -n flag to actually remove stuff

and same for base

[thin dawn]

That is a mod event bus.... parallel processing

[fleet hatch]

Doesn't matter, only ParallelDispatchEvents are actually dispatched in parallel, everything else is fully serial

[thin dawn]

Ah

Okey

That was changed then

Let me fix that then

[ashen cedar]

that was always that way

[thin dawn]

Ah....

Sorry

Fixed

[ashen cedar]

tech what's the state of the data reject

[royal solar]

haven't looked at it yet but we have to merge an FML PR first

I'll have a look later to actually run the game with both data runs

[ashen cedar]

ah i thought you did the datagen

[royal solar]

I haven't touched it, and I'm happy not to

in principle we need to split the events

[ashen cedar]

did i say i hate mojang for this yet

[vagrant saffron]

I'm 100% committed to doing all my datagen on one side, even if it turns out client datagen refuses to generate stuff in the data folder and I need to use mixin to fix it

[ashen cedar]

i don't think we need a new event

event.addProvider(event.isClient(), () -> new Provider(....)

[royal solar]

so, I always found this isClient/isServer stuff quite stupid

[vagrant saffron]

concur'd!

[royal solar]

I think it's cleaner to separate the events than to have to pass a boolean every time

[vagrant saffron]

NO

the direction I thought you were going was "and so we should just ignore the boolean and generate everything"

[quick thunder]

Is there even a use case for dedicated server datagen? If it is basically integrated server when datagenning, then there isn’t much reason to not datagen everything at once. Can’t think of a result where datagenning all could break server side prod

[royal solar]

we should do as mojang does

[vagrant saffron]

I've never done datagen the way mojang does and I'm not going to start today

[royal solar]

so TG I suppose you can ask mojang why they're doing it this way

[vagrant saffron]

the way I have it set up is I take a block or item or whatever and I use a helper to define all the files for that block

[royal solar]

it turns out that in their case you can actually run datagen on the dedicated server

[quick thunder]

Ok. But is there an actual reason to separate client/server datagen that has real world modder impact

[vagrant saffron]

so instead of one dataprovider per file type I have one dataprovider per block

[quick thunder]

If we are running datagen in our ide, and that’s the only way we run datagen in neo with both client and server sources, what’s the point of dedicated server datagen?

[royal solar]

I don't think so since we don't support running datagen in prod

[lethal panther]

Do they call it data pack vs resource pack generator at all?

Isn't that the distinction

[ashen cedar]

call where

[lethal panther]

I just mean naming in mojangs own code

[ashen cedar]

?

[royal solar]

resource pack is ambiguous

[lethal panther]

Is it?

[royal solar]

it is both the client-specific resources, as well as a general term that can either refer to client-specific resources or server-side data

(internally at least)

[lethal panther]

I thought one generates assets/ the other data/

[royal solar]

yes

but classes like PackResources are used for both

[lethal panther]

So user facing one generates resource packs the other data packs

[ashen cedar]

it's still the provider that decides where to save

[teal patrol]

New version detected: 1.21.4-rc3.

[ashen cedar]

<@&1067092163520909374>

it doesn't stop

[burnt parrot]

[Reference to](#mojira message) #mojira [➤ ](#mojira message)This ticket has just been resolved as Fixed!

matyrobbrt referenced · Nov 29, 2024, 4:36 PM

Hoimar Bauer

[MC-276962] JVM crash occurs when minimizing maximized window

How to reproduce

1. play maximized but not in fullscreen a world.

Status

Resolved as Fixed <t:1732898155:R>

Fix Version

Future Update

Assignee

[Mojang] Timur Nazarov

Votes

6

Comments

15

Duplicates

1

Created

<t:1727291475:R>

Quoted

[north fox]

Mid night snapshot!!!

That was quick

[deft kindle]

They're very excited about getting all the bugs fixed

[sand vine]

well, 18:23 here

[swift linden]

Changelog: https://www.minecraft.net/en-us/article/minecraft-1-21-4-release-candidate-3
SnowMan: https://github.com/neoforged/Snowman/commit/50e28f56ca60275db23c9ce4cff42f74f4cfa6f1
Primer: N/A

Slicedlimes Videos:

• Main: https://www.youtube.com/watch?v=34hnq35hRgI
• Data/Resource Pack Changes: N/A

[deft kindle]

It's noon for me so I just get to chill

[sand vine]

here being the same timezone as mojang

[deft kindle]

What time do they get off work

Was this like a push and leave

[royal solar]

18:23 on a Friday

dangerous

[thin dawn]

😛

[north fox]

00:25 AM

[thin dawn]

@royal solar I got most of the model stuff done

Went a bit nuclear, and yeeted UnbakedGeometry and all associated bullshit

But it is now like 1000x cleaner

[wispy tangle]

MOAR CHANGES

[north fox]

Created : 2 months ago

[royal solar]

well that's not too reassuring

[thin dawn]

Why?

[white plume]

Nice, that's what I was hoping for

[thin dawn]

UnbakedModel is a very nice API

[oblique granite]

rc3

[thin dawn]

And extremely similar to UnbakedGeometry

[oblique granite]

as much as it may not be as useful now as it was before, I'm a tiny tiny little bit offended that you'd call my work (later refactored by amadornes) bullshit ;P

[swift linden]

added snowman link

[thin dawn]

Mine too

I worked on this just as much 😄

XD

But it was more or less bullshit in the context of this update

It was basically fine before

[oblique granite]

let's call it redundant instead ;P

[thin dawn]

It was redundant before

It was just broken now

[oblique granite]

obsole, stale, maybe a bit moldy

[thin dawn]

I will take moldy 😄

[white plume]

UnbakedModel might still need to be patched/extended to allow overriding the render type and composing the part visibility set but a whole separate system shouldn't be necessary anymore

[thin dawn]

As I find that a very accurate situation

[teal patrol]

Minecraft 1.21.4 Release Candidate 3

What's better than shipping on a Friday? How about shipping twice on a Friday?!Here's Release Candidate 3 for Minecraft 1.21.4, fixing some critical issues, including several crashes.Fixed bugs in 1.21.4 Release Candidate 3MC-276962 JVM crash occurs when m...

Nov 29, 2024, 5:22 PM

[thin dawn]

BlockModel already has the patch

[oblique granite]

pats Automation tools

slow, but still reliable

[thin dawn]

Right now any model is basically on its own responsible for handling that

[white plume]

It shouldn't be a requirement for the model to be a BlockModel

[thin dawn]

No it is not

UnbakedModel has an extension that exposes it

But the parsing tis not done ad-hoc anymore

If your model loader wants to support it

It needs to parse it itself

[white plume]

That's fine

I suppose I'll wait until the code is public because I don't quite understand

[royal solar]

the inheritance that UnbakedGeometry allowed was very nice, and not really feasible with UnbakedModel as far as I know

[thin dawn]

UnbakedModel is now simply a bake + resolve

Which was literally the same API as UnbakedGeometry would have been

[white plume]

To me it seems UnbakedModel now provides the same level of inheritance

[thin dawn]

It does, they moved some stuff around

[royal solar]

UnbakedGeometry allowed inheritance of the model loader and a bunch of fields

[thin dawn]

Which makes it very useable as the core of the API

[thin dawn]

You can do that now too

[wise crag]

Does it count still?

It's Friday

[deft grail]

Second snapshot today

[wise crag]

Lol

[royal solar]

ah yes the parent is now available for all baked models

ok yes that looks good then

[thin dawn]

Yeah

This works so much simpler these days

[radiant field]

why are there so many other changes in that RC

they removed the neighbor presence check in section rendering

[fading igloo]

oh, you know
it just happened
I guess the mood was right

[vagrant saffron]

literally minmaxing bugfixes

[thin dawn]

13 patches to go

11 in client

[lethal panther]

That's me deploying to production cheekily

[sand vine]

when the flow is right you don't just stop

[lethal panther]

Colleague: "hey the monitoring just turned red, what's up with that?"
Me: "just look somewhere else for the next five minutes"

[thin dawn]

3 Patches

1 Client

2 Patches remaining

Which are basically only the data gen stuff

@royal solar I am going to leave those to you 😄

[fossil raven]

https://www.youtube.com/watch?v=34hnq35hRgI

[thin dawn]

@royal solar What is now left is adding the missing loaders and models for dynamic buckets, and seperate transforms.

Extending vanillas CompositeItemModel to have the functionality of ItemLayerModel and adding events for the bootstrapping that happens in ClientBootstrap

I will do the first for sure today

Maybe even the last too

But that should then be all model loader shenanigans done

Code is a lot cleaner

[quick cairn]

Primer just has a few additional convenience methods

[thin dawn]

@quick cairn The primer will need to point out the removal of the UnbakedGeometry system

And the IGeometryLoader

[swift linden]

thats modded stuff tho right?
thought the primer went over vanilla only
and neo made blog posts for our changes

[quick cairn]

Do you happen to have the relevant PR(s) or commit changes? It'll make it easier to do an associated separate writeup

[swift linden]

at least thats how i recall happening in past

[quick cairn]

Yes, though I can create a separate document for the vanilla changes

[swift linden]

recent commits on kits should have changes orion is talking about iirc - https://github.com/neoforged/Kits/tree/1.21.4-pre1

[thin dawn]

https://github.com/neoforged/Kits/commit/3f7951b8225dc88c03ea79fe667a8de58167290b <- Primary commit

Basically fixes all the entangled patches that are related to it

An example of how it is then used can be found here: https://github.com/neoforged/Kits/commit/7d5f195a5806c5f837ea7bc2f995ac17213b4e8b

In particular the obj directory shows how it is used in complex models

But the UnbakedCompositeModel shows how it is handled for some simpler approaches

[fleet hatch]

The separate transforms model is superseded by vanilla ItemModel functionality

[thin dawn]

Well that is one less

But not exactly

Actually no it is

Great

[royal solar]

Sounds great thank you! I'll have a look through the commits at some point tomorrow

[thin dawn]

It is not completly done

But it is getting there

Technically AbstractSimpleUnbakedModel is entirely redundant

And can be provided by each modder themselves

But in the name of easier migration

I added it back as an equivalent in functionality of SimpleUnabkedGeometry

[royal solar]

I'll have a look tomorrow

The name has me concerned already lol

[thin dawn]

It modifies how the BakedModel is constructed

The old SimpleUnbakedGeometry created a IModelBuilder instance configured it, and then delegated to the modder the ability to addQuads to it

This was in particularly handy for Hierachical models, like Obj and others

While UnbakedModel is just plain bare, it gives you the input, and expects a BakedModel that is it

So this wrapper reimplements that idea

It made porting Obj a lot easier

[hidden ravine]

https://github.com/neoforged/Kits/commit/3f7951b8225dc88c03ea79fe667a8de58167290b#diff-8ab5dc18bba83b434a4d7db798850c6327cdb277b1699d9ea27ab4dbee1fd29e
doesn't this already fade out the music if you provide a volume of 0 and a null Music?

[thin dawn]

That is indeed now possible, however the old API supports returning null to immediatly stop the music

Which is what this reimplements

And was an original patch

[thin dawn]

@quick cairn Not sure if you already have this, but for the new model loader and baking mechanism the first change is the texture getter, which was previously a Function<...>.apply(...) and now that is accessible via baker.sprites().get(...)

[hidden ravine]

@royal solar I did quite a bit of work on datagen

[thin dawn]

Work on the bucket complete

Bucket model*

[royal solar]

Cool! I'll have a look later

[hidden ravine]

I did have to split the server and client generated resources into separate source directories since otherwise whichever one runs last deletes everything generated by the other one

[royal solar]

Ah yeah that makes sense

Annoying but it makes sense

[ashen cedar]

right, it only gets worse

[thin dawn]

Okey further processing today: ItemLayerModel

[royal solar]

don't use NotNull it's bad practice

[thin dawn]

?!? Sorry, you probably mean, it is bad practice in my opinion.

[royal solar]

no it always is in our code base

what about parameters 2 and 3?

using NotNull at all introduces 3 states: nullable, not nullable, and ???

[thin dawn]

??? is unknown

As per default

[quick thunder]

@SemiNull

[royal solar]

MC and NeoForge already have package-wide defaults

[thin dawn]

Which is the default state any none attributed member in a none attributed package is in

[royal solar]

so don't use explicit NotNull

[quick thunder]

But I am with tech. Pick one and stick with it

Do not mix

[royal solar]

in fact, we have a spotless rule that disallows it for neoforge

but of course it cannot check patches which is why I am telling you 😛

[thin dawn]

I would, but I am not about to go through 100 classes fixing the non null because a package is missing the annotation in vanilla

And in this case, the IDE was complaining in several locations while I was working on it

[royal solar]

this package already has it

[thin dawn]

So it is much easier and much more productive to tag the ones with @NotNull that I personally added, and that I 100% know are not null or need to be not null at runtime

[lethal panther]

Same, I thought our default was that not specifying means not-null. If it's nullable, mark it with @Nullable

Hence the spotless rule in NF

[thin dawn]

I know it has it, but it still complained to me

[royal solar]

don't introduce bad practices because of bugs in your IDE 😛

[thin dawn]

Not sure why

[lethal panther]

I mean if IJ doesnt pick it up we should check why

[thin dawn]

Hmm it works now

[lethal panther]

and fix that

[thin dawn]

It did not pick it up before

[lethal panther]

Weird

[thin dawn]

For sure

Allthough I refactored this a couple of times

because we had like 3 different attempts at this yesterday

[quick thunder]

Switching between branches often?

[thin dawn]

No, I have only one branch checked out in this repo

I make a special repo per port

So I don't fuck things up

And have a second IDEA open with the normal NF repo

So I can reference and look things up

[royal solar]

I mean, the logical thing to do would have been to check why it wasn't working, certainly not adding NotNull inconsistently

[thin dawn]

Really?

Can you please get of your high horse

[royal solar]

^ it is always bad practice

I am pointing things out that need fixing

It's good that you said you'll fix them

And just also pointing out that not adding the annotations would have saved time because you wouldn't have to fix it now 😛

[quick thunder]

Ok tech, you beaten the dead horse

[thin dawn]

Except as already said, that did not work, and I am not going to stop working on a specific topic (aka porting, and actually making the whole model loading and modification system work) to figure out why the IDE things it is not null, when as soon as I add the annotation it is happy. That is not time well spend.

It is easier to go through this later on when spotless complains, because it gives you an entire list of issues to fix all at once, I probably added an import here or there too, by accident. It happens.

You should really not be looking at stuff like this because as you said, it is a waste of time that is trivially fixed by having Spotless list out the violation, and fixing it.

Okey enough on that dead horse

@royal solar What do you think of ItemLayerModel.......

I am not sure how to reintroduce it

Obviously we can patch vanilla to have that logic....

But should we?

I am asking because the vanilla composite item model (which this was an intended replacement for) does not have the relevant restrictions anymore (aka it can have as many models internally as the pack defines), plus it supports the relevant normal models as well, making it in my opinion redundant

[royal solar]

I haven't had a look yet

[royal solar]

(Note that spotless doesn't run over patches for this which is why it has to be checked by hand - even if it could, it wouldn't be able to auto fix it)

[thin dawn]

We need to run spotless at the end anyway...

Yes it does not run over the files right now, but as per porting processes, we generate the patches and then perform spotless

Which will find all problems all at once

[royal solar]

It won't find NotNull usage in patches

[royal solar]

Is it data oriented?

[thin dawn]

Yeah

It is completely driven by a codec on the model

And it supports having our models in it self

So you can do any kind of combination on all layers

Including layers which are specially rendered

[royal solar]

Yeah then we can yeet ours I think

[thin dawn]

Already done 😄

And with the new event just added, modders can now also register custom ItemModels

Which are then also natively supported in it 😄

[thin dawn]

continues porting in voice chat

@royal solar Let me know if you have any issues with the current design of things, and I will address them asap

[royal solar]

I haven't really had the time to review your stuff yet; might still be a few hours

lol sniped

[thin dawn]

That is fine

[royal solar]

I'm not too familiar with this item stuff

I need to check how well it composes

[thin dawn]

Several people have looked at it by now

It actually works pretty good, I am adding events to the ClientBootstrap stuff now

So it should work pretty well down the road as well

[royal solar]

shouldn't we just remove IBakedModelExtension#getRenderPasses?

since it is item-exclusive, and probably best done by making a custom ItemModel?

did you add hooks for custom ItemModels? that's probably very useful

[thin dawn]

That actually has already come in handy with the fluid model

As there is no concept of programmatic dynamic overrides anymore that method has become a very handy tool for when modders want to return a different item model based on the itemstack that is about to be rendered

Which is very usefull for the bucket model for example

As that needs to rebuild an entire model per fluid

Sometimes even flipping it upside down for gasses etc

Which is difficult to otherwise implement

Using things like conditional item model properties

[lethal panther]

I always hated how indirect that is though

[thin dawn]

It is very usefull though during rendering

And with the way it is used now, it is a direct API endpoint

[lethal panther]

I am not critiquing the use case

[thin dawn]

Which should take all of that indirectness away immediatly

[lethal panther]

Just that if you are faced with the problem you are describing

getRenderPasses is not really what you'd go looking for first

[thin dawn]

I know

I did not come up with that name

[rich rune]

couldn't we just rename it?

[thin dawn]

The idea of getRenderPasses was that you could return the individual rendering models with their own render types

Which was usefull

Which is where the name comes from

[thin dawn]

Meh, in reality we could these days, and I am up for that, but then somebody needs to come up with something better 😄

Keep in mind that this name has been in use now for a while

And allthough it felt wrong initially it is what the community now knows

I now added events for all remaining client bootstrap resources

Does anybody know what the best way was to create a MapCodec of a singleton? (aka returning a fixed instance)

[deft grail]

MapCodec.unit?

[thin dawn]

Thanks 😄

All core elements are loading,

Working on test sourceset now

Okey got the tests to the main menu

With a whole bunch of resource errors 😄

now needs to rewrite the entire custommodelloader mechanic...... to have support for vanilla models

Great

[royal solar]

The fluid bucket model should probably be an ItemModel now, rather than a BakedModel

[thin dawn]

I personally see no reason to do that to be honest.

At least not right now

[royal solar]

It should allow us to remove legacy methods, that's a big win 😉

[thin dawn]

What legacy methods?

There are currently no legacy methods......

So there is no win to be had here

[royal solar]

getRenderPasses looks superseded by item models

[thin dawn]

It is not though

That is the whole point

[royal solar]

Explain why we can't use an item model instead?

[thin dawn]

We can, I just see no reason to alter that

[royal solar]

Putting item-specific code in a BakedModel just to go through BakedModelWrapper is extra indirection, and against how I understand the system to work

Admittedly I might have overlooked something that would make it impossible

[thin dawn]

As I said it is not impossible, and it is an extra indirection, but I see no effective reason to alter the behaviour currently

Mojang is heavily working in this area, and this a refactor that can be done later as well

[royal solar]

Do you have any objection against me doing it later?

[thin dawn]

Making it an itemmodel no, removing getRenderPasses yes

[royal solar]

(later = today or tomorrow)

Why not make it an itemmodel?

[thin dawn]

Read that again

You asked me if I had objections

[royal solar]

Ahh

[thin dawn]

I don't have any objections to you making it an item model

But I have objections against removing getRenderPasses

[royal solar]

Ok good

My follow-up question is then what is still requiring us to keep getRenderPasses?

[thin dawn]

BakedModels should be able to alter their behaviour when rendered as Items

If we remove getRenderPasses that means that behaviour now has to be implemented twice

For example FramedBlocks and C&B would need to reimplement the entire code paths to do it

[royal solar]

I think it should be a simple shuffling around of a bit of logic

[thin dawn]

And the way getRenderPasses is implemented now, mimics what CompositeItemModel does while not requiring that rewrite

[thin dawn]

It is actually not

[royal solar]

@fleet hatch can probably comment on this too

[thin dawn]

At least not for C&B

And this concept of getRenderPasses was also explicitly added so that a single BakedModel could render different sub parts (aka different inner BakedModels) with different render types

Again without having to go the route of implementing it twice

The single method is as such not legacy

Easy to maintain and implement

[royal solar]

But you still have to implement new BakedModels to return?

[thin dawn]

And should not be removed

[thin dawn]

Which we already needed today anyway

[royal solar]

So you could wrap these new BakedModels in the item model wrappers and move all of the code to the item model

[thin dawn]

It is not anything different, as you can see from the fluid bucket model

Which also just reuses the original models implementation with a different fluid

[thin dawn]

I am not arguing to blcok the path tech

I am saying that we should keep the methods because they are a good API to keep

It helps people during porting

And it opens up a different approach to how it is rendered

[royal solar]

Let's simplify as much as we can

[thin dawn]

No?

That is not what forge is

[royal solar]

Removing it doesn't take any functionality away afaict

[thin dawn]

He?

Have you not been listening

[royal solar]

I guess I'll have another look later

[radiant field]

I think the better question to ask is how much tech debt does it introduce to keep the API

even if it's redundant

[thin dawn]

It is a single patch to keep it

[radiant field]

Then keep it

[thin dawn]

Which is perfectly fine

[radiant field]

And yeet it later when it becomes uglier than a single patch

[thin dawn]

It is even a lot less then before this

[royal solar]

Every single extra patch should be yeeted if it's not useful

As I said, we'll see

[thin dawn]

I would be open to marking it deprecated and telling people to move item models

[radiant field]

I was about to suggest that

[thin dawn]

But I am not open to just yeeting it if you think it is not "usefull" tech

[radiant field]

Also 'not useful' requires a reasonably complete set of mods to actually determine

[thin dawn]

Exactly

That API opens up a parallel way of handling item rendering

That potentially with some work from each modder individually could achieve the same thing

Or you know

We could save that work for now, untill the community has a much better understanding

And deprecate it in this version (maybe), and then show how to do it in good guides

It saves some porting work for most modders right now

And helps with adoption and understanding

[royal solar]

We already yeeted a lot of things that have become superseded (IGeometry stuff, ItemLayerModel, ...). I don't see why we wouldn't remove other stuff that has become superseded. These arguments like minimizing modder effort were not a consideration before, so you're being awfully inconsistent. Is it just because I am the one pointing it out?

[thin dawn]

No, we thought this very much through.

The question we asked ourselves in particular to the IGeometry stuff is: How difficult is it to reimplement, vs how much work is it for modders to adapt to it. And we tested it out with the bucket, the obj loader etc. And it turns out that reimplementing the patches was way more work then an individual modder would need to do to adapt. So it was altered and modified.

The ItemLayerModel stuff was a question mark for us. We were unsure as it felt like a 50/50 reimplementing it would be relatively easy, but vanilla also had the exact same functionality in its new model system. So it would be at most a re-datagen and done. In the end it got yeeted.

With this method the story is a lot different, simple tests already had shown to me that reimplementing this getRenderPasses system in ItemModels is not as straight forward as it originally seems, as you now also need to pass data into the ItemStackRenderState which is not extensible at the moment, so custom ItemModels will have a lot harder time processing and storing data. If you compare that to retaining a method that already exists (potentially deprecated) that could be implemented with a single patch, the choice is easy to make, and easy to defend, even though it duplicates some functionality.

[deft kindle]

just deprecate it, do some more testing during the BC window, make the item render state stuff extensible and see if it's still needed, then decide if u wanna remove it

this seems very simple to me

[thin dawn]

Yeah it really is

[white plume]

I think getRenderPasses should be removed as well

[deft kindle]

once item stack render states r extensible, i agree

but it doesnt have to be done right this instant

[white plume]

Extensible in what way?

The problem with giving the BakedModel the ItemStack is that the ItemStackRenderState is not supposed to store any state that may be mutated externally (the ItemStack)

In vanilla, you are able to update an ItemStackRenderState once and then render it as many times as you want at whatever point in time you want (before a resource reload), and it will always render exactly the same thing. With the above, that assumption may be violated.

[thin dawn]

What gives you the idea that renderstate is reuseable?

ItemRenderer does not even have that approach right now. As it uses a single scratch state to do the rendering. Yes they "might" eventually go that route, and then yeah we "might" need to address it.

But again, right now none of that is reality and patching in the method helps with adoption.

[lethal panther]

It's not immutable at least, but the state that usually matters is immutable on the itemstack now (Item + Components)

while the stack itself is not

[white plume]

If the stack comes from an inventory it could easily be mutated

[white plume]

It just is reusable in vanilla, based on what data its fields store

[lethal panther]

The item and components themselves are immutable, we just don't have an immutable container for those two pieces of info atm

[white plume]

Ah yes

[royal solar]

Components are immutable but the component map is mutable

So a stack is very much mutable

[royal solar]

why do all the registration events create a map just to putAll in the id mapper?

ok changed it to work through the ID_MAPPER so we get proper duplicate detection for free

[thin dawn]

That was the original design on the older events

[royal solar]

ah yes, looks like that was typically done to ensure that the data can be copied to an unmodifiable map after the event

why does IModelBuilder even exist?

[fleet hatch]

My plan for FramedBlocks was to go all in on custom ItemModels, in part because I need custom forced tint sources and in part because the item layer render state dictates the render type and there can only be one per layer, which means that I won't get around using two layers for my double blocks

[royal solar]

are you saying that it wouldn't work at all with getRenderPasses?

[thin dawn]

With getRenderPasses that is fixed

You can have multiple rendertypes

And even combine the relevant models together

[fleet hatch]

In my opinion, both getRenderTypes() (the itemstack one) and getRenderPasses() should be removed. They are entirely and trivially replaceable by ItemModels

[thin dawn]

It is actually not that trivial

I tried several different things yesterday

It is doable

But not easy

[fleet hatch]

I can't think of a case where it's not simple to do. The dynamic fluid container in particular feels like a perfect use case for a custom ItemModel to me since it can then set up the render state exactly the way it needs to with respect to specific model layers based on the fluid and the render types

[royal solar]

ok I did a had a look through all of Orion's commits, looks generally good

my notes for things to check more thoroughly:

• the BlockModelWrapper patch (used for getRenderPasses and possibly other things) - I have to check how well item models actually compose
• AbstractSimpleUnbakedModel and the IModelBuilder system are weird, they are wrapping a vanilla builder with 2-3 layers of indirection; but first I need to check where exactly IModelBuilder#collecting is used to see if it makes sense
• ConstantItemTintSourceBuilder in the testframework is EXTREMELY suspicious - that stuff probably needs to be datagenned 😄

[thin dawn]

1. is already tested and seems to work as intended
2. ASUM is a bridge, especially combined with IMB, Mark them as deprecated as vanilla as SimpleBakedModel, that does most of the same things with a different API, so a transition API is usefull but can be yeeted in .5/.6
3. That is something I am not sure how to solve, IMHO it should be datagenned, but the idea I had was that a user only needs to datagen that the tintsource has the same name as the block/item, so that it works.

[white plume]

I agree

The behavior of those methods is exactly what LayerRenderState does

[royal solar]

are you sure that you can compose multiple composite-like item models?

[thin dawn]

Yeah

They just add more layers to the state

[royal solar]

ah yes ensureCapacity is just for preallocation

[thin dawn]

Yep

It works very nicely

Composite models just stack layers on the render state

And that is it

They are a very nice tool

[royal solar]

ok yes I see

[thin dawn]

What getRenderPasses does is make BakedModels in general composite capable

Which is a usefull property to have

[royal solar]

but only for items, and without being able to pass the itemstack context down directly

[thin dawn]

They did not pass the context down either, at least not ones we implemented

It basically restores the functionality to what it was today

[white plume]

BakedModels can be composited in general because they just return some geometry and that geometry can be combined

In fact, why even return a list from getRenderPasses instead of a single BakedModel if a List<BakedModel> can be converted to a BakedModel due to the above?

[thin dawn]

Because each bakedmodel is assigned one rendertype

So returning a list means that the composition elements in and of it self can have their own rendertypes

[white plume]

Oh right, but still seems like that should be the ItemModel's job to decide. If the functionality is duplicated, might as well just pass the ItemStackRenderState directly to the BakedModel and let it populate it with its passes.

[royal solar]

it is weird that the composition mechanism is different for block and item contexts

[thin dawn]

What do you mean?, getRenderPasses literally makes it the same system.......

But what ever

[thin dawn]

That last one is also an option

But i chose to retain backwards compatibiltiy in the API

So I reimplemented the methods calling patch

[white plume]

You could add such a method and have it delegate to the existing methods and mark the existing methods as deprecated

[thin dawn]

Also an option

But I focused on getting it actually to work

I was not really designing a new API for a system that already existed and worked

[royal solar]

@thin dawn as discussed I will make client true for CLIENT_DATA and server true for SERVER_DATA

i.e. like this

[thin dawn]

Yeah that should be fine

Given that there is no split sourceset that is not relevant to me

Any data run will just download assets

But the CP is the same between all runs anyway

We need those flags later though

[royal solar]

wdym later?

[thin dawn]

Well when split sourcesets are involved

I need to split the correct CP of

And generate the correct runs for the correct CPs based on what is configured

[royal solar]

ah yes

[thin dawn]

So I need server runs for CPs with the common jar on it

[royal solar]

I don't even know what the metadata will look like by then

we'll probably want to add more

[thin dawn]

And I need client runs where we have the client jar on the CP

Maybe

But this for now suffices

That reminds me @royal solar Can you add a comment to the patch that moves ClientBootstrap.init() to after mod init, we should move that back once we split client moddiscovery / loading and client init from each other

[royal solar]

ah yes, I saw that but wasn't too sure what to think of it

ClientBootstrap.init() got moved really late

but where it normally is, is way too early

an alternative is to leave ClientBootstrap where it is, and fire events later

[thin dawn]

I moved it, because right now all that it does is needed in resource/data load

Which still happens after that

While also being the first moment that mods exist properly on the client

As I said, it is a simple solution for a different problem

[lethal panther]

Is this the point at which we move client modloader begin to before the ctor? 😛

[royal solar]

we can't move client mod loading this early

(this is vanilla)

[lethal panther]

Is it still just calling static ctors that we might mess up if mods access the classes?

[royal solar]

IMO moving clientbootstrap like you did is fine for now

[royal solar]

yes

[lethal panther]

hm actually it's not in static ctors

[royal solar]

the common one is

[lethal panther]

yes but the client one isn't

[royal solar]

indeed

[lethal panther]

but it also doesn't look like it's "baking" these maps it's filling in the boostrap methods

[royal solar]

this is missing the rendertype awareness

so unless we remove getRenderTypes(ItemStack) we need to patch that back in

[radiant field]

the rendertype-aware getQuads overload has never been used on items afaik

[royal solar]

indeed wow

that's quite bad 😄

[thin dawn]

It indeed has noz

Because it is controlled via the buffer

[royal solar]

yeah but you have no way to e.g. only make some quads transparent

[thin dawn]

And getRenderPasses

[royal solar]

I suppose that nobody noticed because it was possible to work around it with getRenderPasses

it really doesn't make sense to send all quads of a submodel to all render types 😅

[radiant field]

ok random thought with very little research done to back it up: what would happen if we provided ItemStackRenderState through ModelData 🤔

[royal solar]

what are you trying to achieve here?

[radiant field]

have one code path for 'dynamic' models instead of one for blocks and one for items

[thin dawn]

I think we can research that later on again

because right now we have the status quo that this works

And with more modders looking at ItemModels, we can get bette rfeedback about what is needed vs what is not

[royal solar]

I wouldn't put this stuff in unbaked

because the class also contains the baked model in some cases

[thin dawn]

It does

Feel free to move them

[royal solar]

ok I'll just move one package level up

[thin dawn]

Good by me

[royal solar]

do we have a test for UnbakedFluidContainerModel atm?

[fleet hatch]

For shits and giggles. I expected a lot more than that

[thin dawn]

Yeah that should be doable

[fleet hatch]

Well then, that's better than I expected 😅
(to be fair, it's kinda "saved" by the fact ModelEvent.ModifyBakingResult is currently not hooked up, otherwise it would immediately crash due to things I commented out)

[thin dawn]

Did you refactor to ItemModels

Or just used the orgiginal paths?

[fleet hatch]

That screenshot is the bare minimum required to make it build and run, none of my item models work yet.

[fleet hatch]

After playing with this for a couple hours, fixing a few things (render types being ignored in certain cases, ModelEvent.ModifyBakingResult not firing, "standalone" models not being baked) and running into a tricky trap with custom UnbakedModels which wrap another UnbakedModel (all UnbakedModel methods except bake() need to delegate to the wrapped model, otherwise all inherited fields [textures, ao, shade, etc.] are not used when the custom model is baked), I've gotten to a state where my block models fully work again. Either tomorrow night or on Monday I'm gonna dive into the item model stuff

[radiant field]

Boolean object? 🤔 https://github.com/neoforged/Kits/commit/169d2ff1e1d0abc7c713ec9cefa79f00f00bd532#diff-a448018b5e46b16ebf14a8673fe6ca771e98b8116a932bb9d49685b2f7520e5cR24

[fleet hatch]

Yes, vanilla uses a boxed boolean to distinguish between specified as true, specified as false and not specified in the file this model comes from

[rich ferry]

OptionalBoolean left in the dust once again

[quick cairn]

@thin dawn I've taken a quick look through 1.21.4-pre1 on kits to check out the changes for the geometry loaders, and is it not fully implemented yet? It seems like the json deserializer instances have not been registered. Additionally, is there a reason why we're using the deserializer over a codec?

[royal solar]

unbaked models still use json afaik

[thin dawn]

The loaders are registered, and the entire system still uses GSON, so........

[royal solar]

this stuff should be moved back to UnbakedGeometryHelper I think

at the very least out of the fluid container model

[fleet hatch]

I don't disagree but hold off on that for now, I would like to experiment with getting rid of these entirely by repurposing the ItemModelGenerator (which should be fairly simple since that itself is the model now)

[royal solar]

I am changing UnbakedFluidContainerModel quite a bit to make it an ItemModel

[fleet hatch]

Heh, I looked into that as well last night, didn't finish it though

Very curious what your approach will look like

[royal solar]

I'm almost done with it

and I just noticed that there is no real TextureSlots equivalent for item models

but inheritance of these textures is somewhat pointless anyway 😛

[royal solar]

here it is: https://github.com/neoforged/Kits/commit/f27c499aa7a4205fad5ebea61f30c1864090bc12

had to move the static methods to UnbakedElementsHelper sorry 😅

[fleet hatch]

Looks good from a quick look on mobile, I'll take a closer look when I'm back home tonight

[royal solar]

why is this crap back

I don't think it's quite correct @fleet hatch 😂

[ashen cedar]

it is perfect

[royal solar]

TIL there's a neoforge:item/default model???

[plush inlet]

some of Lex's work, iirc when I tried to trudge through that

[ashen cedar]

hwat does it do

[plush inlet]

iirc, it's nearly the same if not identical to one of vanilla's models

...goes to my IDE

[oblique granite]

I don't know what it is now

but when I wrote it, it was basically the default transforms, but without a "parent"

so that it could be used in custom loader models

[plush inlet]

ah yes, it's nearly identical to minecraft:item/generated, but without the parent to builtin/generated

and neoforge:item/default-tool, which is minecraft:item/handheld but its parent is neoforge:item/default

these resources aren't datagenned, so there's no comments on why they exist

except for what giga just said now, so 🎉...?

[quick cairn]

Hmm, I’ll take another look, but I thought I just saw everything commented out for the registry

[royal solar]

mild progress? 😄

I am sourcing the ItemTransforms from the item/generated model

[royal solar]

ok not too bad

and with lava

there's still a weird seam on the side - looks like an issue with our mask textures more than anything 🤷

[ashen cedar]

looks like a feature

[royal solar]

yup

[fresh lava]

Wait, are those automatically generated buckets?

[royal solar]

yes

that's what the dynamic fluid container model does

[radiant field]

I cannot unsee the legacy water color on the vanilla bucket now 😠

[royal solar]

I don't think it's legacy

it just looks better in vanilla 😛

@fleet hatch what are your thoughts on the extra UnbakedModel#bake overload that we add?

it feels to me that this RenderTypeGroup is not really relevant to most models

(like all of the other extra parameters that vanilla now passes anyway )

it feels like it could just be resolved using getTopRenderTypes wherever it is necessary

[royal solar]

ok no that won't work

there is no inheritance unfortunately

[fresh lava]

That is cool.

[white plume]

The overload is not public but what I think would be best would be to add some sort of unbaked model data system (I want to add this to Fabric API) where data types are registered and define the composition behavior and default value, UnbakedModel gets a new method that returns a map of data type to data, and bake accepts such a map with the values already composed as necessary

It's the same as what vanilla already does for its 4 existing inherited parameters but done in a flexible way

[royal solar]

yeah that makes sense

[remote fractal]

soo we ready to move to the latest rc?

[royal solar]

we need to gen patches and fix spotless first

we should fix tests at some point but 🤷 😄

[hidden ravine]

Is the junit launch target in FML meant to be forgejunitdev?

[royal solar]

no, @lethal panther probably forgot to rename it

and I didn't notice 😄

send a PR coeh 🙂

[fleet hatch]

I added that last night because render types were completely broken without it
Pepper's suggestion to make this extensible without adding infinitely more parameters sounds good to me though

[royal solar]

looking into that rn

[fleet hatch]

👌

[royal solar]

any idea for a better name?

same idea as dynamic baked model but for the unbaked model 😄

[fleet hatch]

Can't think of anything better, dynamic doesn't sound right in this case

[radiant field]

Isn't UnbakedModelExtension more conventional

[royal solar]

we have an extension already with extra methods

this is similar to IDynamicBakedModel vs IBakedModelExtension

next up: let's deprecate getRenderPasses and getRenderTypes(ItemStack) with the following message: @deprecated Please migrate to {@link ItemModel}s, or if this is not possible contact us at NeoForge.

that will give modders a chance to object in case it turns out to be harder to migrate than expected

[white plume]

👍

Is the render type group of a BakedModel used by an ItemModel used to replace the provided RenderType if the group is defined?

[royal solar]

baked models don't have a render type group?

whatever getRenderTypes returns is used in the ItemModel, if that is what you are asking

[white plume]

Aha

[hidden ravine]

Pushed the changes I had locally
also the biome modifier sync test datagen still needs fixing since the test is executed on the client side but it uses server side datagen

[royal solar]

we might want to make our own datagen client-only

i.e. add all providers to the client datagen event

that should be possible no? 😄

[fleet hatch]

I would say deprecate getRenderPasses() for removal and change getRenderTypes() to return a single RenderType instead of a list (without the method the render type entry in the model wouldn't work for item rendering). Returning a list never made any sense for items since item model rendering never allowed returning RenderType-specific quads, meaning all quads of a given pass are rendered for all render types.

[royal solar]

yeah that sounds good

for the flexible properties... I need name ideas

UnbakedModelProperties, BlockModelProperties, AdditionalModelProperties?

we might also just wait for later

I just want to make sure that we don't break the signature of bake if/when we add these flexible properties

[fleet hatch]

UnbakedModelProperties sounds good to me. Another option could be ModelBakingProperties

[royal solar]

for now I am considering just adding a context interface, but not allow custom properties yet

[royal solar]

the properties will be sourced from BlockModels most of the time, so ModelBakingProperties isn't quite right I think

[white plume]

I need to think of names and a design for the Fabric equivalent too

[royal solar]

are custom models expected to also provide some of these properties?

or will they be BlockModel-exclusive?

[white plume]

Any UnbakedModel should be able to provide or consume any property

If an UnbakedModel does not have a value for a certain type, it is interpreted as the default value for that type

[royal solar]

there's no need for default values IMO (in other words: the default value is just the absence of the property)

[white plume]

Unifying all properties like this also allows UnbakedModel wrappers to properly forward everything

[white plume]

Yes, default value and absent value are the same, but the type might not want the default/absent value to be null specifically

It might make more sense for a certain type to use something else and have the value be NotNull instead

[royal solar]

as an example for NeoForge: the render_type could be null if unspecified

so when retrieving the property, you'd do properties.getOrDefault(RENDER_TYPE, RenderTypeGroup.EMPTY)

[white plume]

Ah, so the absence of a map entry is the only way to specify that

I think that's fine

[vocal jungle]

Out of curiosity, is this for the additional properties Neo adds, like emissivity/etc?

oops, ping

[royal solar]

no worries about the ping

it's only about "top-level" UnbakedModel properties, not of the elements

[vocal jungle]

Ah, okay

[royal solar]

the only such property we add in the port is render_type

[vocal jungle]

Yeah, UnbakedModelProperties is a good one in that case

[white plume]

There is also part visibility but I'm not sure what exactly is happening to that

[radiant field]

Why is render type a top level property

[royal solar]

visibility could also be added back (it was there in 1.21.3-)

[vocal jungle]

we should really deprecate visibility

[white plume]

Because child models can override it

[vocal jungle]

That is, as a thing shared by all models

We should instead provide a specific "composite model" type which composes one or more unbaked models with their own visibility

[royal solar]

that's more or less how ItemModels are expected to work now, but there is no nice equivalent for block models (yet?)

[white plume]

The block model system will be further refactored very soon according to Boq

[radiant field]

Nvm this was a dumb question

[vocal jungle]

I convinced Orion to add a composite when they were doing kits work, but I don't know if it's still there

[radiant field]

We do not specify render type per-quad like FRAPI

[royal solar]

one possible answer is that it requires rewriting the final stages of the quad emitting pipeline 😛

[vocal jungle]

But yeah, there's no equivalent to composite models for blocks - multipart and variant KIND of work but they can only be specified as a blockstate definition

[white plume]

Neo adds the composite loader though

[radiant field]

The composite model always looked like a weird sibling of multipart

[vocal jungle]

Yes, that's why I convinced Orion to add it when they were porting it in VC 😛

I think it predated multipart/variant stuff

[white plume]

Oh interesting

[vocal jungle]

In fact, I think a lot of the geometry baking context stuff predated json models

[royal solar]

are you sure about that? I think giga added that stuff around 1.14

[vocal jungle]

Shrug

It definitely FEELS like legacy stuff that would predate json models

[royal solar]

the geometry stuff allows for nice property inheritance though

[white plume]

I thought Giga said he rewrote it to use the separate IUnbakedGeometry class in 1.12

[royal solar]

which we currently kinda lost for custom loaders in the port

[vocal jungle]

I believe in the prototype of codec models I had property inheritance working

Though it wasn't exactly pretty

[royal solar]

what saves us is that a block model will delegate to its parent if it has no elements

so in principle custom loader inheritance will continue to work as long as the custom loader doesn't use the elements key 😄

[vocal jungle]

If I remember the implementation right all of the properties inherit if they aren't overriden

[royal solar]

only the ones that vanilla defines + render_type

[white plume]

Yes

[royal solar]

making more properties inheritable is exactly the point of this discussion

[vocal jungle]

Ah, I must've misunderstood then

[royal solar]

as for the data structure I should look into using a ContextMap

[vocal jungle]

But yeah, if it's worth anything here's the code I wrote when I made my prototype: https://github.com/neoforged/NeoForge/pull/1557/files

[white plume]

What's that from?

[vocal jungle]

I introduced an additional "TopLevelUnbakedModel" type for convenience, which was basically used for a nameable unbaked model

[royal solar]

vanilla 😄

rewriting all of vanilla's serialization code to use codecs definitely feels off

[vocal jungle]

There were some issues with it, yes

but the PR also contained effectively what they've done now by putting more emphasis on UnbakedModel

At the very least, you can remove one method (TopLevelUnbakedModel#codec) and two patches and it won't use codecs

[royal solar]

yeah the design of TopLevelUnbakedModel is quite close to how UnbakedModel now works

[white plume]

Interesting, I'll need to take a look at that

[royal solar]

note that Forge patches this in to always allow additional keys

the registration I have in mind would be something like event.register(contextKey, deserializer)

maybe I would combine the deserializer and some merge function in an interface 🤔

[fleet hatch]

The way property inheritance in BlockModel is handled changed: previously bake() would be called on the top-level model and it would then collect all the data and handle the baking. Now bake() is called on the top-level model, which then checks whether it has elements and bakes them if it does or calls bake() on its parent if not

[vocal jungle]

I see

So, in that case, I imagine we should add an UnbakedGeometryHelper class which would contain one or more methods to get the effective elements/rendertype/etc by searching the parents, and then change bake() to instead bake if any of those are retrieved from a different model

We could do something like this:

Pair<UnbakedModel, List<BlockElement>> getEffectiveElements(UnbakedModel model) {
    while (UnbakedModel m = model; m != null; m = m.parent) {
        if (m.getElements() != null) {
            return Pair.of(m, m.getElements());
        }
    }
    return null; // should never happen
}

[fleet hatch]

The simplest way to deal with this is to pass the additional data down the recursive call chain like vanilla does with AO, shade and other inheritable data. UnbakedModel#bake() is currently patched to do this for render types and Tech is working on generifying this right now

[vocal jungle]

Oh?

Oh, I see! They changed bake()'s signature

[fleet hatch]

Yes

[vocal jungle]

In that case, we should definitely do something like this

[fleet hatch]

The whole recursive chain starts in UnbakedModel.bakeWithTopLevelValues()

[vocal jungle]

I see

and bakeWithTopLevelValues basically calls bake(this.x, this.y, this.z)?

which recurses all the way to the topmost thing able to provide a baked model

If so, that's a lot cleaner than the approach I took

[fleet hatch]

Another property that should use this system is the root transform

[white plume]

Is the root transform something other than the thing in the bake settings?

[fleet hatch]

It traverses the parent chain for all inheritable values (except elements), merges them as necessary and then calls bake() on the given UnbakedModel

[vocal jungle]

Oh

That's unfortunate

[white plume]

I was thinking it would be up to the loader to decide how to deserialize the json and manually populate the property because maybe one loader wants to parse it differently from another but the meaning of the value is the same

[fleet hatch]

Yes, the root transform is an additional arbitrary transformation

[white plume]

That's something Neo adds in, right?

[fleet hatch]

Yup

[white plume]

If it can be overridden or composed then it should utilize the property system, yes

[vocal jungle]

In any case, yeah, we definitely should make our bake extension use a ContextMap or whatever equivalent

this would let strange people merge elements if they really really wanted to 😛

[fleet hatch]

Yes, the root transform behaves exactly like AO, shade and friends.
It's useful when you for example need a model rotated along an axis which the blockstate format doesn't allow and want to avoid having to make duplicate models which only differ in rotation

[vocal jungle]

Models like that exist? O.o

I thought the blockstate format gave enough degrees of freedom that any rotation was possible

[fleet hatch]

The blockstate format only allows rotation around X and Y but if you for example have something orientable like a repeater and want to make it placeable on all six faces, then you also need rotation around Z

[vocal jungle]

Ah, okay

[white plume]

Does the root transform use the full Transformation codec?

[fleet hatch]

It uses a custom decoder because it supports multiple declarations: https://docs.neoforged.net/docs/resources/client/models/#root-transforms

[white plume]

Oh, a matrix

Well, that's about as flexible as it gets

Are these root transforms composed or are they overridden?

The documentation does not make note of that

[royal solar]

Right now we forgot to make the transform inherited apparently

They should probably be overridden just like the vanilla item transforms

[fossil raven]

I have a question:
When will Kits be updated for Minecraft 1.21.4rc3?

[royal solar]

does it matter?

[fossil raven]

the update will normally be released tomorrow, I'm just thinking that maybe it's better to work on the release than on a pre-release.

[fleet hatch]

They are overridden

[royal solar]

Here we go: https://github.com/neoforged/Kits/commit/a205a27b5fef18b945e7dea5afdb23198d621304

to summarize the design for people who don't have access to Kits:

• there is an additional ContextMap parameter in UnbakedModel#bake that contains extra properties
• there is an additional void fillAdditionalProperties(ContextMap.Builder propertiesBuilder) method in IUnbakedModelExtension for models to register properties
• models are responsible for deserializing properties, storing them, and exposing them in fillAdditionalProperties. fillAdditionalProperties can be used for more complicated property merging since the builder has already been filled by parent models in the chain

retrieving a property looks like this: additionalProperties.getOrDefault(NeoForgeModelProperties.RENDER_TYPE, RenderTypeGroup.EMPTY);

a little verbose, but that allows the system to be extremely simple

[fleet hatch]

Looks good, just two things:

• fillAdditionalProperties(ContextMap.Builder) should probably be marked OverrideOnly
• Shouldn't the default implementation of fillAdditionalProperties(ContextMap.Builder) be no-op since the static helper already handles the recursion?

[royal solar]

oops for 2) that's a leftover from a previous attempt where the model itself would be responsible for delegation to the parent 😄

[royal solar]

as discussed I am also doing this now

[thin dawn]

Why was this done, if the model already needs to store the information after deserializing, why is the context then added?

Is this so that sub models can read information from their parent?

[royal solar]

yes exactly

or rather, read information from their child

[thin dawn]

!?

So the parent would invoke fillAdditionalProperties?

And then read it. Okey, but why then pass it into bake(...)

[royal solar]

so imagine we have this hierarchy:

• model 1: sets some property and plans to use it
• model 2: has model 1 as its parent, overrides the property

[thin dawn]

That seems like a majorly bad idea

Because it completely and utterly breaks any kind of model isolation.....

Their whole idea always has been that at most there is a parent -> child relationship and the parent can or can not decide what to emit......

[royal solar]

this is how properties like textures, AO, render types work

[thin dawn]

the other way around where model 2 can determine the properties of 1 is risky, and totally nmot needed

[royal solar]

not every property has to be exposed in this way, it's just for properties that support being overridden by child models

[thin dawn]

Textures for sure not, as the slots are determined on root level, and should be handed down from the parent to the child, so there is at most a 1 -> 2 relationship never a 2 -> 1.

The same can be said for render types, as they are individual to each bakedmodel in the end, so the child can have different rendertypes without ever having to depend or know the parents.

The only one I currently think of is AO, but given that that is a static property determined by methods, its inheritance logic is something the parent model can determine freely without ever needing to touch any of this

[royal solar]

this inheritance is how every parameter to bake works

[thin dawn]

No......

If a composite model is baked, it will bake its children

[royal solar]

the composite model goes through ModelBaker#bake which will in turn call bakeWithTopModelValues

[thin dawn]

What are you looking at....

[royal solar]

so there is no interference between the children for a composite model and their "parent"

[thin dawn]

Because I don't have that method

[royal solar]

UnbakedModel.java

[thin dawn]

Yes, as I said their is a 1 -> 2 relationship in those methods with 1 being the parent as it can and does override the properties of 2.

So how would that work with your additional properties? Because the values generated by 1 will override the values generated 2

Causing the values of 1 (the parent) to be used passing them downstream

Aahhh

But that is exactly what you want.....

Hmm

And because it is a lot easier now to use a model reference instead of a direct model object for inner models it is a lot harder to define that information flow in the same file

Okey I see where you are going with this

I still find it majorly risky to break that barier for any and all information

But I guess that is likely the only option to do some of the data passing

[royal solar]

this is not about inner models but rather about model inheritance with "parent": xxx

to the user it should look like properties are inherited and can be overridden

[thin dawn]

Sure, but why the rendertype?

[royal solar]

how about

{
  "parent": "block/cube_all",
  "textures": {"side": "block/glass"}
  "render_type": "cutout"
}

[thin dawn]

Actually properties is not at all the same as textureslots, guilight and AO

You inject the parent first

You need to inject the own model first

And then inject the parent

https://github.com/neoforged/Kits/blob/fdc1910b597c8bfe2e17bd8cc70c0bd67f46a3ad/projects/neoforge/src/main/java/net/minecraft/client/resources/model/UnbakedModel.java#L58-L61

[royal solar]

I inject the parent first so that the children can manipulate and override the data in the parent

[thin dawn]

Yeah but that is exactly what I don't think is a great idea

I mean at most people shoot themselves in the food because the behaviour is exactly the opposite of the rest.....

[royal solar]

it looks different in how it's implemented but it does the same thing: just add your properties to the builder, and it will override anything that the parent might have specified

[thin dawn]

Yeah I am now reading TextureSlots.Resolver....

Wtf

Ignore what I said

This makes sense

Just the way mojang did this is completely counter intuitive

Why would you walk the parent last, add its values always last

And then reverse the entire fucking list

Just add the parents first..........

Okey this makes sense

Fine

Seems reasonable

This seems scary because the concept and implementation mojang chose

But it is actually pretty tame

[white plume]

Can the type define the composition behavior or will it always be overridden? Some properties like the vanilla textures property is combined instead of overridden.

Actually for that one the type of the raw value is different from that of the combined value, too

[royal solar]

there is no type, just a ContextKey

you get access to ContextMap.Builder which has getter methods, so you can compose properties with a bit of manual code
I suppose we could add a builder.merge(key, value, mergeFunction) function if we wanted to make this a bit easier

[royal solar]

that was the main goal yes. I considered adding a new UnbakedModelProperty<T> type with a merge function and a serializer, a new UnbakedModelProperties map to hold them, etc... but it's a lot of extra code

[royal solar]

that's true, but taking that into account would make the system too complicated IMO

[fleet hatch]

On a completely different note: is there a way to make this not annoying: https://github.com/neoforged/Kits/blob/1.21.4-pre1/build.gradle#L15? As it stands, there is currently no way to change the version without fucking around with the build.gradle in some way (i.e. appending some garbage to the version string) because just trying to reload the Gradle project doesn't take the change in time into account, which means you always get the same version string.

[royal solar]

Config cache ftw 😄

[royal solar]

XFact could we maybe convert most of UnbakedModelParser to a patch?

[fleet hatch]

Which class would you patch it into?

[royal solar]

Vanilla already defines a Gson

So why do we have our own 😄

[fleet hatch]

True, patching the additional type adapters into vanilla's Gson sounds reasonable. We still need the separate UnbakedModel version of BlockModel.fromStream() though

[royal solar]

Thoughts on moving all our data providers to the client datagen event?

[fleet hatch]

I wouldn't be opposed to it

[fleet hatch]

Btw, I've been digging into the dynamic fluid container model, in part to see whether we can abuse ItemModelGenerator instead of rolling our own element generator for the front and back face (spoiler: we can't ), and I realised why the weird additional quads on the left and bottom happen: the dynamic fluid container model supports three layers (base, fluid and cover). The base is the "background" (in the test case just the empty bucket), the fluid is what the name implies and the cover is an additional overlay added on top of the fluid (could be used for containers with a "window" in the front through which the fluid is visible). For the test container, the cover is a solid white square with just the fluid area cut out, which of course doesn't match the bucket's shape. Ignoring the fact that the cover is utterly useless for the bucket, this would likely be fixed by making the cover mask fit the bucket's outlines.

[royal solar]

sad for item model generator

[royal solar]

and yeah I figured that out for the test but was like 🤷

[fleet hatch]

Fair. I'm playing around with some more stuff on the dynamic fluid container model, particularly to fix the alignment of the masked layers with the base layer which is broken due to the sprite expansion applied to the front and back quad of the base layer

[royal solar]

playing some factorio for a bit I'll check back in 1-2 hours 😄

[fleet hatch]

Sounds good, have fun 😄

[royal solar]

not sure about doing all of the datagen in the clientData run 🤔

we'd be missing out on createBlockAndItemTags - not much but it's a sign that it's not the right thing to do 😄

so I think we can keep them split; however I will change the output of server datagen from generated/ to generated/server/; and change the client to generated/client/

even better: generated/client_resources/ and generated/server_resources/

[ashen cedar]

ok but again, what do we gain from splitting them

I still feel like it's pointless

[royal solar]

we should support modders splitting them at least, since they correspond to different main classes in vanilla

I don't want to move createBlockAndItemTags to be available in the client event

but let's discuss!

[ashen cedar]

I don't know what to think about those helpers

since splitting is arguably pointless at least currently we should be recommending that modders always use client IMO but even if so, I don't like the idea of adding helpers that are restricted to the virtual separation of the datagen types

[royal solar]

that is the only helper that is server-exclusive

[ashen cedar]

either both or neither, at least with the current state of the mojang code

[royal solar]

how about we delete includeClient and includeServer?

they are super pointless

does anyone ever disable them

[ashen cedar]

certainly you shouldn't be able to disable them when the events are split right?

[royal solar]

you can in vanilla

for example if you only pass --reports, it will not include server providers

[ashen cedar]

okay vanilla is stupid, what's new

okay vanilla is stupid, what's new

just burn those methods

they make very little sense and add a lot of characters for a niche usecase

[royal solar]

yes

we should probably disable dev tools and reports generation too

but whatever, that's easily ignored anyway

[ashen cedar]

what reports

[royal solar]

vanilla allows you to pass --dev and --reports

that info is exposed in the event

the best part with merging the data runs is that I can now call the run data again 😄

[royal solar]

OK done

[fleet hatch]

Now I'm happy with it. No more pixel alignment issues 😄

[royal solar]

nice 😄

[deft kindle]

does neo alter the water bucket texture?

[fleet hatch]

No, that's a custom bucket item with a special model that generates the fluid quads from the actual fluid texture and a mask texture

[deft kindle]

ah ok

[fleet hatch]

Btw, Tech, what's up with the remaining server Main reject?

[royal solar]

we can delete it

I am reintroducing a dedicated interface for custom UnbakedModel loaders. Using JsonDeserializer is bad because:

1. JsonDeserializer receives a typeOfT parameter that doesn't make sense in our case
2. JsonDeserializer receives JsonElement but we always provide a JsonObject, forcing every loader to cast the element to an object
   cc @thin dawn

[fleet hatch]

Sounds good to me. Can you wait on that for ten seconds, I'm moving the custom Gson to BlockModel right now 😄

[royal solar]

wait

I found a little trick for that

    @SuppressWarnings("deprecation")
    public static final Gson INSTANCE = BlockModel.GSON.newBuilder()
            .registerTypeHierarchyAdapter(UnbakedModel.class, new UnbakedModelParser.Deserializer())
            .registerTypeAdapter(Transformation.class, new TransformationHelper.Deserializer())
            .create();

which I think is acceptable

[fleet hatch]

Clever, I like that

[royal solar]

I also tried to skip the UnbakedModel adapter and directly check for custom loaders, but we need to pass a DeserializationContext to the loader and I can't instantiate one in a nice way

[fleet hatch]

I've adjusted the Gson creation, do you mind if I push that? I also moved the registration of builtin loaders back to ClientNeoForgeMod where it was previously

[royal solar]

sure go ahead

[royal solar]

unless we pass the Gson directly to the loader?

the deserialization context is just a wrapper around the Gson

we can also document why we pass the Gson (i.e. it provides access to vanilla's serialization format for a number of types)

[fleet hatch]

Hmm. Where did you try to put the loader check?

[royal solar]

in UnbakedModelParser#parse

we'd have to read to a JsonElement first and then call .fromJson

[fleet hatch]

Ah, makes sense. Not sure that saves us any relevant amount of code. The custom Gson also has support for deserializing Transformations (which just makes me realize that we need to directly patch BlockModel.GSON to not completely break BlockModel.fromStream() for ML mods or mods which intentionally want to avoid deserializing a loader)

[royal solar]

I'll try it after I finish the UnbakedModelLoader refactor

can you push your stuff? 😄

[fleet hatch]

Will do

[royal solar]

oops I made BlockModel.GSON deprecated

I don't think it's necessary so I'll revert it

in fact, there are good reasons to make it public

I quite like this:

public interface UnbakedModelLoader<T extends UnbakedModel> {
    /**
     * Reads an unbaked model from the specified JSON object.
     *
     * <p>Access to parts of the vanilla {@link BlockModel} format is possible by using the {@link BlockModel#GSON} instance.
     * For example {@code BlockModel.GSON.fromJson(<sub object>, Transformation.class)} to parse a transformation.
     */
    T read(JsonObject jsonObject) throws JsonParseException;
}

that's also quite a bit simpler imo

[fleet hatch]

If you remove our Gson instance, remember to move the Transformation deserializer over

[royal solar]

yeah I did

goddamn spotless is so broken with the configuration cache it's not even funny

[fleet hatch]

lol

[royal solar]

every other run it asks me to delete my configuration cache folder

[fleet hatch]

I'm currently looking over patches generated from my current local state and cleaning up things like import changes (at least outside things you're still touching). Looks pretty good so far though

[royal solar]

ok pushed

[royal solar]

great! thank you

once we have patches generated (and they pass spotless) I think we can squash and rebase

[fleet hatch]

Sounds good 👍

[fleet hatch]

What the heck is the point of this patch: https://github.com/neoforged/Kits/blob/1.21.4-pre1/patches/net/minecraft/server/network/ServerGamePacketListenerImpl.java.patch#L66?

[plush inlet]

I can't quite tell

it seems to have been added during 1.20.1 porting by Orion in https://github.com/neoforged/Kits/commit/6ece5e8522e0d519fea896b07d222927e03242d9#diff-635cbed7327289050a039df736d104a7f9e4266ba87334d33a12dbb7341b8e31

@thin dawn do you have any recollection of why you did that?

otherwise, I think we should be able to just move it back to the method, which also means reimplementing the protection against block change sequence number replay

[radiant field]

Is it possible for a multiloader mod to load and bake an unbaked model without relying on any Neo extensions now or is bake still being overloaded?

[fleet hatch]

It's still overloaded because we need to pass down additional values collected from the parent chain (render type and root transform)

[radiant field]

Sad, I'll have to special-case Neo for my dynamic model loading then