1.21.2 Snapshots

[final sky]

that's fine

but you are ignoring that of base as well

[red jungle]

yes

[final sky]

theoretically, this would just work

unfortunately, itemstacks and fluidstacks aren't hashable

[red jungle]

they will never be hashable

[final sky]

that's fine

but there needs to be some way to distinguish the target "type" here

and filtering for ItemStackContentsFactory instances is dumb

because there may be other implementors of DisplayContentsFactory<ItemStack>

[red jungle]

in this case I would probably make an ExcludeItemsSlotDisplay

(and one for fluids)

[final sky]

right but how do you even filter that

you still don't know the type of the factory

[red jungle]

instanceof

[final sky]

that won't help

vanilla's ForStacks checks are already dodgy

[red jungle]

in fact I was going to suggest that we check for instanceof DisplayContentsFactory.ForStacks<T>

[red jungle]

wdym?

[final sky]

ForStacks still doesn't constrain the output type properly

it's clear that it is MEANT for item stacks

but there are displays that could output, for instance, just the item holder

[red jungle]

not our problem IMO

[final sky]

we can't be certain that ForStacks outputs ItemStacks

[red jungle]

hmm

[final sky]

there is probably a reason why vanilla chose not to constrain types here

[red jungle]

wdym?

[final sky]

this interface

there is CURRENTLY only one implementor in vanilla

(and none for ForRemainders, oddly)

[red jungle]

ForRemainders was added for EMI etc

[final sky]

right

what i'm saying is this doesn't restrict the type sufficiently

for example if, ignoring the fact you're an AE2 dev

you used the information from slot displays to make ItemKeys for example

nothing in vanilla assumes that ForStacks PRODUCES item stacks

only that it CONSUMES them

we would have to assume that ForStacks produces ItemStacks for our purposes

(in other words a more appropriate generic signature for this interface would probably be DisplayContentsFactory<out T, in U> but i digress)

[red jungle]

oh right

we could make our own implementation of ForStacks that returns the stacks directly

[final sky]

ForItemStacks?

i mean, here's the thing

if mojang are receptive to changes to this system anyways, then we could see if this system can't be altered slightly

to just make our lives a little easier

[scarlet pawn]

Ugh I see I really need to read up on what the changes actually are. Right now I am blissfully ignorant.

[final sky]

especially since this design is probably gonna bite them in the ass eventually

ForStacks is maybe a bit of a misnomer, because it indicates that the interface consumes stacks

just like ForRemainders consumes remainders

[red jungle]

return factory instanceof DisplayContentsFactory.ForStacks<T> forstacks ? baseDisplay.resolve(context, new DisplayContentsFactory<ItemStack>() { public ItemStack forStack(stack) { return stack; } }).filter(s -> !excludedSet.contains(s.getItem())).map(forstacks::forStack) : Stream.of()

[final sky]

[red jungle]

looks reasonable I hope? 😄

it won't propagate the type of the factory to other displays, but I don't think we really have a choice

if we do this, should we force subtracted to be a simple ingredient?

[final sky]

yeah honestly

simple just means that it doesn’t need info that we can‘t send over a vanilla channel right

so a kubejs ModIngredient for example is simple

[red jungle]

@fathom compass some thoughts related to remainders:

• remainders aren't retrievable through TagSlotDisplay even though the recipe uses them
• in general, remainders are dependent on the stack that you pass, so it's weird to retrieve them without context information about stack that the recipe will use. And in practice, the remainder is just stack.getItem().getCraftingRemainder(); how about just returning that from the SlotDisplay by default?
• remainders are controlled by the recipe ATM; should they be moved to the ingredient instead? You might want to unify matches, assemble, and getRemainingItems in that case. 😄

[final sky]

also tech real quick — this can just use ItemStackContentsFactory.INSTANCE

[red jungle]

simple means that test(stack) is equivalent to items().anyMatch(item -> stack.is(item))

[red jungle]

true! nice

[final sky]

which is essentially the same thing as being sendable over a vanilla channel then

[red jungle]

yeah pretty much

it means that you can represent it as Set<Item>

[final sky]

then all of kubejs‘ ingredients are simple

which is great news for me

either way, i‘m fine with the tunneling through ItemStack that we‘re doing above

it does mean we will need a different slot display for fluid ingredients

or actually nevermind!

we can instance check for ForFluidStacks

and just do the same thing again

[red jungle]

I don't think it's a good idea to mix the two

if you're making a custom one, yet another is cheap 😄

[final sky]

well

the logic is the same

and SlotDisplay is meant to provide us with abstract logic

so i figured we might as well combine the two

either way, not a big deal

[fathom compass]

This schema is mostly aligned with our plans for recipes:

• Recipe is supposed to be a main (and probably only) "decision-maker" when it comes to crafting
• That includes remainders (so Item.getCraftingRemainder() will be gone)
• Displays are meant to represent static list of potential outputs (or some representative selection of it) since calculation of actual outputs will depend on many conditions in the future (loot table style, probably). It will be up to recipe creator to write them (outside of some defaults for trivial recipes)

[final sky]

the last bit is a nice bit of reassurance

i‘m on a train so my data is extremely spotty

and in typical DB fashion the wifi isn‘t showing me the captive auth flow

but right now i‘ve basically just done what you‘ve shown tech

we might want to add a utility method to resolve to an ItemStack stream using an extension interface

(which we can also use for utility methods like resolveForFluidStacks)

because vanilla only gives us resolving to a list currently

because i assume this pattern of tunneling through item stacks (and later fluid stacks) will become more and more common with the custom ingredient displays

[final sky]

oh ew intersection ingredients are gonna be a bit of a pain

[red jungle]

Ok that makes sense, thanks. That also explains why the displays have a Codec!

[red jungle]

I think we have to enforce that either 0 or 1 is non-simple

OR

we make the slot display part of the ingredient's JSON 😄

and honestly I'm kinda tempted to do that; provide a default that makes sense for simple ingredients, and let people override the slot display

[final sky]

wait what

why would intersection have to be non trivial

what about (TAG A) x (TAG B)

also intersection is n-ary

yeah uh no

with respect to intersection ingredients?

shit is fucked!

[scarlet pawn]

Does that mean we should eye a platform introspection API for recipes if the output becomes less predictable for recipe viewers or calculators like AE2?

[final sky]

well, nothing really changes right

recipes were unpredictable before this

they are unpredictable now

[scarlet pawn]

I suppose so

[final sky]

displays just expose the same information as before

it‘s the implementation details that kill us

specifically again the lack of IS hashing

[scarlet pawn]

AE2 is gonna have an interesting time

[final sky]

i mean the ItemKey idea honestly doesn‘t seem the like worst thing on the planet

[red jungle]

I said simple

Tag ingredients are simple because they don't depend on NBT

Can't we give the json writer a way to override the SlotDisplay, btw?

[blazing steppe]

it looks like you are the person behind recipe stuff... brewing recipes as json when? /s

[spiral tapir]

every time they're asked, they have to push it back by another year

[drowsy python]

next rc

[final sky]

brewing recipes as json needs component inputs

or a custom recipe type

that specifically filters for the potion component

and that is gonna mean bikeshed

[blazing steppe]

of course it needs its own recipe type just like smithing and furnaces

I have an implementation ready as a PR

[timber jolt]

👀 once the second point is implemented I can yeet my custom recipe

[red jungle]

depends if the remainder logic is data-driven or not 😉

[timber jolt]

what else would it be if the method is gone?

[spiral tapir]

hardcoded map

[timber jolt]

[final sky]

also depends on how extensive remainder logic is

like tools need to be returned with -1 durability for example

so it needs component transformation logic

unless that‘s what the loot table like system refers to

since i did think about implementing remainders using loot item functions in the past

[timber jolt]

in this case, I just need the water bucket to not empty itself but be consumed fully

[spiral tapir]

I imagine that would be default functionality in the future and then like what max said there would be some crafting function that replaces the ingredient with something else

[timber jolt]

well, if that's the case then I can yeet both custom recipes

[ancient frost]

That includes remainders (so Item.getCraftingRemainder() will be gone)
oof, rip jumbo furnace giving you your lava's bucket back

[scarlet pawn]

It's already in the recipe

[ancient frost]

no no, when you use the lava bucket as a fuel

[timber jolt]

well, if they don't have a way to do that, even normal furnaces will have a problem

[scarlet pawn]

Why are you using remainders at all for that?

[ancient frost]

normal furnaces just eat the bucket IIRC?

[scarlet pawn]

They could trivially hardcode the translation

[ancient frost]

ah no normal furnaces do give you the bucket back

[timber jolt]

that sounds wrong

[scarlet pawn]

But in NeoForgeland you can just use a Fluid handler to extract the 1000mb of lava and have it automatically give you the empty bucket

[ancient frost]

how WOULD that work in vanilla without crafting remainders

[scarlet pawn]

Since you're using it as a fluid container, not really a crafting ingredient

[scarlet pawn]

... hardcoded? 😛

[ancient frost]

hehe probably

[ancient frost]

no I'm using it as a fuel

[scarlet pawn]

Yes but the lava in the container is the fuel

[ancient frost]

let's say somebody makes... I dunno, a bowl of coal

if I eat the coal it should give the bowl back

[scarlet pawn]

Well you can make a recipe type for fuel 😛

Or maybe they will, who knows

[ancient frost]

data-driven fuels in vanilla 🤔

[final sky]

i'm sure some mojangster already looked at neoforge's datamaps

[final sky]

yeah but those are two simple ingredients

you said either 0 or 1 would have to be non-simple

unless you mean that only one ingredient in an intersection at most can be non-simple

[sand raven]

And then they implemented it with two lists instead where you have to match up entries to each list or else crash

[timber jolt]

ah yes, map at home

[sand raven]

Or the folder is the map and the files are the entry and you put just the burn time as the file content. The file name and path is the item

[timber jolt]

I mean, 50 is valid json content

[final sky]

okay but seriously

for intersection ingredients, what's our goal

[ancient frost]

the best json content

[final sky]

an intersection of multiple non-simple ingredients seems... stupid

[timber jolt]

yes

define zero as 3.14159265358979323

[spiral tapir]

nah, they'd for sure componentize that rather than use a datamap-like solution

[final sky]

[spiral tapir]

well, that's a container remainder and is already an item component

[timber jolt]

oh, you mean minecraft:burnable int component?

[ancient frost]

ahhhh maybe that's what was meant by that

[final sky]

i was gonna say for furnace fuels, was that not already a thing

i'm not talking about just furnace fuels though

[spiral tapir]

I'd imagine it'd be called "fuel", rather than burnable

[final sky]

there are non-item data maps

and that's part of their strength

it'll probably be a while before we see entity data components

[spiral tapir]

even then, datamaps do not sound like a mojang kind of thing. They'd more likely just create new datapack registries for each aspect that they want to define in data

[final sky]

"ParrotImitationData" registry

[timber jolt]

it's the name, it doesn't matter

[final sky]

and it's just a registry of records mapping entitytype to soundevent

i can see it

[spiral tapir]

I'm just being nitpicky, let me be :p

Entity components will be cool when they get around to that

[timber jolt]

BE components

or just stream codecs

that would be enough

[final sky]

can i uh

try and nudge this back onto the ingredient discussion

because that's what's kinda important to me rn

[wheat mango]

has hasCustomOutlineRendering been accidentally removed from BE's (in the port)?

or is it intentional

[lucid totem]

Will be replaced by some new hook that works with the frame graph system

[timber jolt]

frame graph?

[round perch]

That's intentional. The flag is now computed when the frame graph is built. For entities, vanilla does this by checking for outlines when they collect the visible entities during frame graph building. BEs however are still iterated during actual rendering with the two separate loops (per-section and global) and I didn't really like the idea of patching that to make it work like the entity approach just to keep that hook. My current idea is to add an event during frame graph building which allows setting that flag.

[final sky]

alright

ended up discussing something with tech real quick

and i think we may in fact just use the item-only displays he has right now by default

and then instead provide a way to override the display manually

(so for example through json)

[final sky]

partially inspired by what boq said here as well

It will be up to recipe creator to write them (outside of some defaults for trivial recipes)

this sort of calls back to an old idea i had with kubejs

where we wanted to have an ingredient type that essentially just wraps another ingredient

and changes the display item stacks

ideally, we would want the system to:

• use new SlotDisplay.Composite(items().map(Ingredient::displayForSingleItem).toList()); by default (this is alright since the VAST MAJORITY of ingredients are simple, and in fact the only thing that makes an ingredient non-simple is if one of its children is a component ingredient
• provide a way for the ingredient type to override this? (this is a maybe, theoretically only component ingredients should need this honestly)
• provide a way for the recipe creator to override this!!
  • either with a custom ingredient (that basically does what i said the kubejs one would have done above)
  • or a custom "neoforge:display" field that can be added to any ingredient

thankfully, slot displays already have a json codec (thank mr boq) so that part is made easier for us :p

[red jungle]

I'm tempted to add an OverrideDisplay custom ingredient that just wraps some ingredient to override the display in a data oriented manner

[final sky]

maybe

i‘m fine with either or really

once that all is a thing

i'll do the same thing with fluids

[red jungle]

@final sky https://github.com/neoforged/NeoForge/commit/bc39e14371734562f835f3b83ca23b7647a0dba4

[final sky]

nice

does the data component ingredient still have a custom display?

[red jungle]

yes

[final sky]

sweet

[final sky]

pasisng thought before i pass out (@silent ping me if needed)

this seems like a good start for a fluid display contents factory

however -- contents should usually be stacked, right?

so maybe it should be like ForStacks instead

but then we run into the problem of "how do we form 'default' fluid stacks"

also s/Fluid/Holder<Fluid>

there is potentially this extremely cursed hierarchy but even that has the issue of needing BUCKET_VOLUME in one place

this separation might actually make sense tho

to differentiate between contexts where we care about fluid amount and contexts where we don't

[red jungle]

Is that really needed?

I'd just copy vanilla and only have a single thing that accepts stacks

[final sky]

right but what stacks do fluid ingredients produce then

iirc we made a conscious choice never to guarantee anything about stack size there

[red jungle]

hmmm

SingleFluidIngredient uses FluidType.BUCKET_VOLUME

that's probably fine™️

Note: No guarantees are made as to the amount of the fluid, as FluidIngredients are generally not meant to match by amount and these stacks are mostly used for display.

right

we can add that disclaimer to ForFluidStacks

[final sky]

right then

what about the structure of the class

theoretically it‘s possible to combine Single and Tag into a holderset based FI

which would allow for other holdersets as well

i think i might do that in fact

[red jungle]

Yeah

[quick mantle]

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

[ancient frost]

MC-277552 - Wandering traders look smaller than in previous versions

[tidal siren]

<@&1067092163520909374>

[inland vault]

New version detected: 1.21.2-pre4.

[prisma pawn]

[tidal siren]

we're rapidly approaching a RC

[subtle leaf]

oh lord

[inland vault]

New version detected: 1.21.2-pre4.

[tidal siren]

👀 if that kills the Action I screm

Maty

We need to add a proper debounce/throttle on that

[inland vault]

1.21.2 Pre-Release 4

Hello! Another day, another Pre-Release. We're releasing the fourth Pre-Release of 1.21.2, further stabilizing this version.Happy mining!Fixed bugs in 1.21.2 Pre-Release 4MC-277496 Text on maps that are in item frames render through blocksMC-277502 Compass...

Oct 15, 2024, 12:58 PM

[sand raven]

We need more bouncing

Ping pong

[final sky]

hold on

did vanilla change its ingredient syntax in json?

is it now just #tag or namespaced:item

oh!

[red jungle]

yup

[ancient frost]

from what I understand ingredients are just holdersets in vanilla now

[red jungle]

that's standard holderset syntax

[final sky]

also no more mixed tag and item lists sadge

actually with neo's orholderset you can still mix that right

[ancient frost]

yes

[red jungle]

yes

[final sky]

nice

[red jungle]

btw this is why you can't use "type" for the ingredient type

[ancient frost]

the tricky bit now is respecting nbt in ingredients

[red jungle]

I would recommend neoforge:ingredient_type

[final sky]

yeah i'll do that

[red jungle]

or fluid_ingredient_type if you prefer

🤷

(I prefer neoforge:ingredient_type)

[ancient frost]

maybe something like SizedIngredient but for datacomponents and the recipe serializer has to opt into it

[red jungle]

it has to work with vanilla recipes

[ancient frost]

why

[final sky]

so hold on

[red jungle]

so it must be done at the ingredient level

[final sky]

there is no explicit map syntax for simple item ingredients

so there shouldn't be any for simple fluid ingredients either

[ancient frost]

why does datacomponents have to work with vanilla recipes? sizedingredients don't

[final sky]

oh man i really need to just

nuke this entire fluid ingredient system

or at least the codecs rather

oh man

should SimpleFluidIngredient even be an explicit type anymore

[red jungle]

make a HolderSetFluidIngredient?

as long as there are factory methods in FluidIngredient people don't have to know about it 😄

[final sky]

fluid ingredient types need a map codec though

should i just use a map codec that always errors

or wrap the existing codec in a field

like "values"

[red jungle]

yeah I'd make one that always errors

in both directions

to make the format as aligned with vanilla's as possible

[tidal siren]

https://github.com/neoforged/Snowman/commit/9ff0153f16d944f53ece437adb847502d7519dcd

[ancient frost]

do we even need ingredient types now

[red jungle]

yes we do, please stop

[ancient frost]

why do we need them

what use case do they have that can't be resolved with custom recipe serializers

[red jungle]

the point is that having to make a custom recipe serializer and type every time you want NBT-aware matching is not good

[ancient frost]

why not?

we already do that with sizedingredients

[red jungle]

it is too much effort for what is a very common use case

[ancient frost]

it's really not that much effort

[red jungle]

sizedingredients would not be suitable for vanilla crafting recipes so it would have been a bad idea to integrate them with ingredients

in this case however it's just a matter of redirecting the test method a little bit 😉

[terse anvil]

Let me just make a custom recipe serializer for every possible ingredient combination that exists

[red jungle]

better start now, it will take some time

[ancient frost]

case in point: mojang does it

[red jungle]

what we could consider is removing the customizability of ICustomIngredient in favor of having a HolderSet + a component predicate

idk if that's a good idea however

[terse anvil]

Except Mojang doesn't have to deal with people wanting to use a Mekanism fluid ingredient along with a CraftTweaker Conditioned Ingredient and a MyMod SeedTypeIngredient all in the same recipe

Maybe an Extreme Crafting recipe that uses a different recipe type altogether but is otherwise fully the same as crafting table recipes

[ancient frost]

I'm not saying toss out the concept of component predicates, I'm saying separate them out like we did with sizedingredient

so you'd have a SchmancyIngredient(HolderSet, count, Predicate<ItemStack>) or similar

[final sky]

HolderSets don't support equals() and hashCode() btw

which is a bit odd

but that is another thing custom ingredients currently have over vanilla ones

[red jungle]

oh that's annoying; Ingredients are supposed to be hashable and equals-comparable

[ancient frost]

why

err I mean

mojang-supposed-to or neoforge-supposed-to

[red jungle]

neoforge

[final sky]

for the StreamCodec

since FluidIngredients are inherently modded, should i just ONLY use a dispatch codec there

i don't see the harm in it, why not

[red jungle]

yeah I think dispatch only is fine

[drifting ice]

Only interesting change this update is one of the Font#drawInBatch overloads changing

[final sky]

@red jungle idk about you but this feels wrong

then again, fluid stacks are supposed to ignore amounts, so whatever

with the difference ingredient i was thinking about doing return base().fluids().stream().filter(e -> !subtracted().fluids().contains(e));

but for intersection that is a bit more involved obviously

also holder equality is a problem

[drifting ice]

Reminder that the block item methods within the Items deferred register should also set useBlockDescriptionPrefix

[red jungle]

what is this?

intersection?

[final sky]

yeah

[final sky]

fluids don't have a default instance

we explicitly never made one because we didn't want to declare 1000 as the default amount

[red jungle]

yeah

make it 1000 then yeah

[final sky]

should i actually add a getDefaultInstance

or just use FluidType.BUCKET_VOLUME specifically in these ingredients like i have already been doing

[red jungle]

I prefer that

[abstract shore]

what theme is that

[final sky]

catppuccin mocha

with some custom colour changes by me

[abstract shore]

looks cool!

[tacit arrow]

Imo use this until handler rework, then we can consider using a resource and not caring about the number

[final sky]

[spiral tapir]

I use the same theme, with color changes. And eclipse syntax highlighting

So good choice lol

[final sky]

https://github.com/neoforged/NeoForge/pull/1595

drafted for now, gotta work on some uni stuff

just wanted to get this out

feel free to change whatever you need

[red jungle]

looks pretty good

[final sky]

oh my god the github actions notifications are so annoying

[sand raven]

@ripe drum we need more github message email spam

[ripe drum]

smh smh

the emails max's talking about are the build actions failing

(yes those are annoying, also yes i can't do anything about it)

[sand raven]

not as bad as fabric pr email spam. Dont ask how I know

[final sky]

oh

i just realised i messed up something hang on

alright, that's fixed

still gotta do some docs and test cleanup

tho if anybody else wants to, the branch is open to fixes from maintainers

[scarlet pawn]

Fabric takes reviews seriously 😅

[wheat mango]

i know this is more of a request, but would it be possible to have lambda$addMainPass$2 (the main level rendering in 1.21.2) not have a different descriptor to vanilla?

it makes any sort of multiloader level rendering injections absolute pain

actually, one second... I don't know why it is different to begin with

[scarlet pawn]

There was a very lengthy discussion on this

Is that file patched by NF?

[wheat mango]

it's LevelRenderer so yes

[scarlet pawn]

We are recompiling the file

and the resulting bytecode gets "printed" onto the vanilla class in production via binpatches

code order is not guaranteed to be identical to mojangs sources when it comes out of vineflower

[wheat mango]

i know, I'm fine with the lambda names being different; i'm referring specifically to the descriptor usage

[scarlet pawn]

meaning the lambda naming will be different

Hm?

[wheat mango]

there's an extra boolean in the descriptor (it seems)

making any injections meant for Fabric/Vanilla fail

[scarlet pawn]

That is potentially caused by a patch to the method capturing an additional variable? (guess)

[wheat mango]

probably... i'm just wondering if it can be avoided

[scarlet pawn]

If it's due to patching? no

In that case there'd be a funtional difference

[wheat mango]

okay

looking at it again, the descriptor just has a different order

(Lnet/minecraft/client/renderer/FogParameters;Lnet/minecraft/client/DeltaTracker;Lnet/minecraft/client/Camera;Lnet/minecraft/util/profiling/ProfilerFiller;Lorg/joml/Matrix4f;Lorg/joml/Matrix4f;Lcom/mojang/blaze3d/resource/ResourceHandle;Lcom/mojang/blaze3d/resource/ResourceHandle;Lcom/mojang/blaze3d/resource/ResourceHandle;Lcom/mojang/blaze3d/resource/ResourceHandle;Lnet/minecraft/client/renderer/culling/Frustum;ZLcom/mojang/blaze3d/resource/ResourceHandle;Lorg/spongepowered/asm/mixin/injection/callback/CallbackInfo;)V


(Lnet/minecraft/client/renderer/FogParameters;Lnet/minecraft/client/DeltaTracker;Lnet/minecraft/client/Camera;Lnet/minecraft/util/profiling/ProfilerFiller;Lorg/joml/Matrix4f;Lorg/joml/Matrix4f;Lcom/mojang/blaze3d/resource/ResourceHandle;Lcom/mojang/blaze3d/resource/ResourceHandle;Lcom/mojang/blaze3d/resource/ResourceHandle;Lcom/mojang/blaze3d/resource/ResourceHandle;ZLnet/minecraft/client/renderer/culling/Frustum;Lcom/mojang/blaze3d/resource/ResourceHandle;Lorg/spongepowered/asm/mixin/injection/callback/CallbackInfo;)V```

the Z is moved to after frustum instead of before

fixing this would be really nice... it makes literally any multiloader project involving rendering pain on 1.21.2

[scarlet pawn]

Are those captured local variables?

[wheat mango]

seems so

captured from the main addMainPass function arguments

I'm pretty sure the argument in question is p_363964_ (?)

frustum is now used before it due to all the dispatchRenderStage calls

just a guess right now

[scarlet pawn]

I am not a big fan of patching a method to reorder the captured variables in a lambda since it's 1) never guaranteed to get the same lambda signatures as vanilla and 2) the effects and reasoning for doing this are completely hidden and will just fall on our feet in the next porting cycle

What are you injecting into that method for?

[wheat mango]

literally everything

it's Iris

it needs to constantly redirect and monitor the entire level rendering

alternatively, is it possible to not use arguments in a mixin injection (just have a CallbackInfo) then use @Local to dynamically get the things I need without relying on signature?

if so, this isn't nearly as much of an issue

[scarlet pawn]

Hm

[vivid ore]

Hey, just be thankful that this lambda has the same name in dev and prod, unlike some out there... but yes, that sort of mixin should in theory work?

Are there any injectors that force you to include args? I'm not sure

[wheat mango]

i mean I think redirects?

but those are rare overall

[scarlet pawn]

are you redirecting the entire lambda?

If so, just redirect the addMainPass call? 😅

[wheat mango]

nah, we're monitoring each part of the render individually

[scarlet pawn]

Hm? Do you need the args at all in those? I thought you could omit em

[wheat mango]

not all of them, yeah

that's what I'm doing right now

but some of them do need some

going to try the @Local idea

good news

    @Inject(method = { "method_62214", "lambda$addMainPass$2"}, require = 1, at = @At(value = "CONSTANT", args = "stringValue=translucent"))
    private void iris$beginTranslucents(CallbackInfo ci, @Local(argsOnly = true) DeltaTracker deltaTracker, @Local(ordinal = 0, argsOnly = true) Matrix4f modelMatrix, @Local(argsOnly = true) Camera camera) {
        pipeline.beginHand();
        HandRenderer.INSTANCE.renderSolid(modelMatrix, deltaTracker.getGameTimeDeltaPartialTick(true), camera, Minecraft.getInstance().gameRenderer, pipeline);
        Profiler.get().popPush("iris_pre_translucent");
        pipeline.beginTranslucents();
    }```

this does actually seem to work

now time to do this 50 more times and come back with results...

[scarlet pawn]

is local mixinextras?

Never used it

[wheat mango]

yep

[scarlet pawn]

Okay, good thing that's on both fabric and NF 😄

[lucid totem]

For the monitoring part, can you not just use RenderLevelStageEvent to track what stage it's at? The lambda issue sounds like an XY problem

[dapper pendant]

Why do many method_?

On Look you can enable an option to use the $lambda$ normally

[wheat mango]

that doesn't work in production though, does it

[dapper pendant]

https://github.com/Gaming32/bingo/blob/main/build.gradle.kts#L28

[wheat mango]

i'm not going to rewrite this entire thing to use Neo only events when the multiplatform one works perfectly

[dapper pendant]

Well if you're remapping mixins it will

Please tell me you're remapping mixins

[wheat mango]

of course

it just does not work randomly for method_

i have tried in the past

[dapper pendant]

Enable nameSyntheticParameters. Without it, Loom strips all mappings for synthetics (such as lambdas)

Which could also be said remapping issue

[wise mica]

i just read up on fabrics blog post, what’s with blocks and items and entity types now being passed their id?

[final sky]

evidently it… doesn’t though

we also had to write a mixin with three targets in kubejs back when we were multiplatform

due to signature shuffling

from what i remembered as soon as we went neo only that just became an event listener

[wheat mango]

well it does now with the local trick

the injections themselves are fine

[lucid totem]

Well, similarly, we're not going to rewrite a lambda signature to address a use case that would work perfectly fine with an event 😉

[final sky]

i mean that's fair

if there is a supported way the supported way should be used

everything else is just kinda... hacky

oh dear that reminds me i should probably replace some kubejs mixins with event calls

[sand raven]

FIX THE EXPORT

[vivid ore]

Speaking of lambdas... we do still need to address the whole "lambda names differ in prod and dev" thing

[lunar dagger]

why do lambda names differ in prod and dev, first

[sand raven]

https://github.com/TelepathicGrunt/RepurposedStructures/blob/a1fe2af421f301b4769565f494c1ab823f9d202a/neoforge/src/main/java/com/telepathicgrunt/repurposedstructures/mixins/neoforge/structures/StructurePoolMixin.java#L18

names looks like this

[dapper pendant]

The method_ thing is because you're using Loom and didn't disable Loom's "feature" of stripping lambdas from mappings

[round perch]

Slightly painful (mostly self-induced pain) but it runs. Some noteworthy stuff I either I already knew or found during some quick experiments:

• RenderHighlightEvent.Block fires twice, once during non-translucent and once during translucent rendering. This is a vanilla change with the intention to fix outlines in non-Fabulous modes (not that it actually achieves this: if you place a non-translucent block [i.e. glass] where you can see a translucent block [water, stained glass] behind the outline, the translucent block will still vanish behind the outlines)
• Block outline rendering currently uses ItemBlockRenderTypes instead of the model-based render type lookup to decide in which of the two phases the outlines should be rendered
• RenderHighlightEvent.Entity is defunct as entities can now definitely not reach that codepath anymore (not that I'm aware of anyone using that anyway)
• Standalone models registered via ModelEvent.RegisterAdditional are currently not loaded (I have a fix for that which I'll push tomorrow)
• Render types specified in the JSON are ignored for most models (I have a fix for that which I'll push tomorrow)

[lucid totem]

How did you handle getLightBlock losing positional context?

[round perch]

Patched 😄

[lucid totem]

What do you mean by patched

[round perch]

Patched the context back in during the Kits phase

[lucid totem]

Hm

[round perch]

Oh, wait, I'm confusing two methods

getLightBlock() was left as-is, the light emission methods were patched to regain the context

getLightBlock() could already not use any level context in 1.21.1 and earlier unless the block was marked as "dynamic shape" so it wasn't a loss that affected me

[lucid totem]

ah

[round perch]

Also, one thing I ran into in the very beginning: Gradle was complaining about not finding jtracy and I had to add libraries.minecraft.net to my repos to fix that

[vivid ore]

Recomp

Basically, in prod, classes that neo does not patch the source of, neo does not distribute binary patches to

So they're just the (remapped to mojmaps) vanilla class

In dev, however, those classes are instead the original class, decompiled, neoformed, and recompiled

So in specifically in classes that neo does not patch, you get methods that look like lambda$someContainingMethod$3 in dev but lambda$someContainingMethod$16 in prod, or the like

In classes neo does patch the lambda names are of course shuffled relative to vanilla, but they're (generally, reliant on some impl details of javac but eh) the same between dev and prod

But if neo doesn't distribute a binpatch for that class... all bets are off, including on stuff like the descriptor

(And I have found classes where due to generics stuff being kinda lossy in the decomp/recomp cycle, lambda descriptors also varied between prod and dev -- I'll try to dredge one up, I don't have a good example off the top of my head)

The two solutions -- and this is kinda a rather big issue so I feel like a solution is warranted -- are:

1. distribute binpatches for every MC class, even unpatched ones. This'll incease the size of the binpatches a bit -- not sure how much, or if it's an appreciable amount. The resulting situation is still... not too great, as you're reliant on impl details of the compiler; worse, if, e.g., ecj were used, it wouldn't work. MDG allows the use of ECJ for recomp (unless this has changed?), so someone with such an option on can already get mysterious errors when depending on someone else with a mod with mixins to lambdas anywhere.
2. avoid recomp. This has... all the issues that led to doing recomp in moddev in the first place, of course. But it would, fundamentally, guarantee that you had the same names in dev and prod, unlike option 1. The issues with using recomp in dev are all theoretically addressable -- even the line number stuff, I have a system I've been working with locally that makes the line numbers line up right -- but, well, the fixes add a lot of complexity.
   Option 1 fixes the issue and shoves the cost off onto users, but we don't know how big that cost is. Option 2 fixes the issue, but would require extensive testing to not introduce other issues, and adds a lot of complexity. I've messed around with some other approaches -- such as transforming the remapped jar to get the lambda names to be correct again, potentially by storing some marker next to them in the source that could be erased in the source that finally gets linked, or whatever -- but that approach turned out to not be super viable due to just how much the lambda names can differ -- stuff like descriptor and the like that's more painful to fix -- and it introduced a fair amount of complexity itself

But in any case, this issue kinda means that no library mod can mixin to lambdas, without risking not working on other people's machines when folks depend on it, and mixing in to a lambda in a non-neo-patched class at all requires looking through the remapped-but-not-recomped bytecode -- which is a pain to locate, if you don't know where to look -- to determine the prod lambda name, to begin with

So it's kinda a bit of a big issue

[spiral tapir]

to answer this, it's specifically used for loot tables and such. they want to be more explicit with stuff like that rather than just always assuming it to be the same as the id with a certain prefix or whatever

[red jungle]

You have to update MDG to fix that

I hope the fix made it to the 1.x branch

[red jungle]

Kind of. But we'll be in a better position to experiment with this once #1279375701115535410 is through

[red jungle]

please have a look: https://github.com/neoforged/NeoForge/pull/1597

[final sky]

hmmm

the ingredient unit tests need fixing

[inland vault]

New version detected: 1.21.2-pre5.

[cosmic acorn]

at this point I think they want to release next tuesday

rc1 on thursday maybe

[short iris]

[Jump to referenced message](#minecraft-updates message) in #minecraft-updates

gigaherz referenced · Oct 16, 2024, 1:59 PM

Minecraft 1.21.2-pre5

This is a new version. Changes listed from 1.21.2-pre4.

Links:
client | client_mappings | server | server_mappings

Quoted | Last updated · Oct 16, 2024, 1:39 PM

[inland vault]

New version detected: 1.21.2-pre5.

[cosmic acorn]

https://www.minecraft.net/en-us/article/minecraft-1-21-2-pre-release-5

err this is the link

[tidal siren]

<@&1067092163520909374>

[cosmic acorn]

tiny

[jaunty zodiac]

we're almost there (ig)

[drowsy python]

it looks tiny

but snowman is the one to decide that

[cosmic acorn]

tiny changelog

not necessarily tiny diff

:P

[drowsy python]

snowman : [phone]

[tidal siren]

I N8N triggered twice again

[dapper pendant]

Heh second one was in Slicedlime's video

[tidal siren]

6/191

[inland vault]

Minecraft 1.21.2 Pre-Release 5

Did we have a Pre-Release yesterday? We don't remember. But here is one anyway, fixing more bugs.Happy mining!Fixed bugs in 1.21.2 Pre-Release 5MC-277630 Recipe book is not updated when one of the available recipes is usedMC-277634 Arrows shot at walls whe...

Oct 16, 2024, 1:58 PM

[lunar dagger]

pain of n8n

well, partly with snowblower as well

perhaps I should poke that soon...

i mean, nevermind the fact that I don't know what Guardian is

[tidal siren]

Pain of 54/191 and rising

[dapper pendant]

Interesting. Ocelot says +92 -71

So close yet slightly different

Very interesting

[tidal siren]

Likely skips structure files?

[dapper pendant]

This probably has something to do with it

[dapper pendant]

Ocelot turns them into SNBT for proper diffing

The gradle.properties is used by the setup, since it's based on GH Actions and, well, Gradle

Basically the action checks for any releases since the release in the gradle.properties

[cosmic acorn]

I was writing an NBT comparar tool at once point, I should finish it.
wouldn't be useful for this, it was intended for use with tortoisegit diffing

the viewer part was done, but I wanted to have a means to transfer data from left to right

well "done"

I apparently never got around to synchronizing fold/unfold

or scroll

[tidal siren]

https://github.com/neoforged/Snowman/commit/487a8f98b2971833d108f74f76e4357e5b59fb78

+117 -98 :3

[round perch]

Reading another discussion about recipes just reminded me: did we check whether the TagEmptyCondition still works after the tag loading changes?

[red jungle]

Good question. Isn't there a test for it?

[round perch]

No clue 😅

[final sky]

hey tech can you forgive me for not properly documenting fluid ingredients and generally finishing up my work on them for a bit ^^;

gotta deal with hacking together dockerfiles for the ctf challenge we're hosting at uni

as well as just my thesis

so the rest of this week is gonna be fucked

[vivid ore]

Oh, I definitely look forwards to the neodev gradle setup being easier to mess with! You're absolutely right that this shoud be easier to address afterwards -- I just want to make sure that we don't lose sight of it given the scope and impact of the issue

[red jungle]

ugh 😅

is there really that much work left?

[red jungle]

yes ofc; it's just hard to evaluate possible solutions right now

[final sky]

thankfully it's mostly docco work

and working on sized ingredient displays

...and tests

[vivid ore]

Well, it's not like there's a huge variety of possible solutions. We only have, like, 2 -- what I'd thought in the past might be a third turns out to not work or be impractical for other reasons. But sure, point taken

[round perch]

Pushed a fix for the last two points and the missing updates to generated files

[narrow lava]

Wait how do you get access to snowman?

[round perch]

By being a team member

[narrow lava]

Ah ok

I'm assuming it's snowblower?

[round perch]

Yes, snowblower is the tool used to generate snowman

[narrow lava]

Ok

[round perch]

@lucid totem I was curious what the implications of the blockstate changes are for a mod like FramedBlocks with very large amounts of blockstates and the difference is actually quite substantial (first is 1.21.1, second is 1.21.2). The cache is also significantly smaller now.

[lucid totem]

the shallow size of BlockState increased but maybe they deduplicate more aggressively to make up for it

I saw some basic deduplication for common shapes

[round perch]

Yeah, empty and full occlusion shapes use a shared "face shapes" array

[lucid totem]

but yeah, 2.5x reduction is very nice

[red jungle]

JCC also fails on PRs against the port branches...

[tacit arrow]

maty

Obviously

[dreamy obsidian]

Does the blockstate stuff include "fixing" the neighbor state data structures to not waste insane amounts of memory?

[round perch]

That's the primary change, yes. Instead of a Table, the neighbors are now stored in a Rerefence2ObjectArrayMap<Property<?>, S[]> (the field is a plain Map in this case). The index into the array is provided by the respective Property

[dreamy obsidian]

But it's still one "large-ish" map object per blockstate?

I.e. still O(n^2)?

[round perch]

Yup

Every state has its own map

[cosmic acorn]

pre6 today, or rc1

[short iris]

Heads

[tidal siren]

RC1

[short iris]

Tails

[clear urchin]

pre6

[short iris]

pre6

[timber jolt]

but I'm for rc1 followed by pre6

[short iris]

pre6

pre6

[hoary cipher]

[cosmic acorn]

the bot is convinced it is pre6.

[blazing steppe]

do we have for today?

[cosmic acorn]

no confirmation, I just expect there to be a rc1 today with release planned for tuesday

[blazing steppe]

if release tuesday then rc1 tomorrow is more likely

[wise mica]

rc1 people win

I can't ping snapshot alarm 😕

[inland vault]

New version detected: 1.21.2-rc1.

[wise mica]

Took ya long enough

[inland vault]

New version detected: 1.21.2-rc1.

[cosmic acorn]

I win!

<@&1067092163520909374>

[haughty raven]

RC time

[tidal siren]

I

[cosmic acorn]

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

[wise mica]

inb4 they completely reworked networking to fix the packet bug 😛

2,000 changed files

[prisma pawn]

whee release candidate!

[inland vault]

Minecraft 1.21.2 Release Candidate 1

We missed you so much since the previous pre-release so today we're releasing Release Candidate 1. If no critical issues surface, this is the version we are planning to ship as Minecraft: Java Edition 1.21.2, the Bundles of Bravery drop.Fixed bugs in 1.21....

Oct 17, 2024, 1:26 PM

[prisma pawn]

so was this notification from a parallel universe where they released fifteen seconds later...?

[tidal siren]

It's the N8N automation being a bit fucky so it triggers multiple times

[prisma pawn]

wondrous

why have people post the link by hand when we can have a bot that does the same thing but janky and slower? :p

[tidal siren]

Oh for this?

That's because the blog release tends to be behind the actual release
Those initial messages come from checking the version manifest (which then kicks starts some automated processes related to handling new MC versions)

[prisma pawn]

ah

hopefully it doesn't start the processes twice too

[tidal siren]

No, it kills ones part way through which then means it fucks up the cache

[prisma pawn]

the snapshot bets site just polls the launcher manifest once a minute

[haughty raven]

yep, excluding all the version number changes, like ~10 changed lines

[bronze crescent]

so many alarms this week

[tidal siren]

https://github.com/neoforged/Snowman/commit/8f851a8e12091398233916fc2cff3a7de9c3defd

9 files changed, +28 -24 lines changed :P

[haughty raven]

slicedlimes update video should be short and sweet today xD

[narrow lava]

great now i have to rerun snowblower lmao

its still pushing the first run lmfao

[dapper pendant]

The fix for the first one is this

I wonder why it wasn't using a loot table before 🤔

[vivid ore]

Oh FFS rc1 already -- I am not ready...

[tidal siren]

if you want them to release faster

[haughty raven]

-# surely nothing can go wrong with faster releases, right...

[narrow lava]

wait what
was that like some kind of joke from mojang? theres no way that was an accident.

also nice job

oh lmao its linked to the version release date ok thats kinda cool

[tidal siren]

Which are mostly correct

There's a few that are fudged slightly on Mojang's side in the Version Manifest to effect sorting

[narrow lava]

lol

[dapper pendant]

Roughly how soon do you think NeoForge will be updated to rc1?

[cosmic acorn]

https://toot.community/@[email protected]/113323266187679295

[haughty raven]

40 seconds lol gotta be one of if not the shortest update videos

[tidal siren]

Whenever the necessary people get time, since it's so small it should be fairly fast once they do
We won't do a proper release until after Mojang drop 1.21.2, however you can see/use via the PR builds available from https://github.com/neoforged/NeoForge/pull/1590

[dapper pendant]

So the passenger issue was this: if a client was told to make an entity ride an entity it believed was unserializable (like a transient creaking), it would reject it

So that check is now server-side only

[dapper pendant]

Yeah I'm not asking about full release. I'm wondering when I can start my update process

[sand raven]

Fabric discord brought up a good point

We need some way to update our tags with experimental stuff only when the experimental datapack is on

Do we have a way to inject into experimental pack?

[tidal siren]

wouldn't you just use optional tag values?

[vivid ore]

No the items are still there when the experimental datapack is off

You just don't want to include them in the tags then

[tidal siren]

right

[vivid ore]

Erm, this should be doable by adding a feature-flag sensitive datapack?

I think you could make a built-in datapack that would work with that with the current API; I'd have to test though

[haughty raven]

Neo could provide it own experimental pack which listens for winter drop flag (or any flag) and adds stuff to necessary tags, issue is the experiments screen enables via pack not via flag, so users would need to toggle on the vanilla winter drop pack + neos winter drop pack + any modders winter drop pack

[vivid ore]

Oh gah, you're right, it does work that way

Hrm. Can we make conditional stuff work with feature flags?

So you could then have conditional data based on which feature flags are enabled using normal neo data conditions

[red jungle]

it should be possible to add a feature_flag condition but it won't work nicely with tag entries since those aren't conditional atm

[haughty raven]

That should be doable flags are loaded before conditions iirc so would be a simple condition which looks up enabled flags and validates them against a JSON list of flag ids

[vivid ore]

Luckily, anything can be conditional vai overlays

[red jungle]

yes that can also work

[ancient frost]

accidentally opened the chest before they saved the structure nbt

[dapper pendant]

Wouldn't be too surprised actually

Seems like an easy mistake to make

[red jungle]

let's do a quick update to rc1

a few rejects; I'll have a look

probably just context differences

[round perch]

I expect one of the rejects to be the gui far plane stuff in GameRenderer#render() as the Z value changed again from 10k back to 11k

[red jungle]

Yes

[solar pagoda]

I think it makes more sense depend on the experimental datapack being enabled, not the feature flag. Because other things (like biomes) are only added by the datapack, so in theory the feature flag could be enabled without the biome existing.

That could be solved by optional entries then, but that doesn't seem like the cleanest solution.

[round perch]

Btw, as far as I can tell, we can remove the delta time patches for screen and overlay rendering in GameRenderer#render(). The associated Mojira ticket is marked as fixed and I can't find any visual differences between vanilla and our patch

[round perch]

We also need to fix the test world / mods button overlap. Conditionally shrinking the mods button (potentially even to a third instead of half the window size) and putting it next to the test world button seems like the best option to me since putting them above one another causes the options and quit buttons to go partially outside the window

[red jungle]

Yeah that seems good to me

[red jungle]

what about this?

new vanilla code:

[round perch]

I'd replace the "else" branch of the ternary with (this.level().getBlockEntity(this.blockPos) instanceof BeehiveBlockEntity hive ? hive : null)

[red jungle]

ok

[red jungle]

so our previous patch (pre1) was wrong

10k - far plane is -11k

[round perch]

Yeah, the far plane computation wasn't adapted to take the -1k into account

[red jungle]

ok yeah

well now it doesn't have to be anymore 😄

[round perch]

Yup

[red jungle]

https://gist.github.com/Technici4n/a21d6e52ad29a9fb3dea10b7ee2e0e2e

I already went through this diff and fixed a bunch of stuff (some lambda parameters changed 😭)

I need to fix this one still

[round perch]

The diff looks fine to me, this ^ is the only thing that stood out to me as well

[red jungle]

[round perch]

I think they removed the parameter

[red jungle]

yeah

[red jungle]

pushed to main repo

[round perch]

👌

[red jungle]

oh god the PRs targeting port/1.21.2 are kinda screwed 😅

I saw your PRs but unless you say you need careful reviews just merge them when ready

[round perch]

Oof, yeah, those are kinda toast 😅 Nothing a force push can't fix though 😄

The main one I'd like reviews on is the frame graph setup event one, the rest is trivial

[red jungle]

maybe a job for shartte

[round perch]

Heh

[round perch]

Finally done with a complete sweep over all the patch changes in the porting PR. A couple things that still need looking into:

• The PanoramaRenderer uses render types now, need to check whether the explicit disabling of the depth test is still necessary
• The EquipmentLayerRenderer needs to have #1541 re-implemented if possible
• The AbstractVillager has an odd patch (not a new one from this update) that makes no sense to me: https://github.com/neoforged/NeoForge/blob/f08da3eb01dc79fde0fc10999cfb1ffe9b0068e9/patches/net/minecraft/world/entity/npc/AbstractVillager.java.patch#L15-L21
• The ReloadableServerRegistries/SimpleJsonResourceReloadListener/LootDataType trio needs a good look with respect to conditional codecs as both the second and the third apply conditions at the moment

[red jungle]

Loot shouldn't use the json reloader as of a few releases ago

[red jungle]

ah no, it goes through SimpleJsonResourceReloadListener.scanDirectory now

you're right, it looks like the condition handling might be duplicated

[drifting ice]

Can someone make the default branch of kits the one currently being worked on for release? It makes it easier to search things for the documentation

[lunar dagger]

done, Kits branch changed to 1.21.2-rc1

FYI to all, pls ping me or an org admin whenever the Kits branch needs to be changed

[round perch]

I'd like some opinions on https://github.com/neoforged/NeoForge/pull/1598: I'm not familiar enough with the framegraph approach to be sure whether these events are actually useful for adding custom passes or whether it would be better to just have the current pre event (primarily for the entity outline post-processing) and then having people mixin between vanilla passes to add their custom ones (I'm not sure adding an event after every vanilla pass for that is a good idea)

[drifting ice]

Okay three more sections to go of the docs. Also the three sections that changed the most but, eh

Will probably hold off on recipes a bit though until it's been updated here

[round perch]

That's going to be a fun PR to review when you're done 😅

[drifting ice]

Yep. Writing the primer does help sometime

Not so much for figuring out what neoforge did :p

[round perch]

Heh 😄

[drifting ice]

As for the framegraph stuff, I think probably the best use would be to have a mixin that makes it easier to obtain the FrameGraphBuilder for other mixins

Since it really depends on the modder to see where they want to add their targets

The pre event is mostly useful for registering the internal and external resources along with the post chain frames, but that could also be done at any time as that purely depends on when it needs to be set

[round perch]

The graph builder is trivial to obtain in a mixin, it's unique in that entire method and can just be captured with MixinExtra's @Local

[drifting ice]

Ah, I thought you had to either capture all locals or none at all

[round perch]

In vanilla mixin you have to capture all locals until and including the one you need. With ME you can just target exactly what you need

[drifting ice]

Then, I don't see much point in the event in general unless we wanted to help modders enforce some sort of pass order

Only thing I could think of is to register the resource handles they need once, but even then, most people who would be mixining here should know how to do that

[round perch]

To be fair, the primary reason why I want an event there is the entity outline post-processing stuff since enabling it has to be done in a short window around where that PR adds the pre event

[lucid totem]

Is there even a benefit to using the passes outside Fabulous?

[drifting ice]

Technically, that's what the post effects also uses, so yes

[drifting ice]

I feel like I might be misunderstanding what you mean by outline the post-processing stuff here. Like are you talking about forcing the frame pass even when there are no entities to show the outlines or vice versa?

[round perch]

As far as I understand it, the frame graph does some optimizations to resource acquisition and allocation (from a bit of research that at least that seems to be the primary idea behind it), so more specific passes should be more efficient with respect to resources.

[round perch]

Forcing the outline post-processing effect even when there are no entities using it to allow using it in BERs and any other non-entity renderers that run during the main pass

[drifting ice]

Ah gotcha

But in that case, it would make more sense to modify the LevelRenderer#collectVisibleEntities method in some way to do that

[lucid totem]

Resources are just framebuffers though, right?

[round perch]

Primarily, yes, but some of them are so-called "internal" resources which are only relevant in a certain window spanning a given set of passes. The blog post I read about framegraphs explained that it for example allows re-using the same memory for two internal resources as long as the "usage window" of these two resources doesn't overlap. I'm not sure whether Minecraft actually makes use of such optimizations though

[red jungle]

I don't find what the new version is

does anything in that cache depend on tags?

[lucid totem]

There is none, the cache is now initialized exactly once

[lucid totem]

Nothing has since 1.16

[red jungle]

oh

so, do we still need to init the cache on bake?

[drifting ice]

I think this only makes use of it for the post chains when dealing with multiple passes. I don't see how its handled in other places unless in fabulous for the render targets

[lucid totem]

we need to init it for newly registered blocks at some point

but we do not need to reinitialize it on each sync

[red jungle]

ah I see

[lucid totem]

not sure what hook is appropriate for that

[red jungle]

that's already the case atm

[lucid totem]

cool

so nothing changes for us then

[red jungle]

the getLootTable call seems very useless

[lucid totem]

I think it's mimicking Mojang doing that themselves at some point?

Let me pull up 1.16

Even in 1.21.1 they resolved the loot table eagerly

[red jungle]

weird

[lucid totem]

They stopped doing this in 1.21.2

[red jungle]

indeed

[round perch]

Yeah, the loot table ID is now computed in the BlockBehaviour constructor

[red jungle]

didn't we have a supplier for the loottable at some point?

[round perch]

We did

[sand raven]

That means we need to do Jira and fill out tickets that we link to PRs!

[red jungle]

OK I'll remove the getLootTable call tomorrow

[toxic river]

Sky rendering seems broken with NeoForge (port/1.21.2) (fyi)

[ancient frost]

pretty sure the neoforge 1.21.2 port doesn't exist yet

[round perch]

It's right here: https://github.com/neoforged/NeoForge/pull/1590

[round perch]

Screenshots or it didn't happen

[toxic river]

the transition is not smooth

[round perch]

Comparison?

[toxic river]

yeap, 2s

NeoForge

Vanilla

[drifting ice]

Ah, the no interpolation between timesteps

Definitely had to rewatch that a few times

[sand raven]

I feel like this could’ve be better described initially than just sky broken

[drifting ice]

My best guess without delving too deep is something to do with the Level#advanceDaytime patch ignore me

[round perch]

I found the issue, the custom time sync didn't tell the client whether it should tick time or not

[drifting ice]

Ah okay

[round perch]

@toxic river the latest commit should fix it

[toxic river]

ty

is wolf armor rendering broken?

[ancient frost]

can you please stop doing that, ever?

the thing where you say "x is broken" or "is x broken"

be specific

[drifting ice]

An image would also be useful

[toxic river]

this

[sand raven]

I don’t know what “this” is

[toxic river]

The armor looks broken

black

[ancient frost]

I don't see any armor on the horses

[toxic river]

wolf*

[ancient frost]

wolfs aren't horses

[drifting ice]

I don't think it's broken

[toxic river]

Are you sure? /j

[drifting ice]

If there's no dyeable tag, it should come out as black

[toxic river]

no?

with Vanilla

with Neo

[drifting ice]

Are you sure you don't have it dyed with your command block flag?

[wheat mango]

that's wrong

[toxic river]

nope

[ancient frost]

is this a new world or an old world that you loaded in 1.21.2

[wheat mango]

by default wolf armor isn't black, it has an armadillo pattern

almost 100% sure

yeah it should be looking like this

oh

i'm stupid

[drifting ice]

Ok, I think I found the issue

Getting the default dyed color doesn't check the component, so it returns the overlay color as an opaque black

https://github.com/neoforged/Kits/blob/1.21.2-rc1/src/main/java/net/neoforged/neoforge/client/extensions/common/IClientItemExtensions.java#L220

This should return 0 if DataComponents.DYED_COLOR is not present

[round perch]

The ARGB.opaque() call should just be omitted

That's what the vanilla code does

[drifting ice]

Vanilla still uses the opaque if the component is present

See DyedItemColor#getOrDefault

Oh i see

The wrapper

[round perch]

Yup

[drifting ice]

Got it now

[round perch]

I'm gonna put this in a local branch for later, the equipment rendering still needs some work anyway, so I can just combine that

[drifting ice]

I wonder if we could use the testframework in some manner during the porting process to ensure that things we expect to line up do line up and we just keep adding to it whenever we find misaligned patches or issues

[round perch]

We could, at least for some issues. I don't think this armor tinting issue is realistically testable though, unless we go to the effort of booting up a client in CI and then compare screenshots

[drifting ice]

I was thinking more we replicate the logic and read the values returned by the method, but fair

[toxic river]

Also: model randomization is broken

NeoForge

[round perch]

I don't think writing tests for individual methods like this is maintainable

[drifting ice]

Fair

[toxic river]

Vanilla

[drifting ice]

The only way I could think of doing the screenshot is to basically spawn an nbt environment with the entities and the data, position the player at a certain location for both, and then check

No idea if MC is deterministic enough for that though

[toxic river]

Well, it's not really broken, it just uses a unique randomization for all the blocks.

[round perch]

Re-implemented the ability to prevent effects from being removed by milk and totems: https://github.com/neoforged/NeoForge/pull/1603.
@red jungle you're a bit more well-versed on holder sets, let me know whether that lazy named holder set hack is fine.
@sand raven you were one of the people using this feature, let me know whether this is sufficient for your use case

[drowsy python]

What is happening here?

Did y'all already start working on neo? Or is this just the decomp-recomp stuff?

[sand raven]

As long as I can make my effect not removable by milk

https://github.com/TelepathicGrunt/Bumblezone/blob/2dc4c2034f1d9caf52cbe4c798754721b4895039/neoforge/src/main/java/com/telepathicgrunt/the_bumblezone/mixin/neoforge/effect/WrathOfTheHiveEffectMixin.java#L23

[drifting ice]

Probably is this: https://github.com/neoforged/Kits/blob/9490295fb5400d4143000e5753325147f5646fc1/patches/net/minecraft/client/resources/model/MultiPartBakedModel.java.patch#L49-L51
As a new random source is created each time and not passing in the one provided

It'd probably be fine if it was using the correct parameter j and not k

[round perch]

You would just add the effect to the respective tag like the test does: https://github.com/neoforged/NeoForge/pull/1603/files#diff-c1ad2efa96934e07e1f46e58084728b87499b51cc0c05e04166358a4efb70cfe

[round perch]

Yup, that's the issue. It should just use the incoming random source that is re-seeded on line 50

[drifting ice]

What happened to ElementsModel? It seems to have been removed

[round perch]

Finally deleted because it was a pointless duplicate of vanilla's SimpleBakedModel. The loader's additional behaviors are handled in the BlockModel#bake() now. This is also why UnbakedGeometryHelper is considerably smaller now

[stiff canyon]

Ah, did we incorporate some of the things from my codec-models experiment?

[dreamy obsidian]

Fun fact, I think Mojang introduced a "bug" in their blockstate refactor that I fixed in FC a week ago. If someone manages to call BlockState#setValue with e.g. an int property and a bool value (and yes, this has happened), it will crash with a very nondescript class cast exception instead of the usual "that's not a valid value" message.

[tidal siren]

that seems like something we should patch

[dapper pendant]

Idk if it's worth checking for heap pollution

setValue is generic, right? Right?

Yeah, it's generic, so adding a check effectively just boils down to checking for heap pollution, and there's a lot of other places heap pollution can happen

[lucid totem]

Would be nice to at least get the block name though, which I suspect the ClassCastException won't show

[dapper pendant]

Fair

It might be best to wrap the code in a try-catch and rethrow it as a ReportedException with context

[toxic river]

Water is not rendered behind smoke particles in Fast/Fancy mode

[toxic river]

I had a weird client/server desynchronization bug, I couldn't break any block, no idea how to reproduce the bug

[ancient frost]

spawn protection?

[drifting ice]

Do we have a doc for the registry order of 1.21.2? If not, I'll quickly generate one

[haughty raven]

registry_order.json in neo generated resources no? or is that oudated for 21.2?

[drifting ice]

That would do it thanks

[lunar dagger]

spearking of generated resources, someone make sure to run the data generators

[drifting ice]

I just realized it didn't matter anyway since we don't force the item properties into a supplier in DeferredRegister.Items

[round perch]

Already done, the PR publish wouldn't work otherwise

[haughty raven]

uh i assume junit tests are not supposed to be failing to compile
IngredientTests to be specific

...\NeoForge\1.21.2\tests\src\junit\java\net\neoforged\neoforge\unittest\IngredientTests.java:37: error: cannot find symbol
        Assertions.assertThat(a.stacks()).allMatch(ingredient, "first ingredient");
                               ^
  symbol:   method stacks()
  location: variable a of type Ingredient

or is someone working on ingredient stuff still?

[round perch]

Ingredients are still WIP

[haughty raven]

ah ok, just the only compile error i got
was wondering if it slipped through the cracks unnoticed

[round perch]

I mean, you kinda can't not notice it, it shows up as a CI error on every PR

[lucid totem]

interesting

[drifting ice]

Someone ping me once this is done so I can doc it up

I'll probably be busy the next few days just rewriting all the item docs

[toxic river]

I don't think so, it works when I restart the game.

[ancient frost]

maybe the server thread froze/deadlocked 🤷‍♀️

[toxic river]

Yeah, probably

[round perch]

Misapplied patch

[lucid totem]

oh

cool

mind fixing that so I don't have to wait for 1.21.2-rc1 setup?

[round perch]

Yeah, I'll push in a second

[toxic river]

The name tags above the players/entities are not rendered correctly.

(I increased the opacity to make the bug more visible)

[round perch]

Once again, what's the comparison?

[toxic river]

Vanilla

#mojira message this

[short iris]

[Jump to referenced message](#mojira message) in #mojira

lolothepro30 referenced · Sep 27, 2024, 12:16 PM

3 tickets have just been resolved as Fixed!

MC-108323

Underlines in chat and /title command intersect each other, causing an ugly effect when fading

MC-276005

The text of sneaking players' name tags is rendered completely invisible or barely visible when the "Text Background Opacity" accessibility option is set to high values

MC-276806

The text of name tags behind block or entities is rendered completely invisible or barely visible when the "Text Background Opacity" accessibility option is set to high values

Quoted

[toxic river]

and this #mojira message

(MC-228070)?

[round perch]

I found the issue: our custom text render types (they allow enabling blur on text) didn't match the vanilla ones. This brings up two questions though:

• Wouldn't we be better off just patching that single line in RenderType directly instead of duplicating the render types entirely?
• Considering that rendering in UIs is a lot more batched now, does the conditional filtering even still work properly? If I do enableFilter(); drawText(); disableFilter(); (pseudocode), then the text will very likely not have blur applied because drawText() doesn't flush the buffer

[lucid totem]

Yeah, that doesn't sound right, the blur version needs to be its own render type as it has separate state

[round perch]

Yeah, that'd be the real solution. Bit of a pain, but doable. The only question is where to put the flag. I don't really want to patch all text rendering methods in Font and GuiGraphics, so the only option I can think of is to add a "semi-global" flag to Font, which would be safe since it would be used immediately to retrieve the render type in Font.StringRenderOutput

[toxic river]

Also, I have noticed that in some rare cases, the BE are not rendered.

[round perch]

Screenshots with comparisons or it didn't happen. How many times do we have to tell you that??

[toxic river]

It's so rare that I don't have a screenshot.

and no idea how to reproduce the bug

[round perch]

Screenshot it next time you encounter it. As it stands, there is nothing for me to go off of and I'm not gonna go hunt a needle in a haystack

[tacit arrow]

Any specific BEs in particular? If I had to go out on a limb and guess, it is probably something related to global BEs or the BE culling, so being close to them and at weird camera angles might be how you try to reproduce it for a picture lolo

[toxic river]

It seems to affect the chests and pistons, I don't know about the other BEs.

like this

Here, the piston head is not rendered.

[round perch]

What does the render bounds debug renderer show for that piston?

[toxic river]

nothing

[round perch]

Hmm, I'll have to look into that later

[toxic river]

I don't think it's a Vanilla bug(?)

[round perch]

Ok, the fact that there are no render bounds is correct, the piston renderer uses infinite bounds

[drifting ice]

Ok, I have basically decided to write three new pages for the item docs since they've pretty much changed and need to be broken up into their own separate section

So woo!

And agh too

[round perch]

Doesn't look amazing, but the improved approach works and in an actual use case it serves its purpose (the artifacts have always existed, that's not new)

[round perch]

This should be better: https://github.com/neoforged/NeoForge/pull/1604

[lucid totem]

That is a really large patch for something I'm not even sure anyone is using....

[round perch]

I know that @dusty egret is using it somewhere

To be fair, I would also prefer a cleaner solution (especially visually), but I'm not sure there is any other way to achieve this due to fonts also using an atlas

[dusty egret]

Agree with better solution, if there would be any, but atlas has terrible resolution and AA is not even implemented, AA/multisampling can be somewhat simulated via fragment shader but hell no.. we're using it to rendering in zoomed environment where for small zooming levels (that have non-integer value) the text become pain to see/read, with filtering it's just 50% pain

@round perch btw in terms of breaking changes, could the public static flag stay in the same class?

[red jungle]

I hadn't noticed the failing test

I'll fix it later

[toxic river]

I'm still trying to find a way to reproduce this bug xd

[round perch]

I would prefer to not keep the flag in a barely related place, Font is a much better place for it since that's where it's used

[toxic river]

um...

I don't know if it works, I tried to make a world to reproduce the bug.

Ok... Vanilla bug

[red jungle]

@final sky should I finish the fluid ingredients?

[final sky]

uhh let me check i have nothing left unpushed

alright yeah, feel free to finish them

they still need additional tests for network serialisation imo

and idk if you've made a stacked SlotDisplay for SizedIngredient yet

but if so we need one for FI as well

[red jungle]

I haven't and just removed the stacks() method

[final sky]

hmm

does seem like a qol worth adding at some point imo

[red jungle]

yeah maybe

I just want to make sure we don't add things prematurely in the porting rush

[final sky]

that said i don't think they need to be cached anymore right?

the displays

since vanilla also just creates and discards them

[red jungle]

yeah

do you remember what needed finishing?

[final sky]

i think i actually had the 15 minutes to go and do most of what i had to do

[red jungle]

also, does anyone know of a codec that always errors i wonder if that already exists

[final sky]

(mostly docs)

[final sky]

the only thing coming to mind is the conditional codec

but that also only errors of ops.compressMaps is true

[round perch]

From a quick look, MapCodec.of(Encoder.error(""), Decoder.error("")) should do the trick

[final sky]

oh Encoder.error exists?

damn

wait no that won't work

MapCodec needs a MapEncoder

and that doesn't have an error method

[ancient frost]

do you want a codec or a mapcodec

[final sky]

MapCodec specifically

[red jungle]

we can use assumeMapUnsafe here

[final sky]

anyways i think only what i said above now that i look at it again

[red jungle]

the conditional codec?

where?

[final sky]

^

this and moving things to unit tests was about it for my work iirc

and sized slot displays, which can be in a later PR

[ancient frost]

    
    public static <T> MapCodec<T> error() {
        return MapCodec.unit(0).flatXmap(x -> DataResult.error(() -> "error"), x -> DataResult.error(() -> "error"));
    }

[final sky]

i just did this tbh

MapCodec<SimpleFluidIngredient> erroringMapCodec = new MapCodec<>() {
           @Override
           public <T> Stream<T> keys(DynamicOps<T> dynamicOps) {
               return Stream.empty();
           }

           @Override
           public <T> DataResult<SimpleFluidIngredient> decode(DynamicOps<T> ops, MapLike<T> mapLike) {
               return DataResult.error(() -> "Simple fluid ingredients cannot be decoded using map syntax!");
           }

           @Override
           public <T> RecordBuilder<T> encode(SimpleFluidIngredient ingredient, DynamicOps<T> ops, RecordBuilder<T> builder) {
               return builder.withErrorsFrom(DataResult.error(() -> "Simple fluid ingredients cannot be encoded using map syntax! Please use vanilla syntax (namespaced:item or #tag) instead!"));
           }
       };

[red jungle]

I think I'll just keep that 🤷

adding this:

/**
 * Base fluid stack contents factory: directly returns the stacks.
 *
 * <p>Fluid equivalent of {@link SlotDisplay.ItemStackContentsFactory}.
 */
public class FluidStackContentsFactory implements ForFluidStacks<FluidStack> {
    public static final FluidStackContentsFactory INSTANCE = new FluidStackContentsFactory();

    @Override
    public FluidStack forStack(FluidStack fluid) {
        return fluid;
    }
}

for "symmetry" with items

[final sky]

oh shit did i forget to add that

welp, that was one thing then :p

hm

[red jungle]

I went through the PR and I think we're good enough for a first pass tbh

[final sky]

one thing we definitely need to solve is how to do

• stacking slot displays
• with tags
• without losing the tag information in the SlotDisplay

[red jungle]

if there's a bug in some stream codec it will be found soon enough

[red jungle]

wdym "losing the tag information"?

I'm more concerned about losing remainder info

[final sky]

that too

in general just

no slot display information loss

[red jungle]

I think this is just a vanilla problem in general, unsure how we'll address it

(and to what extent)

[final sky]

that is true, i think the slot displays may need a second pass in future versions

[red jungle]

yeah

it's just really hard to figure out what exactly the recipe viewers need atm

[final sky]

that said, the tag slot display explicitly contains the tag key used

including when sent over network

[red jungle]

especially for vanilla recipes; modders can always make their own displays to pass more info

[final sky]

which would mean no more guessing games for recipe viewers

[red jungle]

yeah that's very nice

[final sky]

i'll just err on the side of caution and say all recipe viewers :p

i wonder if displays should be like... decorators almost?

so you can only ever add information on top

(or unwrap to a lower level with less information)

welp, not like that's for us to figure out, that's for mojang to think over

[red jungle]

forwarding to an inner display for unknown factories is quite easy

I pushed a fix for IngredientTests, now all PRs should pass CI

(once that is merged in of course)

[red jungle]

@final sky why is there still a TagFluidIngredient?

wait nevermind

I am on the wrong branch

niiice

love that FML bump 😄

[red jungle]

tsss max using random values in a test

[ripe drum]

fox

where's that spinning fox emoji

[haughty raven]

[final sky]

randoms in tests are good for one thing and one thing specifically -- making a value that deliberately should never matter :p

[red jungle]

that's still bad I think

you can still make it some constant heh

[ancient frost]

https://xkcd.com/221/

xkcd: Random Number

RFC 1149.5 specifies 4 as the standard IEEE-vetted random number.

[blazing steppe]

hmm rc2 tomorrow?