1.21.5 Snapshots

[gleaming pendant]

maybe yeah; but we can always release anyway 😛

[drifting finch]

Apperently I am wrong

[floral mesa]

of note, the string concat is not cached so it becomes a hotspot in the allocation profiler 😂

[oblique tusk]

epic issue with sub-issues, or issues and a milestone?

[fleet dome]

That is actually also what I was primarily referring to lol 😄

[drifting finch]

Epic with sub

Lets try the new github system

See how it goes

[fleet dome]

At the very last that shoudl have been pre-allocated in a static final field somewhere

[drifting finch]

Pin the epic as well

[oblique tusk]

yea, I was about to say

what's the scope of the epic issue? immediate work to be done after porting, incl. new bugs?

[fleet dome]

oh oh, i was insulting the code and woke the upside down man

[oblique tusk]

so we don't have this issue live for months

[floral mesa]

my thoughts exactly

[sharp haven]

Pipelines do not say what texture they use, pipelines say what type of textures they expect to use. Pipeline is the shape of the thing, a render pass is the thing that fits that shape.

[oblique tusk]

speaking of the upside down man
that makes me wonder if Dinnerbone's actually Australian

but then again, Stockholm

[sharp haven]

(though today they don't even say what type because in reality everything today is the same type lol. But that's the intent.)

[oblique tusk]

(I was thinking in terms of timezone, and what ungodly hour it is in Australia)

[fleet dome]

Hm let me real hard at it again

[limpid crypt]

Okay, it's a conflict between the resource pack and NeoForge that's crashing the game.

[fleet dome]

Well you do define which samplers seem to be expected by the pipeline

.withSampler("Sampler0")
.withSampler("Sampler2")

But it might actually be faster to just bind them all from the renderstate ig

[oblique tusk]

( @drifting finch just in case you missed my question )

[sharp haven]

It's like this (intent, not current design):
Pipeline: I will use two textures and five uniforms and draw to this one colour texture with this other depth texture. This is how I will do that: [stuff]
Render pass: I will now use this pipeline. Here are the inputs the pipeline wants.

This is a fairly common API design from the last... I dunno, 10-15 years? It's not at all unique to MC.

[fleet dome]

Yes, what I meant was that you could check the key set in the pipeline for which samplers to fill, but it's not really a concern.

However, I'd recommend doing a private static final String[] SAMPLER_NAMES = IntStream.range(0, 12).mapToObj(i -> "Sampler" + i).toArray(String[]::new)
and pick from that to avoid the 12 string allocations for each draw

[sharp haven]

That's true, but ideally we don't even use strings here at all 😄 (but that's a future concern)

[fleet dome]

Heh, it was just the first thing that came to mind when I saw the loop 😄

[sharp haven]

(also only a few places use/expect "Sampler" + i, it's just awkward code that was moved from somewhere else and hasn't been rewritten yet)

(well, I say a few, it's the majority - but it's not the only pattern)

[fleet dome]

Is it reproducible with that resource pack? Does it happen with the resourcepack in Vanilla?

[oblique tusk]

Reloading ResourceManager: vanilla, file/Energy Shaders [Java] v. 2.7.0.zip, mod_resources, mod/neoforge

[fleet dome]

Unless you're swapping out shaders, I am not sure how that's supposed to happen

Tzzzzzzzzzzz

[merry falcon]

Remember when you said you did nothing? So you had a custom resource pack AND summon a warden. Those are two VERY important things to mention in a bug report. We should be told EVERYTHING and WE will decide what is relevant or not

[warm night]

Hey sci I can help with the issue creation and stuff if you want.
yes the Epic ticket is a great idea to connect together issues, we could also use the milestones in github to get a grouping

[limpid crypt]

https://github.com/neoforged/NeoForge/issues/2046
@drifting finch Is it OK?

[warm night]

we could also consider making a bug report template that has mandatory questions to fill out to make the reporting process easier

[oblique tusk]

that is an idea I've been comtemplating the past few months on and off -- using the issue forms system that GH has instead of our current issue desc template

[true turret]

descriptor set (layout)?

[oblique tusk]

we'd have to try to ensure the epic ticket has a bounded lifetime, so it doesn't just live forever
like, an epic ticket for 1.21.5 on-release bugs is good, then closing it after all the initial bugs get fixed and any more reported bugs are processed as normal

milestones, I still want to use them more, particularly for progress towards stable builds, but that's partly dependent on our stable build policy (or lack thereof)

[sharp haven]

Right now I guess it is also a single descriptor set layout but I don't like that. However, we don't even have a concept of a UBO yet, so...

This is what works for today. 😄

[true turret]

Push constants and inline ubos are a thing too

[proven compass]

simply start rendering over from scratch

gets rid of all the systems u dont like

[oblique tusk]

if only they could

[sharp haven]

I mean, I did kinda rewrite anything that touched opengl! Does that count?

[oblique tusk]

[proven compass]

not enough, purge the entire client package

split sources allow for no errors on the server from that

[true turret]

UBOs would be real nice though, looking squarely at the matrices

[fleet dome]

I made that point today that once mods ported to 1.21.5, if everthing is well there shouldn't really be LWJGL/GL Constant usage in there. We might have to add more state to Pipeline though

[sharp haven]

In reality many big mods will ignore this and continue to raw opengl because more control / less work / version compatibility with earlier MC. It's a shame but whatever, I've done what I can to give the ecosystem a lil push.

[fleet dome]

Well, I haven't really started yet with AE2 but plan to do so, soon.

I think it's actuall nicer this way

But I already know that I am using some GL state you aren't 😄

GL20.glLogicOp(GL20.GL_XOR);
GL20.glEnable(GL20.GL_COLOR_LOGIC_OP);

😄

[true turret]

Should be easy enough to add those, no?

[sharp haven]

Yeah and that's fair, I didn't want to add too much that we aren't actually using ourselves, at least not in this version

[fleet dome]

I just don't know if it's worthwhile to add all of those to the RenderPipeline, that'll be the big questionmark for us in 1.21.5+

what the policy is on which GL state do we add

Adding this stuff is not on you lol, we'll have to find a good middle ground, I guess

[sharp haven]

One day I'll look into moving this into an external library with a much more expanded api but it'll be after we know what that api actually settles on

[true turret]

IMO, all of it

[fleet dome]

If it's an abstraction, you'll find some stuff only GL has that other APIs don't

I already had some trouble with that with Stencil

Like which design-route do you take? Model it around GLs model? Lowest common denominator?

[true turret]

Adding stuff like that before Mojang/Dinnerbone gets to it (and maintaining it as Dinnerbone breaks it) is specifically what I want to do.

[fleet dome]

Maintainers should get together to decide on how heavyhanded we want to be here

[oblique tusk]

i'll arrange the flights /j

[fleet dome]

Mandatory Webex Meeting! Save travel cost!

[floral mesa]

...webex?

please no

[fleet dome]

Alright alright, Skype for Business

[true turret]

teams

[fleet dome]

Teams is actually good though

[floral mesa]

@steering_council take away his maintainer role please /s

[true turret]

is it though?

[gleaming pendant]

just because you want to do it doesn't mean it's a good idea; it should be discussed

[fleet dome]

yes

Since the React port, it is

When it was still written in Angular 1

Those were the hell-times

[true turret]

yes, im aware, but that doesnt mean its not what i personally want to do

[gleaming pendant]

I read "I want to do" as "I want to do as a NeoForge maintainer" 😛

if that was not the point just carry on

[fleet dome]

well the discussion part I also had in mind

if we want to keep mods in the Pipeline ecosystem (and IMHO we should since it makes graphic issue debugging just soooo much easier)

we have to have a plan for how to deal with GL state mods use that pipelines don't cover

[oblique tusk]

sounds like a topic for a new brainstorming thread?

[fleet dome]

probably, yeah

[oblique tusk]

"rendering pain: OpenGL state"

[fleet dome]

HOW to do it, is actually pretty clear as an overall strategy (except for how to model each individual state maybe)

[floral mesa]

it's highly unlikely we can enforce this

[fleet dome]

we cannot enforce anything

but if we don't add state that's missing

we've decided for them NOT to use it

[true turret]

^ this is why i want to add it

[fleet dome]

For UI rendering at least it's only half-true, since you can combine the two, but it'll still be prone to state set by mods leaking and messing up other mods.

[sharp haven]

My too-scents here is to try not to add anything until you see it actually being used, just to avoid "api for the sake of it" - but I largely would trust whatever Rogue's idea of the api looks like, it probably matches my own decently enough

[true turret]

you are aiming for VK-ish, yes?

[fleet dome]

adding state to the pipeline is easy enough, my initial stencil design did that. but yes, there's a LOT of VERY obscure GL state

that almost no one is using

[sharp haven]

It's certainly VK inspired because that's my familiarity but that doesn't mean it should reflect VK exactly. At the end of the day, it's GL, and the api shouldn't impose any tax by forcing GL into a box that it doesn't fit in

[true turret]

so, ish

[sharp haven]

Yeah, heavy on the ish 😄

[fleet dome]

Personal take: Also check what DX12 does (and maybe metal)

[sharp haven]

(if you can make it line up with VK principles then that's ofc great, because they're mostly great principles.)

[fleet dome]

In terms of exposing/modelling specific state, that is.

[gleaming pendant]

interesting way to write two cents 😄

[true turret]

IMO, metal actually makes mose sense to check against here, because of the eventuality of running VK on top of Metal, and making MoltenVK's life easier with it

[fleet dome]

probably, yes

but I have no familiarity with how DX12/VK driver support looks like on Windows these days

I am outdated and still hung up on the whole ANGLE situation for WebGL 😛

[true turret]

D3D12 is W10+, VK is W7+

[fleet dome]

Well that's not what I meant

the driver quality on Windows was what I was referring to

[true turret]

oh, afaik both are perfectly fine

[fleet dome]

Is Vulkan fine even for Intel iGPUs nowadays?

[true turret]

i can check that

[fleet dome]

well, can you with a large enough sample size to know the random crashes it might have? 😄

[true turret]

a lot of games run on D3D, and the front-end of the API is implemented by MS
VK is done per-driver, but has much less wiggle room in the spec as compared to GL, and is used by some major games (DOOM)

also, compared to GL, VK's validation layers are extremely helpful for keeping you within spec

[sharp haven]

DX is largely flawless, it's the gold standard for most manufacturers
VK is largely great with the exception of early drivers. There's not much room for them to go wrong and the conformance tests help a lot.

[fleet dome]

Ok so better situation than with GL at least.

[drifting finch]

Much better

[true turret]

its much much better than GL

[sharp haven]

One thing that a lot of our community forgets when they go "omg mc is switching to vulkan" is that our userbase doesn't all support vulkan though. Our minspec definitely doesn't.

[fleet dome]

Quick google shows Vulkan starts later for iGPUs than DX12 does. I don't remember how old Intel® HD Graphics 6000/5500 was

[true turret]

validation layers and compliance testing is a pretty integral part, in no small part because of the issues with GL drivers

[drifting finch]

DX Strict setup did kind of made it flawless.

Vulkan contained a lot of the learnings of the organizational and structural defecits of OGL

[true turret]

afaik SPIR-V's existence is so that you dont have GLSL compiler weirdness on one specific driver

[drifting finch]

Well the later versions of DX

[fleet dome]

Yes, I personally also looked at DX11 support for what I modeled the stencil data model on lol

[true turret]

DX is largely flawless
except when its not, ahh stories from work

[drifting finch]

DX11 and 12 are close to perfect and complete when it comes to standardized rasterization

[drifting finch]

It highly depends on your viewpoint and the way you look at it

[true turret]

crashing the driver because internal issue (not our issue)

[fleet dome]

Well the DX driver is more heavily utilized by Windows so it'd be expected to be more solid ig

[true turret]

both are very solid in general though

[fleet dome]

Especially since WebGL goes through DX

[sharp haven]

I say largely 😄 I run into issues with WARP a bunch <_<

[true turret]

you mean UHD 550/600? thats skylake

[fleet dome]

https://www.intel.com/content/www/us/en/support/articles/000005524/graphics.html

[true turret]

consult table (its correct) https://en.wikipedia.org/wiki/Intel_Graphics_Technology#Capabilities_(GPU_hardware)

[fleet dome]

That's Broadwell

[frail patio]

I have a question I have been wanting to ask Dinnerbone; it's not exactly related to the rendering backend but I'll ask now since Dinnerbone's here. What is the purpose of BlockState.emissiveRendering if models can define their minimum light since 1.21.4?

[true turret]

oh, yea, i forget about broadwell

there were very few consumer chips with it, except for laptops

[sharp haven]

No clue, haven't touched that.

[true turret]

i think one i5 and one i7 on LGA 1150

[sharp haven]

I'm very laser focused right now sorry, I only know what I'm tearing up 😄

[true turret]

i do actually need to get one of those CPUs

[warm night]

(bold assumption that code has a purpose)

[merry falcon]

it’s about to be put on the trello board now right? Maybe not trello…

[frail patio]

Ah ok. Code that uses the method was refactored in rc1 so someone else must have done that.

[warm night]

more like a jira ticket...

[true turret]

but also, only since Skylake is there support on windows for VK, and its VK 1.3 too which is real nice for featureset (thats better than what Kepler has)

[merry falcon]

Nope can’t be jira. Jira is unusable

Is the search even working again for jira

[true turret]

[warm night]

only the top one not the one inside the issues view

[merry falcon]

Are you serious

STILL broken

[fleet dome]

DX11 Blaze3D backend next!

[limpid crypt]

https://mojira-viewer.jacobsjo.eu/ is better

[true turret]

poke Dinnerbone for that, not me

[fleet dome]

Fun side project 😄

[oblique tusk]

random observation: MC Bedrock Edition's logo in the Mojira main page has the "Bedrock Edition" subtitle, despite official branding iirc being that Bedrock no longer has that subtitle in its logo

I know it's to differentiate the two, but an odd yet neat detail regardless

[true turret]

im not touching D3D if i can help it

[sharp haven]

I also. 😄

[fleet dome]

I did port an ancient game from DX8 to DX11 once, I think it was fine 😄

[true turret]

it is notable that in general D3D12 and VK are pretty similar APIs, at least in overall structure and recommendations on what to do

[fleet dome]

Yes

[true turret]

different terminology is used between them, but its broadly generally similar ideas

[compact hare]

But have you ever used D3D12 via a pure C interface?

[true turret]

like, memory is handled quite differently, but in both cases the core (recommendation for D3D12) allocate big block, then sub-allocate that is the same, for the same driver reasons

[warm night]

no idea why yall are so negaive the dx12 devs are nice and helped me with my uni assignment....

[true turret]

im sure the devs are great, D3D on linux when?

[warm night]

he he he

[true turret]

counterpoint, vulkan.hpp

[compact hare]

The point is in Vulkan you can as it is specified as a C API, IIRC
D3D really isn't

[true turret]

Vulkan is fully in C, and some old C iirc too

for portability reasons

[compact hare]

Yup

D3D is built on top of COM so you can use it

But it's really not pretty

[fleet dome]

So beyond stencil I haven't checked what else is different between VK and D12. I should check Metal for that, btw.

Oh great. Searching for Metal Stencil gets me a lot, just not what I am looking for

Ok, Metal has the Vulkan model

[true turret]

wdym Vulkan model?

for D3D12 its also part of graphics pipeline like it is for VK

[fleet dome]

No I mean the particulars of the state

D3D12 does not allow separate write/read masks per face (front/back face)

Vulkan / Metal do

(It's also dynamic state for Vulkan in addition to the pipeline)

[sharp haven]

I am blissfully ignorant to Metal but my understanding is that it's very similar to vulkan and that's what allows moltenvk to be mostly thin

[true turret]

requires VK 1.3 or VK_EXT_extended_dynamic_state

[fleet dome]

Yes 1.3

[true turret]

except for sparse shit

[true turret]

GCN 1/2/3 and Kepler are VK 1.2 only (but afaik do have the extension)

^ on windows, GCN 1/2/3 are 1.3 on Linux, Kepler isn't because Nvidia

oh, MVK also doesnt have VK 1.3 yet, but does have extended state

[gleaming pendant]

intellij did this:

@Override
    public <T> HolderLookup.@Nullable RegistryLookup<T> resolveLookup(ResourceKey<? extends Registry<T>> key) {

thoughts on the @Nullable position? 😄

[true turret]

i understand why, but also no

[fleet dome]

That is seriously valid?

[drifting finch]

Yep

[fleet dome]

That honestly makes no sense to me 🤔

[drifting finch]

The nullable is there to indicate that the inner class can be nullable

But when it it s not null

Then the outer is also not null

This is, granted a core design of java

[fleet dome]

It's a method return type

[drifting finch]

But it is the logic behind the type

Yes

Unless RegistryLookup is static

[fleet dome]

you can't return those separately, so what's the point

[drifting finch]

Then it makes no sense why it would put it there

Hmm

[oblique tusk]

depends on the location that the annotation is valid for

[fleet dome]

Hm

I suppose so

Semantically for Nullable at least it's pointless, from a type system perspective there may be a point

I suppose this is coming from the type composition rule that also allows byte @Nullable []

[drifting finch]

Yeah

[fleet dome]

@lunar saffron Soooooooo

Your 2nd patch file referred to the 1st, can you just attach both 😅

Or make a branch and push your stuff there?

[lunar saffron]

Wut, that makes no sense. But sure, I can make a branch in a second

[fleet dome]

Maybe I misread it, but it looked like it reverted some stuff with options

that I interpreted as being your intermediary fix before moving the modloading to the start

[lunar saffron]

Those are existing patches, not ones I made

[fleet dome]

OH

lol 😄

[lunar saffron]

That's how we previously handled reloading only the keybinds from file after modded ones were registered

Still want a branch for it? If so, I'll have to shelf some stuff I'm experimenting with right now 😄

[fleet dome]

Nah ok

So, do we kill LogicalSidedProvider?

This thing also seems at odds with split sources, tbh.

[gleaming pendant]

I am killing it already

[oblique tusk]

murrdurr

[gleaming pendant]

btw I got the client to compile and launch with split sources

I basically redid the split from scratch using the existing PR and going through each file 😅

the server fails with

[23:32:36] [main/ERROR] [minecraft/Main]: Failed to start the minecraft server
joptsimple.UnrecognizedOptionException: gameDir is not a recognized option

but that seems to be a general issue with the port? 😛

[fleet dome]

There is indeed no gameDir for the server

One of the two previously didn't validate CLI options

I don't remember which one

[gleaming pendant]

huh

[fleet dome]

Yes the server previously ignored everything unknown

If that was removed we probably have bugs in our launch targets

[gleaming pendant]

ah ok

yeah, should be an easy fix

[fleet dome]

since we rely on gameDir we might have to strip it

[gleaming pendant]

on it 😄

[lunar saffron]

The gametest server still does

[fleet dome]

That's an FML fix

[gleaming pendant]

wait we rely on it??

[fleet dome]

If I am not fully mistaken FML does

[gleaming pendant]

it's always . anyway

[fleet dome]

Or was it ML

So, the proxy you can apparently set on the MC command-line. we're kinda ignoring that, ig?

@lunar saffron so I'd actually move it as far up as this:

Since for the dedicated server, we've been doing it here:

[lunar saffron]

Sure, seems reasonable to me

[compact hare]

The point is that the annotation is next to the type that it is affecting. It is actually required if you want to e.g. @NotNull List<HolderLookup.@Nullable RegistryLookup<@UnknownNullability T>>

Because otherwise there could be potential ambiguity

[gleaming pendant]

couldn't you @NotNull List<@Nullable HolderLookup.RegistryLookup<@UnknownNullability T>>?

[compact hare]

Compile error

[gleaming pendant]

what 😄

[compact hare]

Try it

[gleaming pendant]

ugh

[compact hare]

Because in generics, the two things are different

HolderLookup. is a qualifying type

[oblique tusk]

get hecked

[compact hare]

And RegistryLookup is the actual type that you can annotate

[fleet dome]

Okay but that means it's just crap syntax and not as stupid as I initially thought

[compact hare]

It's the same crap syntax you need for inner classes

[fleet dome]

Because it is an inner class?

[compact hare]

class Foo { class Bar {} }
void main() {
  Foo f = new Foo();
  Foo.Bar b = f.new Bar();
}

[fleet dome]

oh you mean that stuff

yes

but from a type reference perspective for devs, it's not too dissimilar from a fully qualified class-name

[gleaming pendant]

@fleet dome ML has this: gameDirOption = parser.accepts("gameDir", "Alternative game directory").withRequiredArg().withValuesConvertedBy(new PathConverter(PathProperties.DIRECTORY_EXISTING)).defaultsTo(Path.of("."));

passing nothing does indeed work

[fleet dome]

and you dont put the annotation like this: java.lang.@Nullable String

[compact hare]

Actually you do

[fleet dome]

Are you for real 😄

[compact hare]

Like, as far as Java is concerned

Try it in a generic 😛

It'll likely complain

I have not tried it, I admit

But I believe List<@Nullable java.lang.String> will complain

[fleet dome]

Well I am not even about generics

[compact hare]

Because as far as Java is concerned, the annotation should be on String

[fleet dome]

since the initial case wasn't a generic

[gleaming pendant]

BTW I wonder if we should remove gameDir entirely

[compact hare]

I see what you mean

The answer to that is likely dumber

[gleaming pendant]

let me check if all Mains default to .

[compact hare]

Backwards compatibility

Since they had defined annotations for methods and annotations for parameters

And then they had to retrofit them

[gleaming pendant]

ugh we patch this in the datagen Main:

+        OptionSpec<java.io.File> gameDir = optionparser.accepts("gameDir").withRequiredArg().ofType(java.io.File.class).defaultsTo(new java.io.File(".")).required(); //Need by modlauncher, so lets just eat it

[compact hare]

I think

[fleet dome]

My personal opinion is that we shoudln't

Vanilla Launcher profiles pass "--gameDir", "${game_directory}",

[oblique tusk]

defer to after porting, if at all

[fleet dome]

well the dedicated server currently doesn't launch anymore 😄

well although Tech did fix it

But seriously tech, relying on working dir actually sucks

[gleaming pendant]

this only matters for dev launches

which should have the right directory set

it is ridiculous that we are passing --gameDir .

[fleet dome]

not for tests, no

I suppose those are handled differently, ok.

[gleaming pendant]

and you think that passing . explicitly is any better? 😛

[fleet dome]

no, it should pass the placeholder 😄

and the placeholder should be interpolated correctly

[gleaming pendant]

launcher profiles are in principle not related to the userdev config

[fleet dome]

but since we might not have implemented it (or NG might not have), well yes. if it fixes the problem at hand, go with remove it for the dedi for now

[gleaming pendant]

nah I'll remove it everywhere + remove the corresponding datagen main patches

since I want it to be consistent and I don't want want to patch in the argument in the server main

[frail patio]

Does that weird MCP start class still exist?

Yep https://github.com/neoforged/NeoForm/blob/1.21.5-dev/versions/pre/1.21/1.21.5-rc1/inject/mcp/client/Start.java

[proven compass]

pops up in the log every time i reload gradle

Cannot inject duplicate file mcp/client/Start.class

[gleaming pendant]

yeah it's really annoying 😄

[fleet dome]

Alright. So. Events

• ClientStartedEvent right before the client ticks for the first time. This is where Fabric also emits this.
• ClientStoppingEvent right before the client starts freeing its resource (head of #destroy, same as fabric)
• ClientStoppedEvent after everything has been freed, right before System.exit(0)

[gleaming pendant]

ah yeah those don't exist yet

sure then

[fleet dome]

we don't introduce an event to get a half-constructed minecraft instance, yes? Since people can shoot themselves in the foot with Minecraft.getInstance() just fine, while the constructor is still running 😄

[gleaming pendant]

🤷 maybe find a reasonable use case for these events idk

I suppose they're broad enough that something will always come up

[proven compass]

is it just to have parity with fabric

[lunar saffron]

We currently have a GameShuttingDownEvent, that's shared between dedi server and client though. On the client it fires from Minecraft#stop()

[fleet dome]

yup I saw that one, it doesnt give access to the client though

and the dedi has specific events too

[frail patio]

I think separate events are cleaner

[fleet dome]

Rather, there are even more:

[lunar saffron]

That's fair, yeah

[lunar saffron]

I mean, yeah, why would it? There is only one

[fleet dome]

Parity, really.

[lunar saffron]

Fair

[fleet dome]

Well good enough, I think.

oh lol, I didn't even know we had net.neoforged.neoforge.client.StencilManager

[gleaming pendant]

wtf is that

that's absolutely cursed 😄

[lunar saffron]

Speaking of stencil: I just spent quite a while toying around with the pipeline modifier idea, taking into account the discussions from earlier. Here's the implementation I came up with. For demonstration I opted to add both the lambda-based approach for wrapping the actual rendering "task" and the AutoCloseable approach I brought up.

[gleaming pendant]

https://github.com/MinecraftForge/MinecraftForge/commit/5ca854efa85e49758d63acd4d7a159324701d6a7

incredible 😄

[fleet dome]

Yes, that is very nice indeed

I wouldn't use the AutoClosable, personally

yeah the implementation doesn't really conform to AutoClosables intent either hm

[lunar saffron]

I like the idea of it because it completely avoids allocations. The only thing I'm a bit torn on about it is that it currently exposes the entire PipelineModifierStack instead of a "useless" object you can only close but not do anything else with

[lunar saffron]

In what way?

[fleet dome]

repeatedly calling close() will ultimately lead to an exception

while AutoClosables close is intended to be idempotent

Regarding the modifier itself: as I said, it's a lot less useful if you pass in a builder

And yes, I am aware that we can't really apply a location if we don't.

[lunar saffron]

I guess we could enforce returning a builder

[fleet dome]

If it was @Nullable, potentially

You do want the ability to "pass" on a transform

[lunar saffron]

Hmm, true

[fleet dome]

which for a Function<RenderPipeline, RenderPipeline> you would do by just returning the input

the only downside to doing that is not auto-modifying the location, ig

We could potentially also pass in a Supplier<RenderPipeline.Builder> builderSupplier that does that internally

as a second argument, I mean

[lunar saffron]

Hmm. I would prefer preventing a modifier from setting a location

[true turret]

On the other hand, LWJGL's MemoryStack does exactly this

[fleet dome]

It woudl be better but I think we're somewhat constrained here, sadly

[lunar saffron]

I don't think the idea of returning a nullable builder is that bad, just looks a bit wonky in the generics: Function<RenderPipeline, RenderPipeline.@Nullable Builder>

[fleet dome]

This does disable the ability to return a pre-existing pipeline

[lunar saffron]

Hmm, that's true, yeah

Another option would be a BiFunction<RenderPipeline, ResourceLocation, RenderPipeline> and hoping people use the provided RL as the pipeline's location when they "mutate" the provided one

[fleet dome]

beats having to manually build it

[lunar saffron]

Yeah

[fleet dome]

There's also an additional check we can make, I think 🤔

.... build modified location ...
var modified = modifier.apply(pipeline, modifiedLocation);
if (modified != pipeline && modified.location().equals(pipeline.location())) {
.... emit a warning that the modified pipeline should use the modified location ....
}

had to check if locations are mandatory (they are)

[lunar saffron]

That sounds reasonable and also avoids getting annoying when a pre-built pipeline is returned intentionally

[fleet dome]

we might even consider throwing, tbh.

if someone is returning a pre-built pipeline that has a location-clash with some other pipeline, then there's something afoot already

[lunar saffron]

That's true, yeah

Here's v2

[fleet dome]

https://github.com/neoforged/NeoForge/pull/2049/files

Never really thought about it, but there aren't any client-side registries

right?

[lunar saffron]

There kinda are for all the item model things, they are not proper registries though, they use ExtraCodecs.LateBoundIdMapper. I had thought about using that but, at least without patching it in, there is no way to look up an entry from it directly, it's only accessible via the codec it creates

[fleet dome]

Yeah, a wonder we can even use registry keys, heh

Well anyhow, I am already in the nitpick stage which is probably a good sign 😛

[lunar saffron]

Vanilla does the same resourcekey-without-registry shit for recipes and advancements 😅

[fleet dome]

Oh right, I ran into that for recipes, you're right 😄

[lunar saffron]

If you're happy, then I'll make a PR for it against the porting branch

[fleet dome]

Stuff like:

• We dont need to extend BiFunction if we override the apply method anyway (just remove the extends, add @Functionalinterface)
• Make some stuff @ApiStatus.Internal
• Still not a fan of auto-closable 😄
• If you want to keep the AutoClosable stuff, at least make pushPipelineModifier return AutoClosable, not the concrete type

And remove push from the name, since if there's a push, I'd expect a pop 😄

[lunar saffron]

I wanted to return AutoCloseable but I can't due to the default signature containing throws Exception, I'd have to use an intermediate type that removes the exception

[fleet dome]

Otherwise, I'd say yes, make the PR

[fleet dome]

That'd still be preferable to returning the full type

[lunar saffron]

Then I'll do that

[lunar saffron]

What would you use instead? Calling it applyModifier() as well is confusing in the presence of the method called from the render pass

[fleet dome]

withPipelineModifier?

[lunar saffron]

Sounds good

[fleet dome]

I personally would have been fine with the push/pop approach 😛

[lunar saffron]

I would like to prevent the kind of issues people regularly run into with mismatched push/pop on PoseStack 😄

[fleet dome]

we can add an upper limit of ~100 to this stack

unlike posestack, the stuff you're pushing here has an ID

so figuring out who screwed up is easy

[lunar saffron]

That's true, yeah

[fleet dome]

I'd just plonk an upper limit on it and throw (including the list) when it's exceeded

[lunar saffron]

Another option could be to do an end-of-frame check to see whether the stack is empty

[true turret]

After events it could also be checked

Ie: after render level/etc events fire, and other similar points

[lunar saffron]

That could become problematic in certain scenarios, i.e. when a mod renders a fake level with a permanently active modifier and fires the level stage events for that. Checking once in RenderFrameEvent.Post is the easiest option and the one that's the least likely to cause issues

[fleet dome]

Yeah we could do that too

[lunar saffron]

Here we go: https://github.com/neoforged/NeoForge/pull/2050. I opted to remove the AutoCloseable option and instead add push/pop methods in addition to the lambda variant

[true turret]

doesn't this still need to solve the identity problem in GlDevice?

[lunar saffron]

That's solved by caching the modified pipelines instead of having the modifiers spit out a new pipeline every time

[true turret]

ok, yea, overall, i like

thought that occured from this though, should any attempt be made to catch someone dynamically creating RenderPipelines?

there is absolutely nothing stopping you from creating them often, and even if actually identical, that will leak them all until the next resource reload

[lunar saffron]

Hmm. In theory that would be a good idea, I'm not sure how practical it would be to actually check for this though

[true turret]

for the obvious case, it would probably be good to have the GlDevice check for an existing pipeline with the same id

at a minimum, they should be unique for debugging purposes anyway

[lunar saffron]

Yeah, that case would be trivial to check for

[gleaming pendant]

@sterile abyss it is time to fix gametests

[drifting finch]

@gleaming pendant Your updated PR looks good

I am going to try to run it through the NG and MDG mdks later today. Could you do me a favor, and also run this on your end as well. Then we have at least 2 systems with both plugins where we know it works.

Not sure if it is worth testing with Architectury Loom?

[gleaming pendant]

we can discuss in #1330296017005842513

[drifting finch]

Sure, already answered there

[sterile abyss]

yeah yeah I'm on it

[sterile abyss]

SOE

[drifting finch]

Oeps

[sterile abyss]

but that worked before...

why does it now want to call itself

oh...

generic signature change in mc's

[12:10:31] [Server thread/INFO] [minecraft/GameTestServer]: 6 required tests failed :(
[12:10:31] [Server thread/INFO] [minecraft/GameTestServer]:    - neotests:tests/slimesplitevent: NBT Data not copied on tick 0
[12:10:31] [Server thread/INFO] [minecraft/GameTestServer]:    - neotests:tests/shieldblockevent: Entity should be holding Stone at -6377834, -56, -3667736 (relative: 1, 2, 2) on tick 102
[12:10:31] [Server thread/INFO] [minecraft/GameTestServer]:    - neotests:tests/testcauldroncapability: Expected a second invalidation after cauldron destruction on tick 0
[12:10:31] [Server thread/INFO] [minecraft/GameTestServer]:    - neotests:tests/testcomposterinvalidation: Should have invalidated a second time on tick 0
[12:10:31] [Server thread/INFO] [minecraft/GameTestServer]:    - neotests:tests/survivabilitydeadbushtest: Did not expect block to be Dead Bush at -6377826, -56, -3667647 (relative: 1, 2, 1) on tick 0
[12:10:31] [Server thread/INFO] [minecraft/GameTestServer]:    - neotests:tests/playerpickupxp: Entity test-mock-player failed test: experience level, expected: 4, was: 2 at -12755685, -114, -7335163 (relative: -6377842, -56, -3667581) on tick 45

progress

[drifting finch]

Good start!

[sterile abyss]

the invalidation tests not passing is

java.lang.RuntimeException: Incorrect direction param passed in
    at TRANSFORMER/neotests@1/net.neoforged.neoforge.debug.block.CanSustainPlantTests$CustomSuperSustainingBlock.canSustainPlant(CanSustainPlantTests.java:716) ~[%23157!/:?] {re:classloading}

why on earth does this.. throw?

the cactus can random tick and call that method with UP therefore throwing as cacti does occlude

[drifting finch]

Ask @merry falcon

[sterile abyss]

he's getting stabs no matter what he says

down to 4 failures

did mojang make dead bushes placeable on farmland this update?

yeah they did

down to 3

good thing we have tests since LivingShieldBlockEvent is not fired anymore

[merry falcon]

The test is not for growing cactus

It’s for placing the block and making sure it indeed can only be placed on a soil block and that can sustain is checking just below

It’s not for testing the entire growing. If something is ticking the test beyond the testing itself, something wrong overall

Plus look at that block class

It doesn’t even extend cactus. Therefore it can’t even GROW on its own

It’s just manual/auto placing plant test. Not a growing test

[merry falcon]

You’re gonna need to explain more instead of stabbing because what you described is completely nonsensical for what that test was for. We had issue where the wrong direction was passed to sustain block or wrong side block passed in. That test was to ensure we always pass side block. What are you even doing

Did you break the sustain block code path and then just adjusted the unit test to accept the incorrect behavior???

[drifting finch]

I guess we will see

[fleet dome]

In soviet neoforgia grunt stabs you.

[sterile abyss]

just because you're not growing it it doesn't mean it can't grow

the exception is inherently a bad idea because while the other tests run the entire game ticks

so if the cactus is placed as a one of the first tests until the other hundred tests run it might have randomly ticked since it's not cleared

and therefore it crashes the entire game

also known as a flaky test in the sense that it has a random chance of crashing the entire game

[merry falcon]

Maty, read the class of the custom block again

It does NOT randomly tick grow

It doesn’t have it implemented

It just extends Block

[sterile abyss]

what custom block

[weak lichen]

matyrobbrt

[Reference to](#1313560104359956542 message) #1313560104359956542 [➤ ](#1313560104359956542 message)why on earth does this.. throw?

telepathicgrunt referenced · Mar 23, 2025, 10:23 AM

[sterile abyss]

that's the... soil?

not the cactus you're placing

[drifting finch]

given the method is canSustainPlant

Yeah that is the soil

As in a method on the soil block

[merry falcon]

If the cactus grows, the soil won’t be calling canSustainBlock with the upper cactus because… it’s not next to the soil. The cactus right above the soil should be sustain

The more you explain this, the more you’re not making sense. What exactly is the crash

[sterile abyss]

the cactus checks if it can be sustained when it randomly ticks

[merry falcon]

Cactus calls the below block’s cacSustainBlock

Cactus on cactus calls below cactus’s. Cactus on soil calls soil’s

I’m not at my computer rn

[sterile abyss]

precisely, and since the cactus remains on that block after the test it tries to grow when it randomly ticks, checks if it can grow, calls the soil's method, the soil checks if the block can occlude, but since the cactus placed on top of it can occlude it crashes

[merry falcon]

Now I am starting to remember. What I did was surround the soil in stone blocks. Attempt the cactus place, then had the soil check its surrounding to make sure it was not passed in a stone side position and only the top position

You can adjust that to stay isolated in the test but the soils needs this side position validation or the test is useless

It’s a test that we pass correct block and position to the dirt block when placing plants

[sterile abyss]

in the survivability test? no, you're not placing any other block, but air doesn't occlude so it won't crash there

[merry falcon]

I’m fairly certain I did surround the dirt block

That was the point of the exceptions entirely

[sterile abyss]

not in this test, maybe in another one

[merry falcon]

For later reference, how do I get into the porting branch. Is it on the Neo repo or somewhere else. Never did work on porting branch before

[sterile abyss]

right now it's on neo

side note that whole test class is

700 lines of the same code written 14 times with small adjustments

[merry falcon]

If you have a better way, go ahead and refactor it but each plant has different requirements and placements

Sugar can needs water. Cocoa beans need side. Etc

[sterile abyss]

screm the blocked damage event is pain with the new component stuff

I'll have to settle with being able to customize the shield durability damage amount without knowing what the original durability damage amount would be (unless you calculate it yourself)

[13:46:27] [Server thread/INFO] [minecraft/GameTestServer]: 2 required tests failed :(
[13:46:27] [Server thread/INFO] [minecraft/GameTestServer]:    - neotests:tests/testcauldroncapability: Expected a second invalidation after cauldron destruction on tick 0
[13:46:27] [Server thread/INFO] [minecraft/GameTestServer]:    - neotests:tests/testcomposterinvalidation: Should have invalidated a second time on tick 0

now this

ah I know why

the tests are using destroyBlock which doesn't seem to trigger affectNeighborsAfterRemoval

after onRemove got turned into that, that method is behind flag checks so I'm not sure it's a good idea to have people rely on it to invalidate the caps considering it can be bypassed now, both intentionally but more importantly, accidentally

[sterile abyss]

although no, destroyBlock does seem to call that... something weird is going on

ah, after removal

yeah the block state affectNeighborsAfterRemoval receives is the old block state unlike onRemove which also got the new one so the migration to the new method in our patches was wrong

[14:14:18] [Server thread/INFO] [minecraft/GameTestServer]: 1 required tests failed :(
[14:14:18] [Server thread/INFO] [minecraft/GameTestServer]:    - neotests:tests/testcauldroncapability: Expected a second invalidation after cauldron destruction on tick 0

that's one fixed and one left

[14:19:19] [Server thread/INFO] [minecraft/GameTestServer]: ========= 132 GAME TESTS COMPLETE IN 13.87 s ======================
[14:19:19] [Server thread/INFO] [minecraft/GameTestServer]: All 132 required tests passed :)
[14:19:19] [Server thread/INFO] [minecraft/GameTestServer]: ====================================================

and fixed

[oblique tusk]

yay!

[sterile abyss]

https://github.com/neoforged/NeoForge/commit/84d2cb63783e285d1b6aefeed11efbf0893934e2 let's hope for build success

blergh right spotless

[oblique tusk]

get hecked

[sterile abyss]

well gametests are successful so yay

and gametests are fully fixed (SP too)

yw @lunar saffron

[oblique tusk]

now how about you patch the automation to not make labels that don't exist

[sterile abyss]

I fixed that already dummy

[lunar saffron]

Make sure to remove the relevant part of the LivingEntity reject to avoid confusion later on

[sterile abyss]

ah there was still a reject?

[lunar saffron]

Yup

There are also still two rejected hunks related to fluids, one in LivingEntity and one somewhere else

[fleet dome]

Uuuh maty, I am trying to get FML 7.x to build but Did you forget to run "git fetch --tags"?...
Git has still not fixed this. What was your workaround for this?

[sterile abyss]

use our action

[fleet dome]

Isn't main doing that?

I merged main into 7.x

The only difference seems to be this in release.yml

[sterile abyss]

oh then yes it should just work

[fleet dome]

I'll just try to stupidly rerun it then

Oh... maybe it's GU getting confused due to the merge commit

Ok no this workflow is different from main, and I don't know why. I'll look into it.

Oh... maty, are you using a GH action to determine the version number if version_task is not set?

[sterile abyss]

yes

[fleet dome]

Okay that is it then

[sterile abyss]

if it's set it runs the task

[fleet dome]

Yup that fixed it

[oblique tusk]

someone remind me: how do we merge the porting PRs? was it done manually (pushing the commits from the branch, fast-forward style)

[drifting finch]

Yeah

[sterile abyss]

yes

I'm actually free on tuesday so I can do the whole dance when it releases

[oblique tusk]

alright then

just checkin'

i wonder if GH will count the PR as merged if it sees all of its commits on the main branch, even without a merge commit

probably not

[sterile abyss]

iirc no

I know lex did that with a lot of PRs after the fork and github didn't do anything to the PRs

which makes sense

[oblique tusk]

alright then

[merry falcon]

these game tests are ran parallel right?

[lunar saffron]

Partially. They are batched, the batches run sequentially and the tests within a batch run in parallel

[merry falcon]

I see what you did maty for the plant tests. That works yeah.

[lunar saffron]

I didn't expect it to be so (relatively) simple but I found the reason for the warden shader crash (https://github.com/neoforged/NeoForge/issues/2046) and it's not a bug on our or vanilla's end.
The emissive layer of the warden uses a RenderType with RenderStateShard.NO_LIGHTMAP. This means that the lightmap texture won't be bound and therefore won't be added to the GlRenderPass#samplers map via GlRenderPass#bindSampler() in RenderType#draw(). The RenderPipeline.Snippet used for all RenderPipelines used by the entity render types however does specify Sampler2.

When a pipeline is compiled in GlDevice#compilePipeline(), it calls GlProgram#setupUniforms() which iterates over all samplers specified by the RenderPipeline, calls GL20.glGetUniformLocation() for each of them and stores those which return something other than -1 in GlProgram#samplers (this is where the X shader program does not use sampler Y which has been logged for the entity emissive shader for ages at this point comes from).

When the GlRenderPass prepared for one of the warden's emissive parts is set up via GlCommandEncoder#trySetup() and GlRenderPass.VALIDATION is true (which is the case since we set SharedConstants.IS_RUNNING_IN_IDE to true in dev), the command encoder goes over all samplers in GlProgram#samplers and throws an exception if the given render pass does not contain those samplers in GlRenderPass#samplers.

When the vanilla entity.vsh and entity.fsh shader files are compiled for the RenderPipelines.ENTITY_TRANSLUCENT_EMISSIVE pipeline, the GlProgram#samplers list does not contain Sampler2 despite the pipeline declaring it, presumably because the shader compiler optimizes it out (the vertex shader uses Sampler2 to assign a value to out vec4 lightMapColor but the fragment shader's use of that "field" is guarded behind a #ifndef EMISSIVE check and the aforementioned pipeline declares that define), and as such the shader program's samplers match the ones bound in RenderType#draw() by the relevant RenderType.
When the resource pack's entity shader files are compiled for the aforementioned pipeline, the use of Sampler2 does not get optimized out and therefore ends up in GlProgram#samplers which the validation in GlCommandEncoder#trySetup() catches, causing it to throw an exception.
This behavior can be replicated with the vanilla shader files by simply removing the #ifndef EMISSIVE check on line 31 of entity.fsh.

Running the resource pack linked in the issue in a NeoForge production environment where the validation is disabled makes the pack work as expected.

[drifting finch]

So in summary: Works as intended

Great

[warm night]

What an in depth bug investigation. Be sure to add your findings to the GH issue so it is not lost XD

[lunar saffron]

I put the full explanation in a spoiler in my closing comment on the issue

[oblique tusk]

perfect 👌

[marsh mural]

Changelog: https://www.minecraft.net/en-us/article/minecraft-1-21-5-release-candidate-2
SnowMan: https://github.com/neoforged/Snowman/commit/7fbc226aefbd95c973e48ccf68514f3e0119a25c
Primer: https://github.com/ChampionAsh5357/neoforged-github/blob/dd3dfc7bda520050a5e2c03c91c433a16f692547/primers/1.21.5/index.md
-# Changes: https://github.com/neoforged/.github/pull/14/commits/dd3dfc7bda520050a5e2c03c91c433a16f692547

Slicedlimes Videos:

• Main: https://www.youtube.com/watch?v=P0XnPy_D5kQ
• Data/Resource Pack Changes: N/A
  <@&1067092163520909374>

[spice geyser]

[tawdry trout]

where do I place bets for release date

[steep hamlet]

In the same place for the snapshots

[marsh crescent]

https://discord.com/channels/313125603924639766/1250829990195626115

[haughty spade]

ooh RC2

[limpid crypt]

https://www.youtube.com/watch?v=P0XnPy_D5kQ

[dark wasp]

are there any further critical problems?

[warm night]

For some reason that sounded soooo funny

Like something from "The office" or similar show

[stiff imp]

what do you mean bets?

they literally said it'll be tomorrow lol

[proven compass]

shhhhh

$100 on it releasing tomorrow

[gleaming pendant]

can someone take a look at Rogue's comments on the porting PR?

[fleet dome]

yeah I can

[true turret]

missed this one earlier, but i can mention it here, createRenderPass should have another OptionalInt added for a stencil clear value

[thin cairn]

Release time

[fleet dome]

I am actually not super keen on adding that stuff right now, since the only mod I know of that uses stencil will probably not be around until 1.21.6 😄

But ig I'll do it anyway

[gleaming pendant]

Could be deferred until we know what exactly we do with the stencil hooks tbh

[true turret]

i mean, punting it until later is fine, ill get legal's blessing and can do it myself later

[gleaming pendant]

Right now they're still global state, and we don't have time to change that before release

[true turret]

the stencil clears are fine, only thing under discussion was it being dynamic state or not

[gleaming pendant]

how does the testmod currently do it

[fleet dome]

global state

But that's not the point

[gleaming pendant]

ah encoder.clearStencilTexture(Minecraft.getInstance().getMainRenderTarget().getDepthTexture(), 0); right

[fleet dome]

dynamic state is not global state

Pipeline state is fine for it, but i requested self-review on the pipeline manipulators

then we have to move the stencil stuff into the pipeline

[gleaming pendant]

stencil is niche enough not to bloat createRenderPass I would say

[fleet dome]

I wouldn't add it there either atm

you can't actually reach createRenderPass

if you render via utility methods like renderItem

[gleaming pendant]

well true typically if you are stenciling a complex object you don't even control the render pass

[fleet dome]

Well depends, Immersive might? Maybe?

[true turret]

the clear and clear at renderpass start match the idiom B3D uses already for clearing, and map 1:1 to VK's clears

[gleaming pendant]

maybe but then an extra clear call shouldn't be the end of the world, right? 😄

[gleaming pendant]

1. VK is not really a concern and 2) stencil is really niche IMO

KISS

[fleet dome]

clear depth and stencil at the same time I can do since it touches the same texture memory

clear color and depth and stencil, I'd skip atm

It bloats the patch more than it already is

[gleaming pendant]

I meant adding a createRenderPass overload with an OptionalInt to clear stencil

[fleet dome]

Ah

ig it is feasible that is used

by special renderers

so fine

fine

[gleaming pendant]

well not in the port I would say 😛

add it later if a use case can be made for it

[true turret]

if we ever see a clearStencil immediately followed by renderpass start it can be added then

[fleet dome]

You could have said that about the entire stencil system 😄

[gleaming pendant]

it only got added back because Soaryn kept complaining about it

just to not even use it in the end, I think

[fleet dome]

Urgh very much possible. I'll just leave it as-is for now

[gleaming pendant]

well at least we have a test mod that allows us to keep something working reasonably with little testing effort

[true turret]

concern no, but it is where B3D is pulling its idioms from

[weak wasp]

The one change primer: https://github.com/neoforged/.github/pull/14/commits/dd3dfc7bda520050a5e2c03c91c433a16f692547

@marsh mural

Literally a regression

[drifting finch]

<@&1128776937410670663> With regards to todays launch, which of the following PRS will be merged into the porting branch before release and which after:

• #2047 Split Sources
• #2050 Pipeline modifier system
• #2053 Custom blockmodel definitions
• #2052 Datagen support for custom blockstate models
• #2040 Forbid access narrowing with patches

Please indicate your preference in a orderly fashion so that we can release post haste.

MyVote: All of them

They look stable and functional so I see no reason why not to

[gleaming pendant]

not so fast 😅

split sources is almost ready, I still need to address your comment which I forgot about yesterday (moving the attribute to the configuration)

2053 and 2052 need proper review

2040 needs rebasing I think

2050 might be ready

[drifting finch]

I am reviewing 2053 and 52 now

[gleaming pendant]

ok https://github.com/neoforged/NeoForge/pull/2047 is ready now

note that merging it will require changes to the other client PRs

shartte requested a self-review on 2050 so probably a bit nicer to wait

[drifting finch]

I don't like 2053's optional based design

But given that the record has no contract interface this is probably the easiest to maintain......

[gleaming pendant]

I'll have a deeper look later

[drifting finch]

Realistically speaking, we should have patched in an interface that represents the contract exposed by the record

So that it can be reimplemented it and then do a dispatch codec based on it

But that is a largisches patch surface that I don't personally like

[gleaming pendant]

well I think that instantiate is the entire contract

[drifting finch]

For sure

I am not arguing that it is not

I am arguing that replacing all relevant usages of the contract is possible

But that it is prohibitive in patch size

[gleaming pendant]

well I'm not convinced that it would be better anyway

[drifting finch]

Me neither

I just don't like the extra Optional

[gleaming pendant]

we use this pattern in quite a few places, e.g. Ingredient or previously for custom entity/item predicates

[drifting finch]

I know we do

I still don't like it

[gleaming pendant]

in any case I think the PR is good; I left minor comments

2052 needs a more careful review for which I don't have time now

[gleaming pendant]

@drifting finch let's merge this one?

[drifting finch]

Yep

Hit it

It is your PR anyway

So you should have the honor

[gleaming pendant]

thanks ^^

[drifting finch]

Can we do 2040?

[gleaming pendant]

yes

CI is going to fail anyway because of the non-port/ branch 😒

[drifting finch]

Yep

Already merged

[gleaming pendant]

I was thinking of the blog post

I don't really have time to write it today

[drifting finch]

I have time

So if you get me a TLDR of what you want in it

LIke some short bullet points

I will work it into a post

[gleaming pendant]

• NBT changes with some examples of how to port to them
• model changes, which I should elaborate on
• rendering pipeline changes
• other misc NeoForge changes:
  • Move mod init entrypoint earlier. Minecraft is not accessible from the mod ctor anymore.
  • NeoForge is split now. Can't be used optionally in mods yet, however ModelData was moved to a different package.
• link to primer

(+ anything else you might think of)

[compact hare]

Can't be used optionally = must be used period or MDK not ready?

Because it could be read as both

[gleaming pendant]

ugh sure; it won't be forced

the point is really that people shouldn't panic when they see that ModelData is red in their IDE

rather they remove .client in the import

[drifting finch]

The NBT changes are new to me (as in other then those in vanilla)did we introduce breaking NBT/DataComponent changes?

Yeah I will take care of that

[gleaming pendant]

I don't think we did, but I like to document the most modder-impacting changes first

otherwise they get lost in the rather long primer

[compact hare]

Sad moment

[drifting finch]

Alright

[drifting finch]

We are not forcing split package on modders. Period. We will offer support for it

And they can choose to use it natively when they want

needs to contribute a .devcontainer to websites......

[compact hare]

Okay sure, but can't I be sad about it? Lol

[drifting finch]

Sure, but not sure why you are......

I have not really seen a sidedness issues in any of the packs I play in years. So I am not sure what the fuss is about.

Yes it is easier for yourself and the IDE to maintain, and I might or might not adopt it, but that is entirely my personal opinion, plus you can make your own decision about it. Once it is available you can use it.....

[compact hare]

I am more a fan of compile-time guarantees. There can't be a sidedness issue if the compiler enforces client stuff to be relegated to client-only. And I am personally of the opinion that split should be the only thing that's available, due to the aforementioned guarantee. But I also know other people disagree so it doesn't really bother me that much TBH. I'll live with it

[drifting finch]

I for one agree with you on that, and all new projects will have it implemented

But some of my more complex projects, like C&B etc, will for sure not switch to it

The project is massive

And it would not be economical of my time to port that to this scheme, when I don't really have issues with it

[warm night]

Just use chatgpt... /s

[fleet dome]

Even if you forced split sources on modders

modders could just shove all of their code into the client source-set and essentially do the same crimes they do now

In the same way almost everyone just shoved their datagen into clientData 😄

I'd like https://github.com/neoforged/FancyModLoader/pull/260 too, but I'll make the Neo-Side PR first

Oh and Orion, thanks for making the handy list of PRs 😅

[drifting finch]

😛

[drifting finch]

On it

[sterile abyss]

we still have fluid rejects

[drifting finch]

Oef

[haughty spade]

I wish i could help, but mojang always releases when i'm busy >_<

[gleaming pendant]

🤷

The fluid system is cursed

[drifting finch]

I am out, I have no knowledge of how it works, beyond registering a simple none placeable fluid

[stiff imp]

Thank you

[oblique tusk]

is it time to call in Ash

[drifting finch]

I need somebody to take a look at https://github.com/neoforged/NeoForge/pull/2050 so I can merge it please

nvm

XFact is a maintainer

So this is mergeable

[lunar saffron]

Time to rebase the datagen one again 😄

[drifting finch]

Sorry

[lunar saffron]

Not your fault, that was gonna happen either way

[drifting finch]

@gleaming pendant Are you able to review the Early Loading Screen stuff?

Or can I merge that (initially only the FML part)

[drifting finch]

@fleet dome You around?

[drifting finch]

Announcement text: https://github.com/neoforged/websites/pull/61

@weak wasp Very nice work on the primer!

I still need to rewrite some sections as I just plainly borrowed from ash

But it is a start

[gleaming pendant]

I wanted to review 2052 😦

[drifting finch]

Sorry

I did not know when you would be back

[gleaming pendant]

I for sure wasn't gonna be before the release

(I don't have time)

[drifting finch]

It is okey

[gleaming pendant]

But there wasn't really a rush IMO

[drifting finch]

In the announcement post I am already adding a section that the model system will get tweaks and updates for the first few versions to address modder concerns and needs

As well as left room for you to write your own piece on the model rework in the next few days

[fleet dome]

At work. And you ignored my self-requested review on the pipeline manipulators grmbl

I get one YOLO merge 4free in FML now 😄

[gleaming pendant]

Yeah same for me on 2052. I requested a self review on purpose 😛

It's fine we can fix it up later. Just not a fan of doing the review + fixes after the merge 😄

[drifting finch]

The initial attempt is good enough, and was reviewed by me. and the writer as the second maintainer.

That is good enough to get started

I want to get this stuff in the hands of modders

[gleaming pendant]

Cause then I am the one who is under pressure to get the changes potentially undone

[drifting finch]

So they can tell us what to tweak and improve in the next few weeks

[gleaming pendant]

Well for 2052 we specifically weren't sure of the approach

[drifting finch]

I am aware of that

And I am adding sections to the announcement post, to that regard

I still think that getting this out there

And interating on it is better then sitting here

As it is one of the two approaches we were considering

If it is good enough, then it can stay

[gleaming pendant]

I don't like it. You are really forcing my hand to review it asap, and then I have to go out of my way to make changes by sending a PR myself

[drifting finch]

Are you planning on using this feature?

[gleaming pendant]

I will have custom models so definitely. And I was there for the design of the API+some discussion on how to datagen them a few days ago

2052 is a lot more complicated than it seems. Somehow we have to think about the VariantMutator. Right now it also doesn't allow generating multiple custom models in the same MultiVariant, which I find a bit weird

Hence the self-request for a review

We're also missing important docs on CustomBlockStateModelBuilder it seems 🤔

[lunar saffron]

To be fair, I should probably have split that PR into two and made the datagen part of it a draft

[drifting finch]

I understand your concern, I simply disagree that we need to get everything right on the first try. And no there is no pressure from me or anyone else on you reviewing this.

You can provide feedback like any other modder or maintainer when you start using the feature, either later today, or in the next few days or weeks.

We had this problem in the past that PRs where held up because a single person wanting to review, when other maintainers were okey with it going out. And we came up with the solution that unless you as the maintainer that wants to review it, actually active blocks it for a reason it can be merged and later altered. We chose that so we can get stuff out the door and in the hands of modders, to get feedback from a much wider audience. And that is the protocol I followed here.

[gleaming pendant]

I mean you didn't ask XFact and I think even he knew that it should get some review

[fleet dome]

Hey

I am not against YOLO merges

But I'll remind you of it next time I want to YOLO merge something 😄

[drifting finch]

Be my guest

Over the last few months I have changed my stance on a lot of things when it comes to developing software for a community

[fleet dome]

Because honestly at this stage there's not much of a problem "fixing it in post"

[drifting finch]

I am more then okey with fixing stuff in post

Especially in a breaking changes block

[gleaming pendant]

We'll live with this one, but yolo merging is not great in general. Now if there had been no review for 2 weeks it would have been a different story

[drifting finch]

Of a version that already has massive changes in this area

[sterile abyss]

I got the livingentity reject done now there's only one in flag logic in localplayer

[gleaming pendant]

I specifically asked for a self review to signal that the PR should not be merged yet, apologies if that wasn't clear

[drifting finch]

Sorry that was not clear 😦

If you need to block it: Self assign

Or actively reject the pr with an accompanying message

I am pretty sure we agreed that Self Assign was the way we indicated that a maintainer would be blocking the PR on review grounds

But I might be mistaken

[gleaming pendant]

Idk I thought self review was obvious enough. I don't know if I want to request changes yet

[oblique tusk]

at least, in my view, a self-assignment means the maintainer is going to shepherd the PR and thus will be handling its merge and other processes
(but not to the extent that they're the only one who can review it)

[lunar saffron]

Different question (which, for better or for worse, is related to that PR): for some reason, the latest PR publishing artifacts from the porting PR do not include the changes from the blockstate model datagen PR despite that PR clearly being part of the changes of the porting PR. How the hell did that happen?

[fleet dome]

yeah well I can see that, it's not super obvious

[oblique tusk]

you've checked the commit it built from has it, yes?

[fleet dome]

and ugh ffs i should try to fix the start.java shit before the final 1.21.5 hits but i'll mist that mark I think

i am bound up at work trying to build a health-check for some ancient message broker that had its last release in 2009

[lunar saffron]

I didn't, but looking at the edit history of the bot comment, it published the (at that time) last two commits in the wrong order, "overwriting" the newer publish with an older one

[limber ginkgo]

1.21.5 real™️

[marsh mural]

Changelog: https://www.minecraft.net/en-us/article/minecraft-java-edition-1-21-5
SnowMan: https://github.com/neoforged/Snowman/commit/82a9d7077b7d1426086b792c7193b6cbbb86cb81
Primer: https://github.com/neoforged/.github/blob/main/primers/1.21.5/index.md
-# Changes: https://github.com/neoforged/.github/pull/14

Slicedlimes Videos:

• Main: https://www.youtube.com/watch?v=LcYGZuA7ev4
• Data/Resource https://www.youtube.com/watch?v=V3nqaMukxf8

<@&1067092163520909374>

[oblique tusk]

ah. @sterile abyss any way we can prevent this from happening in the future?

IT BEGINS

oh lawd it here

[marsh mural]

oh i see snowman got fixed

guess il cancel my run

[fleet dome]

speak of the devil lol

[limber ginkgo]

the timing of this message is really funny

2 minutes later 1.21.5 releases

[warm night]

it is here

[steep hamlet]

that is one massive update

[oblique tusk]

alright gang, who's going to drive the release /j

[steep hamlet]

you!

[oblique tusk]

if we don't release sub-1 hour, are we even trying bro

[steep hamlet]

sub 20 minutes mate

[sterile abyss]

I can

I think I just fixed the remaining reject

[steep hamlet]

my 0 code modloader is already ported!

[sterile abyss]

I'm testing it while neoform builds

FAILURE: Build failed with an exception.

* What went wrong:
Circular dependency between the following tasks:
:neoforge:clientClasses
\--- :neoforge:compileClientJava
     \--- :neoforge:jar
          +--- :neoforge:clientClasses (*)
          \--- :neoforge:compileClientJava (*)

(*) - details omitted (listed previously)

[drifting finch]

eeeehhhh

[gleaming pendant]

Oh ffs hahaha

[oblique tusk]

[drifting finch]

That looks great XD

[sterile abyss]

that was running the IJ client

trying the gradle task now

seems like gradle might work, it's compiling neo

[gleaming pendant]

Build and run with idea works for me, maybe delegated builds are broken though

I see how that happened, it's unfortunate but quite logical

[sterile abyss]

yeah gradle works

[drifting finch]

I tested build and run with idea too.....

I hhave joined their darkside stuff

[gleaming pendant]

The jar task will have to be split into a common jar task and a combined jar ig

[fleet dome]

It's like painting racing stripes on your IDE!

[gleaming pendant]

Not too difficult but one has to track all usages of the jar task first

[thin cairn]

Hope I'll be able to port my mods to 1.21.5 Neo in a reasonable amount of time

Architectury is broken

[tired bluff]

Compile classpath should prefer the classes variant, not the jar, which I thought avoided this. Odd that it's not doing that.

[sterile abyss]

my patch seems to have worked

that leaves us with no rejects

[sterile abyss]

interesting, this only happens when IJ tries to run :neoforge:clientClasses

when I run it manually it works...

[wanton turtle]

hmm can i release my mod for neoforge 1.21.5 before neoforge 1.21.5 itself releases? yes!

[oblique tusk]

I wouldn't recommend it, at least

caveat emptor and all that

[sterile abyss]

@gleaming pendant is there a reason you went for clientApi depending on the project instead of on the main sourceset?

[thin cairn]

What build plugin are you using?

[wanton turtle]

MDG

[thin cairn]

Wait don't you do cross-platform?

[wanton turtle]

I do

[sterile abyss]

dependencies {
    clientCompileOnly(sourceSets.main.output)
}

sourceSets {
    client.compileClasspath += main.compileClasspath
}

this seems to please IJ too

[thin cairn]

What do you use for that then? Wait, are you the one who was making Stitch or whatever?

[oblique tusk]

I personally am wanting to port my one content mod to 1.21.5, which has the potion_duration_scale data component that I need

[wanton turtle]

modstitch yeah, wrapper around MDG and fabric loom so its unified DSL/API

[thin cairn]

Can you send me a link to it?

[wanton turtle]

https://github.com/modstitch no documentation yet nor would I yet recommend using it, its very early and breaks frequently - jared's multiloader template is good and uses both MDG and Loom without my thing on top

[sterile abyss]

although then removing the clientApi dep breaks the testframework as it can't see libs anymore. leaving that dep but replacing client.compileClasspath = main.compileClasspath + main.output also works

[thin cairn]

I could probably use Jared's for Bingo, but for World Host (which has 8 version targets), I'd rather have a unified API

[wanton turtle]

do you use stonecutter?

[thin cairn]

No, ReplayMod Preprocessor

Or rather my fork of a fork of a fork

[wanton turtle]

essential's fork?

[frigid sand]

Same Minute

[sterile abyss]

I'll leave the IJ delegated issue for later, decompiling 1.21.5 and running final test before pushing the update

[thin cairn]

No my fork of Deftu's

Deftu's adds some handy syntax, and then mine gets rid of the remapping

I use Mojmap in all my targets, so the remapping was just getting in the way

[sterile abyss]

[18:39:15] [Server thread/INFO] [minecraft/GameTestServer]: ========= 132 GAME TESTS COMPLETE IN 20.66 s ======================
[18:39:15] [Server thread/INFO] [minecraft/GameTestServer]: All 132 required tests passed :)
[18:39:15] [Server thread/INFO] [minecraft/GameTestServer]: ====================================================

very well

lifting rules

[merry falcon]

breaking changes in worldgen in 1.21.5:

• patch_pumpkin and patch_sugar_cane swapped order!!! FOC crashes until you fix this!!!
• several vanilla oak tree/tree selector features have _leaf_litter appended at the end now (e.g. trees_birch_and_oak_leaf_litter)

We need this added to primer. Especially the placedfeature order swapping as this will crash biome mods until they match the change

[oblique tusk]

"FOC"?

[lunar saffron]

Feature order cycle

[oblique tusk]

thanks

[real bolt]

Fully Ordained Curle

[sterile abyss]

I am running the now tradition test of do furnaces work

[oblique tusk]

make it into a game test

[sterile abyss]

there's nothing tradition about that

[weak wasp]

No primer update for the 1.21.5 release, so I've just marked it as ready to review

[marsh mural]

with that could who ever reviews and merges the primer pr ping me so that i can update the pinned message to not be using RC2 primer links

[sterile abyss]

#1313560104359956542 message

[weak lichen]

telepathicgrunt

[Reference to](#1313560104359956542 message) #1313560104359956542 [➤ ](#1313560104359956542 message)> breaking changes in worldgen in 1.21.5:

• patch_pumpkin and patch_sugar_cane swapped order!!! FOC crashes until you fix this!!!
• several vanilla oak tree/tree selector features have _leaf_litter appended at the end now (e.g. trees_birch_and_oak_leaf_litter)

matyrobbrt referenced · Mar 25, 2025, 4:44 PM

[sterile abyss]

[18:50:24] [Download-1/ERROR] [mojang/RealmsClient]: Failed to fetch Realms feature flags
com.mojang.realmsclient.exception.RealmsServiceException: Realms authentication error with message 'Invalid session id'

blerh someone remind me that I need to update the log4j rule to suppress that message now

[wanton turtle]

speaking of log4j can we get pretty colours in dev like fabric loom has

[sterile abyss]

we already have colours, run in debug

[marsh mural]

you get colors?

[wanton turtle]

oh i never do that that'll be why lol

[real bolt]

screms at IntelliJ “dumb” terminal”

[wanton turtle]

thanks

[sterile abyss]

furnace works

[merry falcon]

False

[sterile abyss]

very well then, release in 5

4

3

2

1

[merry falcon]

Don’t you see, that furnace is missing a pixel

[marsh mural]

oh no

updates people updates

[wanton turtle]

damn i just built it for the pr build

[lunar saffron]

If you don't get colors in debug either, add systemProperty 'terminal.ansi', 'true' (assuming MDG, NG might be slightly different) to the run configs in the build.gradle

[real bolt]

You can force the support with

[merry falcon]

Maty, did you test hitting enderdragon?

[sterile abyss]

lol @oblique tusk seems like GH was smart

[wanton turtle]

will it auto-announce how will i know when its on maven

[marsh mural]

i has color now yay

[lunar saffron]

Watch #builds

[oblique tusk]

YES

[lunar saffron]

@sterile abyss did you intentionally restore the porting branch? 😄

[weak wasp]

Done, also add the thank yous for telepathic and rogue on their comments and reviews

[sterile abyss]

yes

I don't like branches going missing

[lunar saffron]

Ok, just wanted to make sure

[oblique tusk]

oh lawd it building

[wanton turtle]

gotta love some codechicken diffpatching

these tools have amazing names

[oblique tusk]

mmmm, codechicken

[lost imp]

Mfw typo detected in the release post

A cold chicken can be seen in the forefront, with a creeping peaking out from behind a tree.

[real bolt]

TBF, we can probably think about ripping dumb terminal support out
iirc it was mainly due to Eclipse having issues with the ansi but that hasn’t been the case for a while

well at least rip it out in dev

[oblique tusk]

someone remember to close that TriState issue

since I believe it was eliminated

[merry falcon]

Did we replace it with MsoTriStateEnum

[oblique tusk]

over my dead body

[wanton turtle]

was this done?

[sterile abyss]

should have probably created 1.21.4 from 1.21.x before pushing 1.21.5

[oblique tusk]

smh maty

[merry falcon]

For those out of the loop

https://learn.microsoft.com/en-us/office/vba/api/Office.MsoTriState

[oblique tusk]

someone ought to make a doc outlining all the tasks to be done when porting

and releasing

[wanton turtle]

simple git command

[lunar saffron]

Handled

[sterile abyss]

it's fine, just had to cancel the release

[wanton turtle]

detatch head and checkout new branch right?

[sterile abyss]

tho now that I think about it

I think a 1.21.4 release would have tried to happen either way

rules restored, and the freeze rule has been lifed

we're back in normal business

[lunar saffron]

Yes, that was already done very early on

[sterile abyss]

heh, release within 57 minutes

could have done better smh

[wanton turtle]

iris released in 6 minutes, with neoforge build

[proven compass]

neo is out before fabric

[wanton turtle]

fabric is always out?

[proven compass]

yall went real fast

[proven compass]

for RCs

[wanton turtle]

the rc builds can just be renamed

[proven compass]

but they havent published an official 1.21.5 i don't think unless that was published this morning

[sterile abyss]

aktually we did manage to get a release before fabric at some point

courtesy of myself, as always

[gleaming pendant]

Uh that should be the idiomatic way of doing it I thought

[unkempt lynx]

the Neo builds aren’t out

not comfortable posting Neo builds based on the RC stuff with how many changes are out

[wanton turtle]

they are as of 2 minutes ago

[wanton turtle]

oh

i thought you said you did

[merry falcon]

Spreading disinformation

Jail for you

[wanton turtle]

https://modrinth.com/mod/yacl/version/3.6.6+1.21.5-neoforge

first then

[tired bluff]

Failed to parse MC version from NeoForge version 21.5.0-beta. Using capabilities of latest known Minecraft version with Minecraft version 1.21.5.

Should I be concerned?

[marsh mural]

i beg to differ

[oblique tusk]

it's time to PORT

[gleaming pendant]

No

[oblique tusk]

...after I go buy food

[gleaming pendant]

I guess MDG doesn't know about 1.21.5?

[marsh mural]

im using mdg

[oblique tusk]

did the gradle minecraft dependencies fire for 1.21.5

[marsh mural]

was able to setup mod dev just fine

[wanton turtle]

it says using capabilities of 1.21.5

[tired bluff]

https://github.com/neoforged/ModDevGradle/blob/bf4f81f5e4fa1e3d55d2533f1477ee1ff19f503f/src/generated/java/net/neoforged/moddevgradle/internal/generated/MinecraftVersionList.java#L12 What. Why????

[gleaming pendant]

Yeah it works i just don't know why there's a warning

[gleaming pendant]

Why not? 😛

Do you know why it exists?

[oblique tusk]

without access to a list of some sort, you cannot figure out the relative ordering of MC versions by release date

[sterile abyss]

someone just has to update the list

[oblique tusk]

it seems to be generated

so someone just needs to poke the MDG bear

[unkempt lynx]

1.21.6 is coming

probably

to fix Intel card having GL logspam

[oblique tusk]

until that comes, we shall henceforth proceed with the portings and bugfixings and all that joyous acts which derive

[wanton turtle]

@unkempt lynx can you publish neo builds so I can build controlify with compat in

wait compileOnly exists nevermind

[marsh mural]

primer links updated, points towards the merged or output now (not RC2)

[oblique tusk]

anyway, about 3 days hence or thereabouts when we're sure there's no 1.21.6 coming, this thread will be closed and locked

[sterile abyss]

making a few wording tweaks to the blog post before merging it, @drifting finch do you have anything else to add?

[oblique tusk]

did you mention our new team member /j

[wanton turtle]

my mod somehow puts a screen in before the neoforge crash screen lmao

Minecraft.getInstance() now appears to be null in mod constructors. is this intended?

[gleaming pendant]

Yes

That should be in the blog post I think

[wanton turtle]

is the blog post in a PR somewhere I can read?

[drifting finch]

Nope

[wanton turtle]

the blog doesn't actually say what event you should hook into to restore the old functionality in which i desire

[proven compass]

client start event should work fine, no?

[merry falcon]

Commonsetupevent?

You’re gonna need to go into more detail of what you’re trying to do lol