1.20.5/6 Snapshots

[timber jay]

Heya, how would I check if an ItemStack is Food on 20.5? On 20.2 it was isEdible on ItemStack

[blazing valve]

check if the stack has DataComponents.FOOD

[timber jay]

Awesome, ty

[blazing valve]

Updating to pre2

This time I remembered there is a workflow 😄

[elder swallow]

can you squash everything and rebase afterwards?

[blazing valve]

I can, but I am still investigating how that kits stuff works

I assuming this? https://github.com/neoforged/actions/actions/workflows/start-kits.yml

[elder swallow]

yes

you can run it manually

[blazing valve]

I am writing this shit down

1. Start Kits branch with action
   1. Press "Run workflow"
   2. Fill out the survey.
      1. The workflow checks out the specified branch from this repository.
      2. It updates minecraft_version in gradle.properties with the given Minecraft version.
      3. It updates neoform_version in gradle.properties with the given NeoForm version. Grab it from its project page. The version does not include the Minecraft version (i.e. 20240415.193619).
   3. Wait

[fallen igloo]

yep

[blazing valve]

Oh no don't pin it

this is just a snapshot of what I am writing in an MD file to PR to the repo 😄

* What went wrong:
Execution failed for task ':base:cacheVersionExecutableClient1.20.5-pre1'.
> Failed to download game artifact EXECUTABLE for CLIENT

Is this intermittent?

[elder swallow]

could be yeah

[blazing valve]

remote: This repository moved. Please use the new location:        
remote:   https://github.com/neoforged/Kits.git

I think the action is using a lowercase link

[blazing valve]

First draft: https://github.com/neoforged/NeoForge/pull/805

[blazing valve]

1.20.5-pre2 is done and on the main repo. Continuing with Squash/Rebase

PS: I have no clue how to run our clientside game tests, there's an overlay and.... then what?

[elder swallow]

you can enable a specific one with /tests enable <test id> and then try to validate it

other than that idk 😛

[blazing valve]

And you do that on every change? 😛

I'll not finish the rebase today, Patch engine failure. I really need to check Diffpatch and see why it can't just reject the hunk instead of throwing like that

[elder swallow]

And you do that on every change?
of course not 😛

client tests are not really automatable - that's kind of the point

in theory we could pay a QA team to go through them for every commit

[blazing valve]

You can automate parts of it

Anyhow... This sucks. Running setup with -Pupdating=true

[elder swallow]

did you merge patches?

[blazing valve]

Apparently it did that automatically 😄

[elder swallow]

ah ^^

@fallen patrol any reason why diffpatch can't reject invalid hunks?

[magic pivot]

Agh, why did pre2 have to come out already

Still not done documenting pre1

At least a good 20 mousewheel spins away

[fallen patrol]

Re-run with -si, Patch engine failure is an assertion that failed during patching

[stray bay]

are we gonna have to provide a way to register splash screen backgrounds for teleporting between dimensions?

[teleporting between the nether and the end with commands seems to default to the nether's background]

[true jacinth]

twilight forest has such functionality

[rigid walrus]

It's currently determined by which dimension you are teleporting to or from where if either of those are the nether it uses the nether portal then it checks if either of them are the end where it uses the end portal render type otherwise it will render the menu background as if you aren't in a world
If you are logging in it will use the menu background
If you are entering the end exit portal it will use the end portal render type
If you are respawning it also uses the menu background

[stray bay]

is it hardcoded or is there something in levelstem

[rigid walrus]

The ReceivingLevelScreen constructor takes an enum value which the respawn packet determines by using the first check and is hardcoded

[stray bay]

i bet the enum's sent by ordinal, so we don't want to extend it [at least not without a synced registry]

could add our own packet

maybe a packet to be sent before the respawn packet that sets a "use this instead of the enum" value

[rigid walrus]

It's not included in the respawn packet. It's determined by the client after receiving the respawn packet

[stray bay]

ah well

we could still send an override value ahead of the respawn packet

i guess the question is if the server should be responsible for any part of this

[fallen igloo]

my idea was to patch in a hook and basically ignore the enum

[dapper hornet]

does the artifact publishing for the PR need to be started manually or does it publish automatically?

[restive raft]

once a Maintainer checks the checkbox on the PR, it'll automatically publish on each push to the PR

(the checkbox is automatically ticked for PRs opened by Maintainers)

[blazing valve]

I believe there was something about conflicts preventing the publish?

[restive raft]

ah yes; merge conflicts on PRs prevent publishing as well

[dapper hornet]

Cannot use ID syncing for non-synced built-in registry
What does this mean ?
Let me know if this is too early in porting process to bring up issues

[stiff galleon]

the short version is that neoforge has a registry that's supposed to be marked synced but isn't

[dapper hornet]

its one of my custom registries. how does the marking work ?

[stiff galleon]

oh, you do that when you create the registry, there should be a method on the registry builder

the long version is, when someone sends a packet with a registrable in it, you can either refer to it by its string id or its int id

int ids are smaller for packet size, which is good, but they require that the stuff have the same int ids on both sides

in vanilla, this is a given, but in a modded environment, this isn't deterministic because parallel modloading + client can have extra mods the server doesn't

so fnorge has a system whereby the set of int ids of certain registries are synced from server to client to ensure the ids are the same on both sides

such a registry is called a "synced registry" and they can be marked as such in a registry builder

a "built-in" registry are the static registries, like blocks and items (as opposed to dynamic/datapack registries, like biomes and dimensiontypes, which mojang has created their own mark-this-registry-as-syncable system for because mojang does need it for dynamic registries because datapacks have the same problem as mods here)

[blazing valve]

It only matters if you write numeric IDs for registry objects to packets

If you only ever write the string IDs, it doesn't matter

[fallen igloo]

well the standard RFBB stream codec for registry entries is using the numeric id

[blazing valve]

Yes, that is true. It depends on how they port. But I suppose in general the sanity check will prevent footguns heh

[reef parcel]

Yet another pre release :p

[restive raft]

<@&1067092163520909374> 1.20.5-pre3
https://mdcfe.dev/mc-changes?ver=1.20.5-pre3
https://www.minecraft.net/en-us/article/minecraft-1-20-5-pre-release-3
https://github.com/neoforged/Snowman/commit/e28cd2c3197e97a9e848910c5260ad534ed50829

[magic sierra]

Minecraft 1.20.5 Release Candidate 3 wtf

[lean anchor]

o.o already?

[cloud raven]

[slow prism]

lol

[restive raft]

given the ID of the version, it's probably a mistake on their end

[prisma locust]

rc3 was so good we skipped rc1 and rc2

[lean anchor]

Betting it was the serialization bug that tipped the scale

270668

[reef parcel]

Blog post correctly says pre release

[restive raft]

https://github.com/neoforged/Snowman/commit/e28cd2c3197e97a9e848910c5260ad534ed50829 Snowman

[lean anchor]

Any -level code derps in there?

[restive raft]

does this count

[spiral radish]

They're now caching the hashCode of ItemContainerContents
And added a lot of hashing to codecs by the looks of things

[summer furnace]

I'm updating my prediction of RC1 for later this week and 1.20.5 next week

[prisma locust]

VoxelShape#getCoords changed from protected->public in pre2 and from public->protected in pre3

[median wasp]

Might've been visible for testing but they forgot to annotate it as such

[blazing valve]

Fuck gotta hurry with FML stuff

[restive raft]

someone double-check me: break-able labels can only be on loops, right?

[blazing valve]

🔥

[blazing valve]

No, I was just as surprised

It's poor-man-goto in Java

[slow prism]

nope

[open kindle]

what the fuck

[blazing valve]

It doesnt even need to be an if

[restive raft]

this is gonna make me crack open the JLS

[blazing valve]

label: {
break label;
}

also works

[median wasp]

Oh yeah that's normal

JDK 8 uses that as goto

[spiral radish]

I've used that before

[prisma locust]

guessing this is to cut down on allocations

[spiral radish]

Cursed code is fun

[restive raft]

A break statement with label Identifier attempts to transfer control to the enclosing labeled statement (§14.7) that has the same Identifier as its label; this enclosing statement, which is called the break target, then immediately completes normally. In this case, the break target need not be a switch, while, do, or for statement.

JLS 21 ed., sec. 14.15 "The break statement"

[median wasp]

So basically there was a bug report on data component saving being super slow

[restive raft]

a switch-a-roo

[lean anchor]

Wait, is Codec.EMPTY not a codec? lolwat

[stiff galleon]

might be a mapcodec

[restive raft]

it's a MapCodec

[stiff galleon]

for {}

[restive raft]

https://github.com/Mojang/DataFixerUpper/blob/7e3103305ec9db060fac04478dfc08179d04b1fa/src/main/java/com/mojang/serialization/Codec.java#L741

[stiff galleon]

yep

[restive raft]

MapCodec<Unit> EMPTY = MapCodec.of(Encoder.empty(), Decoder.unit(Unit.INSTANCE));

[lean anchor]

[plucky brook]

MapCodec is better anyways

[fallen igloo]

I say rc1 friday release monday or tuesday

neoform needs manual updating (again)

[magic sierra]

I have a question:
Does Mojang seem to be preparing big changes for Minecraft 1.21?

[magic pivot]

Agh! More pres!

[summer furnace]

there's a few minor differences between bedrock and java I think?

but I don't foresee any crazy changes between 1.20.5 and 1.21

I'm hoping to be surprised by some secret new feature though :P

[clear bane]

uh... yes?
but why don't need to show any of that for a different version
they will start with the 1.21 snapshots when they can
and then they will have a lot of new things

[fallen igloo]

(datapack blocks?)

[magic sierra]

I hope my bug report will be fixed soon X)

[restive raft]

aight, how many layers of shulker boxes / chests did you do /jk

[naive hound]

That can't be the first time that's reported. Overloading a chest with written books has long been used by trolls and griefers to chunk-ban people from servers

[magic sierra]

a lot

[clear bane]

but this has been a thing for like.... since the beginning of chests lol

[misty crow]

As far as I know, chunk banning has only been an issue when connecting to a server and is also not a corruption issue since the affected area of the world can be loaded and saved without issues, you just get kicked when you enter the chunk. What Lolo is running into is in singleplayer though, so it's a completely different thing, especially if it actually corrupts the world

[magic sierra]

affects 1.20.5 pre2 NeoForge port

[reef parcel]

VF 1.10.1 is out btw, those fixes would be nice

[fallen igloo]

welp since we didn't do the port yet... coeh/shartte please bump VF

[lean anchor]

Isn't singleplayer now a proper "multiplayer" in 20.5, because the network stuff actually runs?

That'd be why you can suddenly chunkban yourself

[blazing valve]

I am back home now will do

[elder swallow]

cool thanks

please also do the full 1.20.5 squash and rebase 😄

[blazing valve]

NeoForm first and this time I am fiddling with the groovy script

[elder swallow]

in case you are waiting for stuff to compile... I addressed your comments on the FML PR ^^

[blazing valve]

Does anyone know how I would make Gradle complete a set of tasks and only fail at the end

if any of the tasks failed?

Since the neoform tasks are split into project{shared,client,server,joined}Apply

If I run projectClientApply, it fails on projectSharedApply and just drops the client patches 👀

[elder swallow]

try this?

of course if there's a dependency it's still not gonna work 😛

[blazing valve]

well there obviously is

projectClientApply depends on projectSharedApply

[elder swallow]

well then you're probably out of luck 😛

[blazing valve]

Well I just made ApplyPatch not fail for now, but that's obviously only a stopgap

[elder swallow]

any reason why you don't just fix the patches

[blazing valve]

I want to fix them all

Not shared first, then client later 😛

I really need to grok why the hell we need to maintain a separate patchset for joined

They only strip at the class-level now, I think?

[foggy steeple]

That is the first time I seen the word grok said in a conversation

[blazing valve]

I am not a native speaker, you'll have to excuse me 😛

[foggy steeple]

No like, rare word!

[blazing valve]

https://github.com/neoforged/NeoForm/pull/6
1.20.5-pre3, Vineflower 1.10.1, re-add parameter metadata

Oh I comitted my ApplyPatch "bandaid", gotta revert that

[elder swallow]

the neoform build hasn't started?

[blazing valve]

https://projects.neoforged.net/neoforged/neoform
pre3 is published here, so...?

[rigid walrus]

I already did pre3 early last night

[elder swallow]

what about shartte's PR/commit

does it not auto-build

[blazing valve]

Wait. pre 3 was published a few hours ago. Now I am getting extremely confused 😄

[elder swallow]

what did you do 😄

[blazing valve]

Oh I don't know which timezone coehrlich is in 😅

So maybe my changes only relate to VF 1.10.1

[elder swallow]

a very positive one

[slow prism]

new zealand

so it's the most positive

[fallen igloo]

not quite there are a few islands in the pacific that are more

[blazing valve]

Okay, so process question: Kits for pre3 didn't produce rejects, but obviously for some patch files the alignment changed, or the context

Do I still commit that to Kits and make an update commit to the porting branch?

(Commit the realigned patches that is)

[elder swallow]

yes

it doesn't hurt

well as you want

[fallen igloo]

shouldn't we do a session of going through all // Porting <version> comments before we do the squash and rebase?

[blazing valve]

Can't we do that after the squash / rebase?

[blazing valve]

Yeah it throws, but there's nothing to catch it, I don't think running with different args is going to change that 🤔

[elder swallow]

yes let's do it after

[blazing valve]

Here: https://github.com/TheCBProject/DiffPatch/blob/07808bf5623e8aeb410462579040cc9e30da52c2/src/main/java/codechicken/diffpatch/patch/Patcher.java#L101

[blazing valve]

Done

I moved the code that decorates the encode/decode errors for custom payloads into the stream codec, otherwise straightforward

[elder swallow]

cool

even if there are more pres or rcs, we will just wait for 1.20.5 now

[blazing valve]

Keep in mind that this is still with VF 1.10.0

wdym wait? 😅 So no more ports? 😛

It's only bugfixes anyway I suppose

[elder swallow]

well we could but no more rebasing or squashing

this means we can start to look into Big Refactors™️

[slow prism]

if we start merging actual feature PRs then no more squashing

[blazing valve]

yes as long as there's no movement on 1.20.x that doesn't need to happen either

The PR publish did update

[elder swallow]

updated: https://github.com/neoforged/NeoForge/pull/774

I'll merge tomorrow

[prisma locust]

I'll try to find time to rebase the model data PR on 20.5-pre3 tonight, assuming it needs a rebase

[blazing valve]

Plastered you with a few review comments. We should take this opportunity to at least write some docs in the code

[elder swallow]

damn why did I ask for comments 😩

[magic pivot]

Should probably ask, but does the new data pack registry event now support adding to the in-game reloadable data pack registry (the one that handles loot tables and co.)?

[true jacinth]

the new what now

[lean anchor]

Big +1 as a modder to "change ModLoader.get() to something like ModLoader.INSTANCE

Or static-ing the methods ie ModLoader.isLoaded("mymodid")

[true jacinth]

static methods would be preferred

[naive hound]

Agreed

[lean anchor]

IMO the isLoaded would make sense to be on FMLEnvironment/Environment too. Imagine Environment.isModLoaded("mymodid")

prime example of why static-ing would be nice

...should probably move to #1187879036815417456

[true jacinth]

Its the same reason I static'd the network registry

[fallen patrol]

Gradle should catch it

[magic pivot]

Like adding a new registry to the data pack registries

I phrased it badly

[true jacinth]

I guess that might matter on what you're referring to

but let me double check the status of those

last I saw Loot Tables got migrated to a "real" reloadable registry but the others were still reload listeners

[magic pivot]

Maybe? That sounds right

I think that encompasses three registries though

Since all of the loot data (conditions and co) would’ve been migrated too

[true jacinth]

the force pushes to port really fuck up prs huh

[shut wraith]

Yep it's a real pain to fix stuff up on a PR after one, especially if there's conflicts

But in general rebasing to the upstream branch with --interactive and dropping anything before your PR works, if you're as non-git-savvy as I am

[foggy steeple]

YES

[shut wraith]

Is there a reason the porting stuff is done this way with squashing instead of just a commit/commits per version port or whatever?

Like, why can't there be multiple Port to ABCDEF commits for each version it's ported through?

[true jacinth]

I dunno

I don't touch this process™️

[shut wraith]

If you want major refactor PRs ready to merge close to when there's a release, not doing the force pushes would be a lot easier -- so I assume there's some reason for it that I'm not aware of

[foggy steeple]

#1134480199937957969 message

[dry stump]

technici4n

[Reference to](#1134480199937957969 message) #1134480199937957969 [➤ ](#1134480199937957969 message)that's what we do to keep a linear history 😛

telepathicgrunt referenced · Apr 16, 2024, 10:58 PM

[shut wraith]

I don't see how it's necessary to keep a linear history

Say you've ported to pre 1. You can squash all the pre1 -> pre2 changes into one commit on top of that

[dry stump]

gimpansor

[Reference to](#1134480199937957969 message) #1134480199937957969 [➤ ](#1134480199937957969 message)This force push was only to rebase it on top of 1.20.4 now that 1.20.4 has been frozen, giving us a nice linear history

telepathicgrunt referenced · Apr 16, 2024, 10:52 PM

[foggy steeple]

Idk, that’s what I’ve been told lol

[shut wraith]

Ah, right, stuff happening in 1.20.4 still while everything is going on in 1.20.5 land too

I still feel like squashing together all the porting work is a mistake, and makes rebasing stuff a bit more annoying, but whatever, I suppose I see the logic

[prisma locust]

@elder swallow can you review https://github.com/neoforged/NeoForge/pull/802 one more time and merge it if you're happy (alongside whatever else you're merging)?

we've had enough eyes on it at this point, I think

[slow prism]

I should probably set up publishing of the port branches to the snapshots repo

[fallen igloo]

1. why are we merging stuff before we fix the porting comments
2. why are we merging stuff before we update VF

[wooden tendon]

Yeah people, first have a working Snapshot

Then merge shit on top

[fallen igloo]

well too late now they merged the first feature PR

tech why so impatient

[slow prism]

the port was squashed already

also vf was updated

[blazing valve]

Oh it does, but it stops applying any of the subsequent patches in the batc

[blazing valve]

nope, no Neoform release was made with the new VF

Oh wait, now there is one, apparently 🤔

[fallen patrol]

Okay.. But how am i meant to debug the issue, without a stack trace?

[blazing valve]

Well, I linked the exact line where the exceptio is thrown 😛
Here: https://github.com/TheCBProject/DiffPatch/blob/07808bf5623e8aeb410462579040cc9e30da52c2/src/main/java/codechicken/diffpatch/patch/Patcher.java#L101

I can grab a stacktrace if you want

[fallen patrol]

You did?

Ahh, you did

I missed that

Well, that error is quite severe, means that it found an exact match of where to apply the patch, but when applying, the context was different.

Will need steps to reproduce, etc

preferably just the input + patch file

[fallen igloo]

TODO:

• net.minecraft.client.gui.screens.worldselection.WorldOpenFlows.java (68, 46) public class WorldOpenFlows { // TODO 1.20.3 PORTING: re-add the autoconfirm code
• net.minecraft.client.particle.ParticleEngine.java (457, 16) //TODO porting: is this even needed with the particle render order fix???
• net.minecraft.util.datafix.fixes.StructuresBecomeConfiguredFix.java (186, 16) // Porting 1.20.5 check if this is correct
• net.minecraft.world.entity.Entity.java (2803, 217) net.neoforged.neoforge.event.entity.EntityEvent.Size sizeEvent = net.neoforged.neoforge.event.EventHooks.getEntitySizeForge(this, pose, entitydimensions, entitydimensions1, entitydimensions1.eyeHeight()); // Porting 1.20.5 check if this still works
• net.minecraft.world.entity.LivingEntity.java (871, 12) // Porting 1.20.5 re-add or remove PotionColorCalculationEvent
• net.minecraft.world.inventory.GrindstoneMenu.java (137, 70) if (this.xp != Integer.MIN_VALUE) return ItemStack.EMPTY; // Porting 1.20.5 check if this is correct
• net.minecraft.world.item.BowItem.java (28, 44) if (!itemstack.isEmpty()) { // Porting 1.20.5 redo EventHooks.onArrowLoose
• net.minecraft.world.level.block.RailBlock.java (168, 57) return p_55402_; // TODO 1.20.3 PORTING: reevaluate
• net.minecraft.world.level.block.StemBlock.java (93, 145) if (p_222539_.isEmptyBlock(blockpos) && (blockstate.is(Blocks.FARMLAND) || blockstate.is(BlockTags.DIRT))) { // TODO 1.20.3 PORTING: reimplement canSustainPlant check
• net.minecraft.world.level.saveddata.maps.MapDecoration.java (12, 119) public record MapDecoration(Holder<MapDecorationType> type, byte x, byte y, byte rot, Optional<Component> name) { // Porting 1.20.5 this is synced now reevaluate shouldRenderForIndex
• net.neoforged.neoforge.client.gui.widget.ModListWidget.java (37, 47) //this.setRenderBackground(false); // Porting 1.20.5 still needed?
• net.neoforged.neoforge.common.BasicItemListing.java (55, 120) ItemCost cost = new ItemCost(price.getItemHolder(), price.getCount(), DataComponentPredicate.EMPTY, price); // Porting 1.20.5 do something proper for the components here
• net.neoforged.neoforge.common.CommonHooks.java (521, 12) // Porting 1.20.5 redo this for components?
• net.neoforged.neoforge.debug.attachment.AttachmentTests.java (30, 35) public class AttachmentTests { // Porting 1.20.5 nuke?
• net.neoforged.neoforge.debug.item.ItemTests.java (133, 16) // Porting 1.20.5 replacement?
• net.neoforged.neoforge.debug.loot.GlobalLootModifiersTest.java (142, 100) return loottable.getRandomItems(builder.create(LootContextParamSets.EMPTY)); // TODO - porting: we need an AT
• net.neoforged.neoforge.oldtest.item.HiddenTooltipPartsTest.java (55, 17) // TODO PORTING 1.20.5 - fix or remove test

should we revert and reopen the merged feature PR and then bump VF

[true jacinth]

why does the order matter

[slow prism]

why? we can keep linear from here

[fallen igloo]

because we can not squash after we merged the first feature, everything we do now will be its own commit on the main branch

[true jacinth]

There's like 9000 commits on the main branch

we can have another "port to pre4" or whatever if necessary, but it doesn't matter that the whole "port to 1.20.5" block is a single commit

[summer furnace]

specially considering the snapshots/prereleases are publicly available for development purposes

[slow prism]

thats fine

you can squash a set of fixes

[blazing valve]

The squash was only relevant to make it linear with regards to the 1.20.4 branch, that's pretty much it

As long as we don't start merging into 1.20.x, we'll be fine

[summer furnace]

everyone cares WAY too much about linearity, too

yes it can get messy if you use merge commits in merge commits that are merged to a branch and then merged to another branch before merging to the main branch

but even if we did have merge commits for major feature work, it would still be easy to keep things clean

[fallen igloo]

if I could share an image of how our work git repo looks like I would, non linearity escalates quickly

[slow prism]

also yes

[blazing valve]

Well yes, it was decided to do it, so I did it 😛

[summer furnace]

squashing is fine, I'm just against this sense of "must preserve the purity of the sacred timeline!"

nah fuck it, if we have 3 port commits we have 3 port commits, who cares

[blazing valve]

Having multiple port commits does not even go against linearity. It's just in relationship with the 1.20.x branch. The only reason to squash against it really is the effort required to rebase it on that branch

[summer furnace]

yeah that was an aside

[blazing valve]

We can just update Neoform using a bog-standard PR to port/1.20.5

[summer furnace]

IMO, it would have been fine to continue 1.20.x development in parallel, and simply have one merge commit from the 1.20.5 porting branch. limiting the repo to this kind of merge commit would still be perfectly manageable. but I get why we don't want to, even if I don't agree with the reason. :P

[fallen igloo]

is there a neoform version with the new VF yet?

[blazing valve]

There was only one here yesterday: https://projects.neoforged.net/neoforged/neoform

But now there's two

Yeah the new one references VF 1.10.1

[fallen igloo]

I will rename the pre3 branch on kits and then run the action

[rigid walrus]

NZST is UTC+12 so I am usually asleep when mojang releases something

[fallen igloo]

well we normally wait for you but someone got impatient

[slow prism]

all knowledge being in the head of one person is a bad idea

also running the action on kits is useless

just update locally and push the patch changes...

[fallen igloo]

VF bump can cause patch failures so it is not useless

[slow prism]

so fix them

it's not going to be 40, and ther3s no one to collaborate with right now

[fallen igloo]

we have a workflow maty doing everything locally without anyone seeing anything but patch changes is bad

[slow prism]

you're going to push it, everyone will see patch changes

"workflows" should be guidelines not strict rules to blindly follow in all contexts..

[fallen igloo]

on the kits branch all maintainers can see what happened to the source not just to the patches

[rigid walrus]

NeoForm readme: exists

[slow prism]

does that document how to release

[restive raft]

kekw

[elder swallow]

You had months to fix the porting stuff if you wanted to 😛

[fallen igloo]

fixing porting comments is one of the last things

[elder swallow]

Not really

[slow prism]

we've been at the last stage for weeks

[elder swallow]

We need to move forward with actual breaking changes

Just make a PR for batches of porting fixes

[slow prism]

less complaining, more working

[elder swallow]

this please

[fallen igloo]

we only entered the last stage with pre1

neoform bump is done

[blazing valve]

Nice

How bad is it?

[reef parcel]

new pre can be used for a next step in linearity /j

[clear bane]

pre-4 is out

[fallen nymph]

https://www.minecraft.net/en-us/article/minecraft-1-20-5-pre-release-4

[clear bane]

@restive raft sound the bell!

[spiral radish]

<@&1067092163520909374>
https://mdcfe.dev/mc-changes?ver=1.20.5-pre4
https://www.minecraft.net/en-us/article/minecraft-1-20-5-pre-release-4
https://github.com/neoforged/Snowman/commit/3fb2695d03df73a6da4ad41d8a99dc4725ffe90b

[clear bane]

nvm Ater snipped Sci lol

[restive raft]

at some point, I'm going to learn N8N and automate that /jk

[plucky brook]

Did they update the datapack version again?

[fallen igloo]

if I had access we would have a workflow already

[clear bane]

just learn https://nodered.org/

[reef parcel]

actually, they bumped the resource pack version

[median wasp]

Nope, RP version

[plucky brook]

What are we on now?

[summer furnace]

32 it seems?

[clear bane]

it seems like I will have fun with this in like 3 hours

[lean anchor]

oh god rc4

[plucky brook]

Those versions really are turning into an exponential curve if that continues

[summer furnace]

rc?

[fallen igloo]

no pre4

[plucky brook]

I read that as "the shitty TTF glyph provider ..."

[spiral radish]

Snowman, https://github.com/neoforged/Snowman/commit/3fb2695d03df73a6da4ad41d8a99dc4725ffe90b

[restive raft]

Showing 20 changed files with 541 additions and 115 deletions.

(that's excluding assets)

[spiral radish]

smol

[lean anchor]

Guessing this one was 270838/270845, since that breaks maps

[restive raft]

huh. for some reason, Snowman is being regenerated

@spiral radish FYI

theoretically, it should still come out to the same commit hash

but you ought to be informed just in case

[spiral radish]

TC iirc was building twice

First probably finished before the second?

[restive raft]

I'm going to set a limit on it

so it doesn't happen again

should I let it run fully, or should I stop it, update Snowblower with VF 1.10.1, and rerun it

[spiral radish]

Might as well stop, and update?

Current commit is still available in the meantime

[restive raft]

oh right, yea

[dry stump]

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

coehlrich referenced · Apr 17, 2024, 1:22 PM

NeoForm

Build Successful

Version

1.20.5-pre4-20240417.132209

Build Branch

main

Minecraft Version

1.20.5-pre4

Quoted · Apr 17, 2024, 1:22 PM

[restive raft]

catjam

[rigid walrus]

No changes to patches for neoform required this tine

[restive raft]

oh right, VF 1.10.1 means we can disable the workaround for params that was added in ART, right?

[rigid walrus]

Unless you want a ton of final parameters no

[restive raft]

oh

wait, I'm confused

https://github.com/neoforged/snowblower/pull/5 says it disables use of parameter metadata for parameters with null names, until VF 1.10.1 lands with a fix

isn't that separate from ART's removal of final?

[rigid walrus]

The removal of final was in preparation for when parameter metadata would be reenabled

[restive raft]

hmmm.

ah well. I won't worry about it then

Snowblower's running

https://teamcity.neoforged.net/buildConfiguration/MinecraftForge_PrivateRepositories_Minecraft_Snapshots/2914

[blazing valve]

We'd have to update snowblower separately and remove -ump=0 again

[restive raft]

hmmmm

[blazing valve]

*Update to VF 1.10.1 I mean

[fallen igloo]

workflow is running

[elder swallow]

There's no point in updating to each pre release

[fallen igloo]

ffs I thought we fixed this https://github.com/neoforged/NeoForge/pull/794#issuecomment-2061482418

[prisma locust]

But does it matter now that we froze 1.20.4?

[fallen igloo]

it doesn't matter even without the freeze

[restive raft]

oop

it is done

[slow prism]

the bot should not be in the authors

[elder swallow]

Just squash the bot out

[elder swallow]

It doesn't really matter, no

It's just pointless work

Go fix the porting comments instead of updating to pre N+1 😛

[elder swallow]

Please remove the bot from the commits...

Why is it still there

[fallen igloo]

because I use intellij for squashing which only lets me edit the commit message

[slow prism]

then don't use intellij

[fallen igloo]

no

[elder swallow]

Then yeah that's a skill issue 😛

[blazing valve]

I do it in IJ

soft-reset to the commit before, re-commit with refreshed author

[elder swallow]

I use the git CLI 😛

[reef parcel]

ok, uh. so, I had a branch set up for a PR on 24w12a, but I'm trying to rebase it onto port/1.20.5. the trouble is that it's still tracking the remote which still thinks it's old files for 24w12a are correct, so my local wants to pull those changes before pushing. I'm not a git wizard, so I'm a little lost on how to fix this up aside from creating a new branch off of port/1.20.5

[fallen igloo]

except for when force pushes don't work in IJ it can do everything you need and it should not mater to have the bot in there

[reef parcel]

thanks for reminding me of force pushing...

ugh, ugly history, have to fix that

ok

[wild pine]

would anyone be opposed if i patched in a MutableDataComponentHolder interface, which both ItemStack and FluidStack implement?
it would extend the vanilla DataComponentHolder but also have the set/update/remove methods from both ItemStack and FluidStack

i would like to do this as i have a utility to copy a component from ItemStack onto a FluidStack (and another for vice versa) and would like to make it more generic
copy from DataComponentHolder onto MutableDataComponentHolder.

ive been using this for stuff like potion fluid/containers where i simply copy the potion contents component from container and onto the fluid and vice versa

[true jacinth]

Seems reasonable enough

[elder swallow]

Sure

[wild pine]

aight noice 🙂 will also include my copy utility in MutableDataComponentHolder
so can easily components from 1 holder into another

welp there it is https://github.com/neoforged/NeoForge/pull/817

[elder swallow]

Conceptually I think I prefer copyFrom - then it's always the receiving object that gets mutated

You're also missing a few Supplier overloads

[wild pine]

i am?

[elder swallow]

For remove at least I thought

[wild pine]

looks like both itemstack and fluidstack have it

[elder swallow]

It should be in the mutable interface

[wild pine]

oh i didnt add my supplier pr changes to that

[elder swallow]

I guess a single PR that does both might make sense then?

[wild pine]

yeah il merge the supplier changes into the mutable pr

[wild pine]

pr should be rdy for review https://github.com/neoforged/NeoForge/pull/817

[shut wraith]

My resource loader fixes PR now has some 1.20.5-specific fixes too -- notably, KnownPacks and the new vanilla systems there should now behave sensibly with child packs, and some stuff that was broken during porting was fixed

I'm not going to implement KnownPacks for mod resource packs themselves yet because that needs a lot more testing to make sure it's doing what I think it is

So that part will be a separate PR

Also, we need to poke how SynchronizeRegistriesTask#handleResponse works a bit more -- it might be sensible in a modded environment to patch the equals call there to a containsAll in a modded context given some funkiness on how required packs work? Or maybe that would have other side effects? I have to poke that more -- a problem for the future

Or maybe it's most sensible to instead sent the registries with an intersection set instead of either of those comparisons?

Yeah I'm thinking in a modded environment it might be most sensible to use a set intersection there, but that needs way more testing before implementing -- but this would be what's necessary for mods to use KnownPack I suspect to not end up having it not even be used for vanilla

There's also the worries about a neo client connecting to a vanilla server... (luckily this is actually not an issue! hooray to things working out of the box for once) all fixable but yeah, I've included the bare minimum in that PR to make stuff available for mods to use without bugginess. I'll address all the stuff you'd need for KnownPacks for mod resources by default in a different PR I think

[fallen igloo]

ok I did some stuff for PotionColorCalculationEvent:

• rename to EffectParticleModificationEvent
• make it be fired per effect (since color and visibility is per effect now too)
  should I PR or direct commit (I think we discussed before that maintainers still do direct commit to the porting branch?)

[true jacinth]

That sounds like a PR kinda change

[fallen igloo]

I mean it is a porting comment fix which in the past would even be hidden in the squash

[slow prism]

the branch isn't getting squashed again, PR it

[fallen igloo]

ik it isn't getting squashed so why not just direct commit (I'm in my kits project and don't wanna setup another remote)

[slow prism]

because review

[fallen igloo]

we never had reviews for porting todo fixes

[slow prism]

we never had public porting branches..

[fallen igloo]

that doesn't mean it needs a review for every change (we also don't have reviews on the actual porting commits)

[slow prism]

christ, do what you want

[elder swallow]

PR it if you think other people should review it. Directly commit it if you are very confident in your change.

[fallen igloo]

ok I have done some more work and think we should add an overload for the data component methods that takes a data component type holder

[fallen igloo]

net.neoforged.neoforge.debug.loot.GlobalLootModifiersTest.java (142, 100) return loottable.getRandomItems(builder.create(LootContextParamSets.EMPTY)); // TODO - porting: we need an AT ugh for what do we need the AT here?

[fallen igloo]

can we change TooltipContext#registries to RegistryAccess? (it is always returns a registry access anyway)

also to any mojangsta reading this please move the item attribute modifier tooltip stuff into ItemAttributeModifiers by making it implement TooltipProvider and adding the nullable player to TooltipProvider#addToTooltip
(AdventureModePredicate could get a similar treatment)
just giving you ideas for the next version 😉

[restive raft]

why

c'mon, you gotta give context

[fallen igloo]

12 porting todos left

[restive raft]

(pun not intended, but welcomed regardless)

[fallen igloo]

because you can't get a registry from the lookup

[elder swallow]

There's already a PR for Supplier

I haven't had the chance to review it yet, it's very new

[digital drum]

Im trying to use the snapshot and I get this during gradle reload

C:\Users\Nekretaur\Documents\Chemistria\build\tmp\.cache\expanded\zip_00862ed952d18d88dbbfed4e29883aca\net\minecraft\world\level\block\entity\trialspawner\TrialSpawnerState.java:209: error: cannot find symbol
            return list.size() == 1 ? (Entity)list.getFirst() : Util.getRandom(list, p_338524_.random);
                                                  ^
  symbol:   method getFirst()
  location: variable list of type List<? extends Entity>

[elder swallow]

Probably java version issue

[digital drum]

it is 21 tho

wait i just changed the lang level. lets see

[digital drum]

nothing changed

[dry stump]

Created Gist at the request of @fallen igloo: https://gist.github.com/NeoCamelot/d77bd36c2fc0c6bf6dcc392a887a0889

[spiral radish]

https://gist.github.com/NeoCamelot/d77bd36c2fc0c6bf6dcc392a887a0889#file-message-txt-L20

You've got the LanguageVersion set to 17

[digital drum]

checking now

ty it worked

[digital drum]

RC1 OUT NOW

INFO VIA QUILT BOT

@restive raft

[restive raft]

<@&1067092163520909374> 1.20.5-rc1
https://mdcfe.dev/mc-changes?ver=1.20.5-rc1
https://www.minecraft.net/en-us/article/minecraft-1-20-5-release-candidate-1
https://github.com/neoforged/Snowman/commit/1aac58a73389d873bf0e72cfce16539a319abaf2

[magic sierra]

nice

[dapper hornet]

Upgrading a world with horses wearing leather horse armor makes the horse immune to freezing forever
huh ?? how can that happen lol

[restive raft]

i am 🤏 this close to hooking up a bot to look at the MC Launcher changelogs API

but not yet at that point

[slow prism]

n8n sci

[restive raft]

soon™️

for now, we wait for the launcher manifest

[magic sierra]

I hope there's no rc2

[restive raft]

FYI, according to that changelog,

This week's shipping frenzy continues with our very first Release Candidate for 1.20.5! Barring any significant or critical issues found within it, this is the version we are planning to ship as Minecraft: Java Edition 1.20.5 next week.
(emphasis mine)

[slow prism]

(emphasis yours)

[digital drum]

time to merge the unify tags PR i guess then

[short cove]

So it's 1.20.5 ported, now that you all are working on snapshots.

Is it like done

Or do you just get a head start

[fallen igloo]

If I had access I would have done it

[restive raft]

https://github.com/neoforged/Snowman/commit/1aac58a73389d873bf0e72cfce16539a319abaf2

[fallen igloo]

I'll do the port when we get a neoform and then we only have a few porting todos and PRs

[slow prism]

not if i do it before you

[prisma locust]

can someone review and merge the model data PR

definitely not trying to get out of rebasing it another time

[fallen igloo]

I'd say merge all reviewed PRs before I do the update so we don't need to rebase all of them

[prisma locust]

[slow prism]

the PRs won't need rebasing

[restive raft]

oh dear

[digital drum]

sometimes it feels like they just don't know java

[fallen igloo]

welp report it to the VF discord

[digital drum]

oh its a vf issue

[fallen igloo]

most of the time you think that, it is either a decomp issue or legacy code

[slow prism]

someone should get a vanilla jar and check the bytecode

@restive raft do it kthx

[restive raft]

am hunting it down

[slow prism]

[analog ledge]

wouldn’t that change the actual result?

because it would continue regardless of the result of $$7

[prisma locust]

yeah that was my thought

but it could just be dead code from quick commenting on their end

[restive raft]

        47: aload         4
        49: instanceof    #1311               // class java/lang/Error
        52: istore        5
        54: aload         4
        56: instanceof    #942                // class java/io/IOException
        59: ifne          70
        62: aload         4
        64: instanceof    #1313               // class net/minecraft/nbt/NbtException
        67: ifeq          74
        70: iconst_1
        71: goto          75
        74: iconst_0
        75: istore        6
        77: iload         5
        79: ifne          90
        82: iload         6
        84: ifne          134
        87: goto          134

slot 6 contains the result of $$7
note how both 84 and 87 jump to 134

134 being getstatic on the LOGGER field (the rest of the if-true case)

[open kindle]

the Bytecode :abolbsweats:

[restive raft]

so the bytecode does seem to point to the decompiler accurately reflecting the bytecode

(for the curious, 90 and beyond up to before 134 are part of the else of the if (!$$6)

if no one else has any further poking that they want me to do...
deletes the jars and text files from my desktop

[elder swallow]

What

We are not rebasing anything...

Porting to these versions is just a waste of time. You should focus on more important things

[elder swallow]

I can do it tonight

[fallen igloo]

if there are conflicts in the PRs they need to be rebased on the porting branch

[restive raft]

hey Tech and Schurli, what's your timezones
just asking for my notes

(so I know when not to bother you while you're asleep)

[elder swallow]

Berlin time

[fallen igloo]

check my global about me

[elder swallow]

Ah well that would be more reasons not to do these silly updates 😛

[dry stump]

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

coehlrich referenced · Apr 18, 2024, 2:05 PM

NeoForm

Build Successful

Version

1.20.5-rc1-20240418.140438

Build Branch

1.20.5-dev

Minecraft Version

1.20.5-rc1

Quoted · Apr 18, 2024, 2:05 PM

[elder swallow]

Fixing the todos is a lot more important

[slow prism]

there's not going to be conflicts after this tiny rc

[fallen igloo]

I have done 5 already so don't act like I did none

[blazing valve]

Careful with that, they previously passed around the lookup in way more places and it might actually be hard to get to the registry access

I was discussing with tech however that we might add an optional level

[timber jay]

So uhh, what else am I supposed to use here? of course it doesn't show FMLJMLC is marked as deprecated

[blazing valve]

Why are you asking that in the snapshot thread? 😅

[fallen igloo]

well level is one thing but the lookup in there is always a RegistryAccess if you check where it creates the context

[timber jay]

Because I'm on a snapshot

[blazing valve]

Yes but it was deprecated way earlier 😛

[timber jay]

I'm asking what the alternative is

[blazing valve]

Declare an IEventBus parameter in your mod constructor

[restive raft]

you can get the mod bus by way of a constructor parameter

[timber jay]

IDEA didn't tell me about it lol

[restive raft]

yea, as shartte says

[blazing valve]

We're finally going full enterprise

Dependency Injection!

[restive raft]

aight, time to install Guice and use it everywhere

or whatever DI library we prefer

[spiral radish]

https://tenor.com/view/star-trek-enterprise-enterprise-e-first-contact-gif-14633132243386862688

[timber jay]

I apologize if this was the wrong place to ask about that. I never knew it was deprecated cause IDEA didn't show it was errored 🤷‍♂️

[fallen igloo]

we said we go full enterprise so we have to write our own

/s

[summer furnace]

oh hey rc1 already

earlier than I expected

[restive raft]

as long as I get my FactoryFactory, I'll be happy /jk

[restive raft]

with 1.20.5 on the horizon of next week

when next week? no one knows

Monday, as a guess

[summer furnace]

yeah

once rc1 is out, they always plan to release within a week of rc1

so I would expect monday/tuesday release if there's no last-minute rc<N+1>

[fallen igloo]

you'll even get a FactoryFactoryInstanceFactory /s

[summer furnace]

does that mean the game now does disconnect on invalid packets?

[restive raft]

ah yes, I remember seeing that

they added a @Deprecated field

(from ClientCommonPacketListenerImpl.java, https://github.com/neoforged/Snowman/commit/1aac58a73389d873bf0e72cfce16539a319abaf2#diff-3685a00c47d1b0a6afff909a2a12def46aaf0f4f758352caa134eeac1577f8a7R91-R97)

[fallen igloo]

hmm no more super call

I'm gonna push a dev/fix-porting-todos branch to the main repo for us to do the porting todo fixes on

[blazing valve]

🗡️

I didn't even realise the rc1 dropped

[elder swallow]

My point is that fixing the todos is useful and porting to every intermediate version is not. 5 is a good starting point

I imagined our final commit history would look like:

• Port to 1.20.5-pre1
• ... (normal commits go here)
• Port to 1.20.5

Having intermediate porting commit is just useless noise

[fallen igloo]

it is not since there are changes in between that are nice to have

[elder swallow]

Can you name an example from one of the 1.20.5 pre-releases?

What feature in 1.20.5-rc1 is nice to have compared to pre3?

[fallen igloo]

it's not about features it is about code changes

[slow prism]

such as?

[fallen igloo]

this is the PR for the porting todos https://github.com/neoforged/NeoForge/pull/820

[magic sierra]

Note: The menu listing the installed mods is weird with the "Programmer Art" resource pack.

[elder swallow]

Why did you merge that PR arghh

Why is MutableDataComponentHolder.copyFrom static?

Also the javadoc states copes rather than copies

Yeah and the copyFrom logic in DataComponentHolder seems backwards

@wild pine please fix 😛

[reef parcel]

agreed, copyFrom sounds like it would take from the target and put it on the source

[elder swallow]

@fallen igloo you get a

[wild pine]

i guess i could rename the copy method in IDataComponentHolderExtension back to copyTo
since its copying to target from this

the static method originally existed as the copy method was on MutableDataComponentHolder
meaning would need a instance of MutableDataComponentHolder to invoke it
but i guess since its now on any component holder via IDataComponentHolderExtension
the static method is no longer needed

[elder swallow]

It should be copyFrom

On the mutable interface though

Yeah the static method can go

[wild pine]

why put it in the mutable iface? your not mutating the instance
your mutating the provided target

[elder swallow]

That's the whole point of naming it copy_From_

[wild pine]

putting it on the component holder extension allows any holder (mutable or immutable) to easily copy a value to another mutable holder

[elder swallow]

You put the mutation method in the mutable holder

Then the code becomes mutable.copyFrom(immutable)

No change of functionality, just moving the mutation method to the thing that gets mutated

That will also make it more discoverable

[wild pine]

so delete the extensions copy method
and change mutables to be like so
mutates the mutable itself and pulsl value from the (im)mutable

default <T> void copyFrom(DataComponentHolder src, DataComponentType<T> componentType) {
    set(componentType, src.get(componentType));
}

[blazing valve]

I don't quite get why you are so up in arms about that 😅

[fallen igloo]

it was up for review

[wild pine]

uhh pushed my changes but they didnt show up in the pr
do i have to reopen it first? kinda assumed a push to the branch would auto reopn it

guess ill open a new pr?

[fallen igloo]

ok I created a milestone for stuff to be merged into the porting branch https://github.com/neoforged/NeoForge/milestone/5

[slow prism]

coping

[wild pine]

it conflicts, huh https://github.com/neoforged/NeoForge/pull/821

[blazing valve]

Squash your stuff and rebase it onto the port branch

[wild pine]

squashed and rebased

[blazing valve]

Dunno if we should clarify that copyfrom will clear a local component if the other object doesnt have it

Probably might be the expected behavior anyway, so I presume its fine

[elder swallow]

yes

[elder swallow]

it's probably worth clarifying

just because one might otherwise not think about it

[elder swallow]

idk... I guess it's just the suggestion of rushing to merge/review PRs before porting to pre N+1

[elder swallow]

yes, I reviewed it last night before going to bed and you merged it while I was still at work without giving me a chance to review the updated version?

it's fiiine - I'm just annoyed when people don't wait for my review and miss important details in the PR

[fallen igloo]

there was no change request else I wouldn't have been able to merge it

[elder swallow]

yeah it was mostly discussed on discord

as I said, in principle nothing prevents you from merging it - I am just annoyed when it happens and I need to request (or event worse write) a subsequent fix PR

[fallen igloo]

discussions on discord men nothing for PRs if there is no not on the PR

[elder swallow]

I'm aware. You still need to understand that procedures are guidelines rather than strict requirements. It would have been ideal for me to write something on the PR (which I didn't because it was opened so late...) or for you to wait before merging.

But whatever, Apex thankfully quickly made a fixup PR

[wild pine]

im looking at itemstack and noticed i didnt copy the applyComponents methods to the mutable iface
does it make sense to copy those across, those iirc set a group of components in batch

[median wasp]

So turns out Mojang didn't turn on MethodParameters

They just disabled stripping

Javac will always export MethodParameters for specifically record constructors

It's also generated for methods with synthetic or mandated parameters

But in that case, the name will not be written unless -parameters is passed

Which is consistent with what we see in the game

[summer furnace]

I didn't expect them to enable full parameter metadat

[analog ledge]

wait when?

[median wasp]

I think like the last non pre-release non-rc snapshot or so

Like the one after the J21 bump

I wouldn't be surprised if they saw Jasmine's message about it being extremely difficult for a decompiler to detect the mandated parameters and decided to make that information be kept

[summer furnace]

all the mod loader people begging them directly probably had more to do with it :P

[solar zephyr]

What, they are providing method params now? 😮

[summer furnace]

no, they disabled stripping of parameter metadata that doesn't have an associated field

[solar zephyr]

ah okay, still, that is good

[summer furnace]

so that decompiling inner/anonymous classes isn't impossible

(or well, doesn't require guesswork)

[median wasp]

Javac generates parameter metadata in the following cases:

• If -parameters is passed, always generate full metadata
• If the method is a constructor in a record, generate full metadata
• If the method has synthetic or mandated parameters, generate metadata with names stripped

[summer furnace]

the last one is how you can distinguish ```
public class X {
private int value;

class Y {
public Y() {
doSomething(value);
}
}
}

from ```
public class X {
    static class Y {
        public Y(X x) {
          doSomething(x.value);
        }
    }
}

[median wasp]

Does the top one generate a mandated parameter, since the parameter is still used?

Ok, it always generates it as mandated, regardless of if it's used or not

Do you push the snapshot branches to a maven?

[fallen igloo]

they didn't enable params they just disabled stripping, for us

[fallen igloo]

not yet afaik

[digital drum]

we can use the prmaven tho

right?

[median wasp]

That would be useful for early porting to RCs

[fallen igloo]

yea as long as the publishing succeeds

[fallen igloo]

I mean afaik AE2 is already ported

[digital drum]

wait where? if so can U link it?

[lean anchor]

You really don't want a version build for a non-release version of MC...

[blazing valve]

On our repo

But not the latest snapshot, one of the pres

[median wasp]

Well when I said snapshot branches, I meant port branches

[median wasp]

Where is that?

[elder swallow]

https://github.com/neoforged/NeoForge/pull/794#issuecomment-2048537084

I'll move ahead with the FML update, it's had enough discussion

[prisma locust]

should we be throwing an exception if mods.toml is found in the main mod in dev? I was wondering why performance fell off a cliff and then realized my mod just didn't load...

[fallen nymph]

I spend half an hour until I realized filename change

[median wasp]

I think it should only fail if mods.toml is found and neoforge.mods.toml isn't found

Because there might be people with both

[foggy steeple]

That

[elder swallow]

that's one of the goals of the FML locator rewrite yes

[wild pine]

added couple more methods to my component type pr, also noticed that i missed the applyComponents methods from item stack when originally copying the mutating methods, but im unsure if it makes sense to copy those across or not

[fallen igloo]

yea pull it up, also copyFrom should probably have the src as first parameter and the second be a vararg of types so you can do multiple at once

also why is it now so much more patching?

[summer furnace]

looks like we will be getting an rc2, #mojira

[wild pine]

src was originally the first param but the component type was not a varargs
tech said he preferred it component type being first to be consistent with all the other methods

its also more patching as im adding 1/2 methods to classes and a whole extension iface didnt feel right just to implement 1/2 methods
i couldnt also make a extension iface for the inner iface in BlockEntity (DataComponent.Input) since its protected
would have had to make it public which i think would be a new AT line

[fallen igloo]

imo extension interface patches are easier to fix than whole methods when porting
also the vararg thing would be useful

[wild pine]

i can convert them to be extension ifaces but that would mean making the iface in block entity public
unsure where neo stands on changing the visibility of vanilla classes

well all but 1 can be converted, SetComponentsFunction.setComponent has a supplier variant now and thats static
dont belive i can extension iface that

[fallen igloo]

you can skip that one if it is a problem

[wild pine]

pushed the requested changes, kepts copyFrom but added a copyAll method which takes in varargs
also merged in latest changes (mainly so that i could publish locally and use in my userdev workspace xD)

[median wasp]

rc2 is out

[wild pine]

yeah it do be https://www.minecraft.net/en-us/article/minecraft-1-20-5-release-candidate-2

[spiral radish]

<@&1067092163520909374>
https://mdcfe.dev/mc-changes?ver=1.20.5-rc2
https://www.minecraft.net/en-us/article/minecraft-1-20-5-release-candidate-2
https://github.com/neoforged/Snowman/commit/9ec8a4e81d8f7a38f176b04b7928e11d8bae3b60

[fallen igloo]

[spiral radish]

Very few changes

WinScreen, EncoderCache and VillagerTrades

[magic pivot]

I'm so far behind on the primer...

[median wasp]

Does VanillaRecipeProvider fail in Snowman too?

[spiral radish]

I don't see a failure in that class

[median wasp]

It's failed on both RCs in Ocelot

[restive raft]

decompiler failure?

[median wasp]

Specifically buildRecipes

Yeah, decompiler error

Oh, it OOMed

[restive raft]

ah.

[spiral radish]

:3

VF is hungry

[dapper hornet]

what is Snowman ? why is it called snowman lol

[spiral radish]

Snowman is a private repo containing decomp'd versions of Minecraft, each version applied as a commit ontop of the previous

[rigid walrus]

https://github.com/neoforged/NeoForm/actions/runs/8754578287/job/24027023597#step:9:214

[spiral radish]

It's made by Snowblowerhttps://github.com/neoforged/snowblower

As for the name, it's because when asked to keep the LVT Table in the past to help with decomp efforts, they did so although replacing the names with a snowman emoji

[clear bane]

\⛄

[rigid walrus]

I will deal with this in the morning

[dapper hornet]

so is snowblower an automation script around VF ?

[spiral radish]

Side note, they've since replaced this with $$<index> since Java introduced the helpful NPE
(https://bugs.mojang.com/browse/MC-227065?jql=text ~ "☃" ORDER BY lastViewed DESC for a list of issues made before they did so :3)

[spiral radish]

Yesn't, it's slightly more than just an automation script

[restive raft]

it's a tool that uses VF

[magic pivot]

Ok, finally done with up to 1.20.5-pre1. Now just got to do up to rc2

https://gist.github.com/ChampionAsh5357/53b04132e292aa12638d339abfabf955

[lean anchor]

Long live BootstapContext

[shut wraith]

Note on this: a KnownPack is more than just a key about where something came from, it's a key that gets synced about where it came from -- so if the same KnownPack is used for a resource pack on the client and the server, the pack must have the same contents or you'll risk client/server desyncs

Because the KnownPack key is used to avoid syncing the actual data

[lean anchor]

Typo on second bullet of https://gist.github.com/ChampionAsh5357/53b04132e292aa12638d339abfabf955#resourcekey-references - net.miencraft

[restive raft]

[shut wraith]

Any chance I could get https://github.com/neoforged/NeoForge/pull/735 added to the initial 1.20.5 release milestone or at least the 1.20.5 stable milestone? And could I get some review on it if possible? This one fixes some bugs introduced during porting that are pretty easily visible -- like individual mod packs not being properly hidden any more and generally having strange behavior, and the like

[lean anchor]

• net.minecraft.client.gui.navigation.ScreenRectangle#containsPoint - Checks whether the point is within the current rectangle
  This is nice. Thank you, Mojang.

BlockBox is also neat o.o

net.minecraft.gametest.framework.GameTestRunner$Builder - A builder for constructing a runner to run the game tests

is this a nice, vanilla way to boot up game tests?

[magic pivot]

To a degree, a bit more nonsense is involved since you still need to construct most of the objects yourself

Also, finished now with pre2, so everything should be up-to-date

Besides all the spelling mistakes

Which they are probably a lot

[analog ledge]

[09:31:36.224] [main/ERROR] [loading.ModSorter/LOADING]: Missing or unsupported mandatory dependencies:
    Mod ID: 'minecraft', Requested by: 'fabric_block_view_api_v2', Expected range: '[1.20.5-pre1,1.21)', Actual version: '1.20.5-rc1'
    Mod ID: 'minecraft', Requested by: 'fabric_rendering_data_attachment_v1', Expected range: '[1.20.5-pre1,1.21)', Actual version: '1.20.5-rc1'
    Mod ID: 'minecraft', Requested by: 'fabric_api_base', Expected range: '[1.20.5-pre1,1.21)', Actual version: '1.20.5-rc1'
    Mod ID: 'minecraft', Requested by: 'fabric_renderer_api_v1', Expected range: '[1.20.5-pre1,1.21)', Actual version: '1.20.5-rc1' ```

is this how semver is meant to work

apparantly 1.20.5-rc1 is not between 1.20.5-pre1 and 1.21

[slow prism]

that's not semver

[hexed sonnet]

Forge uses Maven versioning

[analog ledge]

isn’t that even worse

[hexed sonnet]

It's subjective

Still, that's weird

[median wasp]

1.20.5-rc1 should still be after 1.20.5-pre1

Because r is after p in the alphabet

[hexed sonnet]

Yes

[dry stump]

Maven versioning: 1.20.5-rc1 < 1.20.5-pre1

[hexed sonnet]

[restive raft]

absolutely perfect

[plucky brook]

[hexed sonnet]

Are those versions assigned manually during porting?

[magic pivot]

I believe that just has to do with rc recognized as a qualifier while pre isn't

[dry stump]

Semver versioning: 1.20.5-rc1 > 1.20.5-pre1

[fallen igloo]

ah yes semver vs maven versioning again

[magic pivot]

Unknown qualifiers are considered 'newer' than known qualifiers

[prisma locust]

seems accurate - https://maven.apache.org/ref/3.3.9/maven-artifact/apidocs/org/apache/maven/artifact/versioning/ComparableVersion.html

[magic sierra]

Path for java installation '/usr/lib/jvm/openjdk-17' (Common Linux Locations) does not contain a java executable
Shouldn't this thing check /usr/lib/jvm/openjdk-21, now?

[prisma locust]

ComparableVersion doesn't look possible to subclass and add a custom qualifier to

[restive raft]

where's that from?
and is that 21 JVM listed in ./gradlew javaToolchains

[reef parcel]

Yeah, I encountered that versioning issue while porting a mod. Decided to just use 1.20.4 for the mc version. Tbf, forge never had to account for this since snapshots weren't public

[wild pine]

I've just been doing [1.20.5,1.20.6) (mc dep) don't plan to release during snapshot cycle so should be more than fine once 20.5 actually releases
(and [20.5,20.6) for the neo dep)

[reef parcel]

Petition to rename either AddReloadListenerEvent or RegisterClientReloadListenerEvent to match the other

Like AddAssetReloadListenerEvent and AddDataReloadListenerEvent

[summer furnace]

rename both

[reef parcel]

Honestly

[wild pine]

Why not merge them and have register method take in the pack type to register listener for?

[reef parcel]

And I prefer "register" but it is more verbose

[reef parcel]

Bad client/server separation?

As it stands, the client one is in the client package and the other is in common

[wild pine]

i mean everything needed to register reload listener can be accessed on either side (PackType and reload listener ifaces/classes)
so i dont see why there would be any sidedness issues

the thing registering and holding the listeners though is sided but thats shoudlnt rly a issue when both sides are abstracted away via the event, but i could be wrong though

[elder swallow]

We need to rework these events anyway for proper ordering

The server one has access to DataPackContents

[wild pine]

yeah true, that would be a drawback with my method, you loose that server pack context

something like this would be nice though imo

NeoForge.EVENT_BUS.addListener(AddReloadListenersEvent.class, event -> {
    event.register(PackType.CLIENT_RESOURCES, <myResourceListener>);
    event.register(PackType.SERVER_DATA, <myDataListener>);
});

[magic pivot]

I mean it's possible to supply a context if we assume that the listener is a factory function, or we could just have nullables

Factory function would probably be safer if we were to entertain the idea

[wild pine]

i was assuming the listener would be PreparableReloadListener since thats what both events register currently

[magic pivot]

Yes, but I was saying something like ctx -> new Listener where ctx would contain whatever objects were necessary from the client resources and server data reloadable managers

[wild pine]

both events are basicly the same, registering the same listener iface to some reload handler, just 1 has server context the other does not

[magic pivot]

Sure

[wild pine]

using factories in the register to pass the context would be a good workaround/alternative though
1 event while but keeping the server context when registering a data listener

could even pass along the client resource manager or something as the clients context

[magic pivot]

Still, I think two separate events make sense since they are called in different places, no?

Unless we're storing the listeners for both in our own intermediary class to push at a specific point in time

[wild pine]

just use functions no? client would be Function<ReloadableResourceManager, PreparableReloadListener> while server would be Function<ServerReloadListenerContext, PreparableReloadListener>
collect all those functions and have reload manager invoke them after event has posted and pass off the listeners to manager like normal

we would be providing the server context though, but it could be a simple record holding everything the event currently has

this is all off top of my head though so might not be 100% working logic

[magic pivot]

Sure, but once again, the timing for each is different

The solution works if we're storing both listeners in our own intermediary class

On the client, the client reload listeners are called during startup while the server resources on world creation/world load/reload

On the server, the server resources are called immediately

[elder swallow]

It makes no sense to have a single event IMO

The server one also fires once per reload, whereas the client one only fires a single time at startup

[wild pine]

oh yeah, the event itself is fired differently depending on sides
for some reason i thought was talking about the call to PreparableReloadListener#reload which ofc thats called differently the handler manages that call
but the event itself being called differently yeah, cant reasonably merge them in that case

[elder swallow]

that's a lot of Supplier<DataComponentType<?>> patches...

I think you're overdoing it

this is too much patching considering that users can just do .get() where necessary

[wild pine]

yeah i just published locally to use in userdev and see how many places i could remove .value() in
but if its too much i can remove them

[elder swallow]

you can even do .get()

[wild pine]

but that simply invokes .value in deferred holder (unless that changed)

[elder swallow]

yeah it's just a little bit shorter 😛

[wild pine]

true

[wild pine]

i was originally patching the methods right into the class originaly though
but schurli said having them be extension ifaces would be better even if its just 1/2 methods being patched in

[elder swallow]

I'd suggest removing all the new extension interfaces in your latest PR

the problem is that this is just a lot of patching for little value

I'd only keep the ItemStack ones (so MutableDataAttachmentHolder and IDataAttachmentHolderExtension)

idk

CopyComponentsFunction and SetComponentsFunction should definitely be removed

I don't like the BlockEntity.DataComponentInput ones either

idk about copyAll - it is definitely redundant with copyFrom however I don't like having the component type go last

[wild pine]

yeah il remove all the ones i recently added
copy/set components, item properties, component map/predicate and block entity input

[elder swallow]

item properties is fine I think

that one is likely to get used a lot

the data component map builder would also make sense I think

anyway just ask yourself what a good tradeoff is

[prisma locust]

Is anyone opposed to us patching StateHolder to support arbitrary Maps in this snapshot rather than requiring it to be a Reference2ObjectArrayMap? Think FerriteCore would benefit from it

[wild pine]

item properties, block entity input and the data component map builder
are ones i would like to keep and think would be used alot

properties for initial registration
and BE input/component map builder during block entity/item component transfers (BlockEntity#collectImplicitComponents / BlockEntity#applyImplicitComponents)
the rest i will agree is prob abit much

[elder swallow]

we generally don't do random field type changes to suit one specific mod 😛

[misty crow]

Keep in mind that these two events behave very differently. The assets reload listener list is built once during startup while the data reload listener list is rebuilt from scratch on every reload (create world screen, world load, /reload).

[prisma locust]

mk

[elder swallow]

ok the block entity input might also be used quite a bit yeah

potentially move it to an extension interface as well

[wild pine]

i dont think i can since the inner iface of block entity is protected

protected interface DataComponentInput

[elder swallow]

ah well patch is fine then

[wild pine]

as for copyAll, the param consistency is nice
but ability to pass multiple components to copy would also come in quite handy
idk both are pretty handy, im more in favor of copyAll though (replace copyFrom with it)

[elder swallow]

I'd remove copyFrom and rename copyAll to copyFrom

[prisma locust]

we have so many deprecations for removal right now that it's not even funny

[elder swallow]

we'll have a look through them shortly

[wild pine]

yay java generics, cause both get andset are generic typed i assume
but the varargs has no type

[open kindle]

might have to use explicit types instead of vars

and if that doesn't work, replace ? with T

[wild pine]

typing the varargs would defeat the purpose of this method
would basicly be the copyFrom but passing the same component type multiple times

and if i replace var its the same issue
i could do set((DataComponentType) componentType, src.get((DataComponentType) componentType)); however but that looks ugly

[median wasp]

This specific generic issue is annoying

The solution is a private helper method that's generically typed

Then you only have to reference the wildcard variable once

[reef parcel]

well, yeah. the naming just sucks from a user perspective. you fundamentally use them in the same way. The javadocs are for important clarifications ("fired every reload blah blah" vs "fired once at startup")

[fallen igloo]

use DataComponentType raw type instead of var

[wild pine]

then theres this nonesense, wasnt quite sure how to cleanly implement this one

default void copyFrom(DataComponentHolder src, Supplier<? extends DataComponentType<?>>... componentTypes) {
    copyFrom(src, Stream.of(componentTypes).map(Supplier::get).toArray(DataComponentType[]::new));
}

[open kindle]

that looks about right

[wild pine]

i guess now that i have that private typed helper
this would be cleaner

default void copyFrom(DataComponentHolder src, Supplier<? extends DataComponentType<?>>... componentTypes) {
        for (var componentType : componentTypes) {
            copyFrom(src, componentType.get());
        }
    }

and pushed now to wait for more reviews from tech

[elder swallow]

I'd probably put the private copyFrom right below the public ones but it's not very important

[wild pine]

i moved it to the bottom to keep public stuff at the top (the stuff user is more likely to care about)

[elder swallow]

even for the user it's a bit weird to see this other copyFrom method without finding it

but as I said, it doesn't really matter

[median wasp]

Where are snapshot/pre/rc builds published? I would like to depend on them. Ideally rc1 or rc2

[wild pine]

couldve made it static so its still usable but yeah doesnt rly matter rly

[prisma locust]

https://github.com/neoforged/NeoForge/pull/794#issuecomment-2048537084

[reef parcel]

https://github.com/neoforged/NeoForge/pull/794#issuecomment-2048537084

lol

[prisma locust]

wait

we also sniped with the emoji

is there an emoji for double sniping

[reef parcel]

NOOO 😄

[wild pine]

was about to post link myself

[reef parcel]

can't believe I was double sniped

[median wasp]

Thanks

[magic pivot]

Should we be using suppliers still, or should it be Holders?

Since that is what minecraft is patching everywhere, and we have transitioned to using Holders ourselves

[reef parcel]

can't use regular holders for DataComponents, since they require the specific type and the Holder just has the registry type

[wild pine]

yeah holder would be Holder<DataComponent<?>> while the supplier would be Supplier<DataComponent<T>>
you loose the generic type with holders, unless you want to pass a deferred holder around

but for things like blocks/items which dont rly need the specific type, passing a holder makes more sense
like DeferredSpawnEggItem could be updated to take in a holder imo rather than the supplier

[shut wraith]

You forgot a comma in that range

Oh wait I'm silly

Misread that

[reef parcel]

which was the one benefit of DeferredDataComponentType, but I figured people would be favoring Suppliers

[dry stump]

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

coehlrich referenced · Apr 19, 2024, 7:09 PM

NeoForm

Build Successful

Version

1.20.5-rc2-20240419.190916

Build Branch

1.20.5-dev

Minecraft Version

1.20.5-rc2

Quoted · Apr 19, 2024, 7:09 PM

[shut wraith]

... discord why did you make it look like I wasn't scrolled up when I was

[plucky brook]

discord classic

[shut wraith]

Discord plus my bad internet if I had to guess. Ah well

[magic pivot]

Ah, ok

[digital drum]

will java 22 be compatible?

with 20.5?

[shut wraith]

In theory sure -- though if you compile your mods with it people will need it to run them which isn't great

[median wasp]

Especially since 22 isn't an LTS

[digital drum]

there're unnamed variables that I would love to use

[magic pivot]

Unnamed variables in J22? Why make code harder to read...

[boreal flint]

mine did it at the same time lol

[shut wraith]

Unnamed variables for "stuff we're tossing out" -- so, like, if you do a pattern match on a record but only want one component of it, you dump the other in an unnamed variable so you don't polute the space of variable names with stuff you're not using that could lead to confusion.

[magic pivot]

I understand the point, but that doesn't mean I like it

[slow prism]

BiPredicate noop = (_, _) -> false

[reef parcel]

unnamed variables, my beloved

it'd be more useful in pattern matching ofc

switch (someRecord) {
    case SomeRecord(_, int num) -> doSomething(),
    case SomeRecord(String a, _) when a == null -> somethingElse(),
}

hopefully that syntax is somewhat right, haven't actually used it myself yet

[slow prism]

; but yeah it's about right

[reef parcel]

on the cases or at the end of the switch

[slow prism]

cases

they're a statement

[reef parcel]

right, yeah

[blazing valve]

switch is also getting more and more stuff ❤️ https://openjdk.org/jeps/8323658

[stiff galleon]

at least until nintendo releases a new one and shuts down the old servers

[shut wraith]

The new stream gatherers are my personal favorite up-and-coming feature

[slow prism]

holy

switch is the new if

[shut wraith]

New java versions are getting all of my favorite fun toys; I love it. Between the new switch stuff and sealed classes we can finally do algebraic data type stuff in java which is awesome

[slow prism]

delete if

who the heck needs it

[stiff galleon]

mojang dev who wrote those giant if-else chains: *uncontrollable sobbing*

[slow prism]

i only now realised the potential of this

so sad it will take 2 years at least

oracle finally realised how annoying checked exceptions are

[stiff galleon]

https://bugs.openjdk.org/browse/JDK-8325803

In constructors of Java classes, allow statements to appear before an explicit constructor invocation (super(..) or this(..)). The statements cannot use the instance under construction, but can initialize its fields, which makes classes more reliable when methods are overridden

omg I could yeet my anonymous block hacks if this gets in

[slow prism]

html moment

[fallen igloo]

ffs java why do you add that stupid shit and and not better generics

[boreal flint]

yeah this one is the real gold

convenience features like switch exceptions and stuff is all cool

but being able to put stuff before a constructor's super/this is big news

[slow prism]

shut up and go somewhere else to complain about good features

[shut wraith]

In all seriousness, because better generics requires reworking a lot more stuff than something that is mostly just fancy syntax sugar

Though I too wish for improved generics

[slow prism]

I suppose more technical changes should go in the blog post instead of a primer (i.e removing antlr or the fml locator changes)

[elder swallow]

I only put the most important things in the blog post

[slow prism]

well we should probably mention this stuff too somewhere

[blazing valve]

I doubt our readers will be excited to hear about antlr

[shut wraith]

Next thing we know, "why did that shaded antlr class I was using dissapear?"

(Hell, did AT shade antlr or just drag it in normally? I don't even remember)

But yeah, those sort of changes are interesting but not nearly as vital to note -- the JST AT change might be worth noting

[blazing valve]

No one is gonna read until that point to get that though

there is such a thing as TMI 😄

And yes, the AT thing may be user-visible indeed

[boreal flint]

JST AT?

do I need to get ready for something AT related lol

[prisma locust]

the JST AT change is retroactive though, isn't it?

[boreal flint]

I got like 160 of em

[blazing valve]

ATs are applied to the decompiled minecraft sources instead of the binary

meaning it is no longer decompiled again when you change ATs

[boreal flint]

ah nice

is that a ng thing? or a 1.20.5 thing?

[blazing valve]

embedded is right, it's NG

[shut wraith]

Uh, was the issue with subclasses ever resolved?

Like, how does JST handle "AT applied to superclass but not subclass"?

Since that won't recompile properly, in theory

[blazing valve]

Hm?

That's been the case previously

So it was left as-is

You can write an AT that leads to a recompile issue today

[boreal flint]

getJumpPower -.-

[shut wraith]

Ah wait, right, cause they're applied before recomp. Make sense

[slow prism]

yws

[shut wraith]

So yeah the modder-facing AT changes in the 1.20.5 update itself are minor enough they're probably not worth mentioning

[boreal flint]

what are they though

[fallen igloo]

uhm what happened to the event result refactor

[blazing valve]

there are none

(modder-facing AT changes)

[shut wraith]

Basically: if for some reason you depended on the antlr runtime that AT bundled (why would you ever do this), it is gone, because AT no longer uses antlr.

But no, nothing about ATs themselves should have changed

[boreal flint]

yeah alright

[elder swallow]

Shadows still needs to finish it

but he's been busy with networking until yesterday ^^

[misty crow]

Speaking of networking: @true jacinth I'm looking into doing the "staticification" of PacketDistributor and with the way I have it currently, the equivalent of PacketTarget#send() creates exactly one packet and "dispatches" it. Would you prefer something along the lines of the previous solution with a Consumer<Packet<?>> or would you prefer a solution where the "equivalent" of PacketTarget#send() returns the packet and lets the caller pass it further, avoiding the lambda soup (almost) entirely?

[elder swallow]

do we need PacketTarget at all?

[true jacinth]

I don't think PacketTarget is necessary

it was necessary to passthrough the lambda to SimpleChannel

but that purpose is defunct

[misty crow]

No, that's gone in my current attempt, I just meant "the effective equivalent of that method"

[true jacinth]

Let me mock something up real quick

[elder swallow]

btw, for serverbound packets you can't do much except a basic "send to server"

[misty crow]

Yup, that's the reason why my PacketTarget#send() equivalent only creates one packet, since it's only used for clientbound packets

[misty crow]

Realistically I only need a decision whether you would prefer sendToClient(player.connection::send, payloads); (closer to the old approach) or player.connection.send(makeClientboundPacket(payloads)); (avoids the lambda soup [almost] entirely)

[elder swallow]

why not sendToClient(ServerPlayer, payload)?

or potentially just player.send(payload) which we might already support

[misty crow]

Most of the multiple-player targets hide the individual player in vanilla code

[true jacinth]

I think the second one here is more preferable

[misty crow]

Sounds good, then I'll go for that

[slow prism]

also, what do we do with the interaction events

I'm tempted to rework them in 1.20.5, but I'm not sure what events should be kept, and which not, since we have like 3 events firing for block place

[misty crow]

The only thing I have to add to that topic is that we need an event between Block#useItemOn() and Block#useWithoutItem() so event-based double door mods can finally stop breaking camo application on FramedBlocks doors (not blaming anyone in particular, the way item-on-block interaction worked before Mojang separated it into those two methods just sucked for that kind of feature combination)

[elder swallow]

didn't we accept a Commoble PR that added finer-grained events?

[true jacinth]

UseItemOnBlockEvent exists now ye

[misty crow]

Yes, Commoble added UseItemOnBlockEvent, which might be cleanly adaptable for that (I haven't checked yet). It cannot provide the necessary distinction between "using item on block" and "using block empty handed or block didn't care about item" in previous versions though

On a completely different note: server shutdown currently gets stuck on sun.nio.ch.WEpoll.wait() on the Netty Server IO thread. Not sure what's going wrong there

[elder swallow]

that sounds wrong?

yeah something is wrong with datagen too

could it be that I made changes to the FML executors and forgot to mark some threads as daemons?

[misty crow]

Potentially, yeah

[elder swallow]

yeah I think it's the new modloading-sync-worker

[misty crow]

I'll look further into it after I've got the distributor PR done

[elder swallow]

let me validate that hypothesis real quick...

yeah that was it

https://github.com/neoforged/FancyModLoader/pull/117

[digital drum]

what's the alt for Player.MAX_NAME_LENGTH in the snapshots?

[summer furnace]

that value was only used in a method called from GameProfileCache.lookupGameProfile()
if you look at this method in the snapshots, instead of calling Player.isValidUsername, it calls StringUtil.isValidPlayerName

[elder swallow]

giga how do you know that? 😄

[summer furnace]

?

I looked at the code?

sorry I should have specified

the value is only used once in the Player class, in the isValidUsername

this method was only used in GameProfileCache

given that I can't know of other usages of the constant, this is the only breadcrumb trail we can follow

oh, crap, StringUtil.isValidPlayerName doesn't have a constant for it

SharedConstants.MAX_PLAYER_NAME_LENGTH seems likely

[digital drum]

Could we add the ability to "copy over" dcomponents?

between stacks (item and fluid) to let's say BEs

[elder swallow]

that's already supported - see implicit components

[fallen igloo]

I just had an idea how we can "fix" vanilla compat for item components better

if the client is not neoforge we save the component to the nbt in the custom data component and skip the custom one

(we can do this by patching the stream codec for DataComponentPatch)

[blazing valve]

Hm? How would you identify a vanilla client in the stream codec?

[wooden tendon]

You can't

[fallen igloo]

hmmm good point

I at vanilla compat getting more difficult

[wooden tendon]

The solution is trivial

If there are custom components

Then no vanilla clients

Simple as that

[fallen igloo]

well yes but we currently need to do something like for milk fluid because of the neo added ones

[wooden tendon]

Yep

[elder swallow]

Actually we don't

There can be additional non-vanilla registry entries as long as they are not used

[fallen igloo]

nope because we need to do reg sync

[elder swallow]

which we don't do when connecting to vanilla clients

[prisma locust]

so a vanilla Neo client won't be able to connect but a vanilla client will?

[blazing valve]

Uh vanilla neo client?

[elder swallow]

what is a vanilla neo client?

[blazing valve]

You mean if it's missing the mod ?

[prisma locust]

a neo client with no mods

[elder swallow]

connect to what?

you can connect two neo clients with no mods

since they both add extra registry entries

[prisma locust]

it sounded like Schurli wanted to make modded server-side item components possible?

[elder swallow]

right now we could register normal item components

and it wouldn't break neo-vanilla connections

(as long as nothing uses these components, of course)

[elder swallow]

this is quite easy to do

you just need a ByteBuf subclass (think RegistryOps)

[elder swallow]

I have considered it - this is basically how item stack attachment sync works in 1.20.4; but I don't think it's worth it