Registries Overhaul

[heavy zephyr]

Although yeah I'd have expected a modify event to fire before the registry is actually built

[cerulean oxide]

ObserveRegistryEvent?

[hoary umbra]

The naming of events is conventionally based on when the event is firing

This one is.. more or less RegistryCreationEvent?

[heavy zephyr]

there is no proper convention

look at RegisterEvent, the event to register stuff

[cerulean oxide]

how does one even review a PR of this size

[hoary umbra]

thats what I said

[cerulean oxide]

I can only really check the few things I'm personally familiar with

[heavy zephyr]

I went through every single file lol

[cobalt cave]

Well, Fabric does currently still do the loot table part:
https://github.com/FabricMC/fabric/blob/1.20.2/fabric-registry-sync-v0/src/main/java/net/fabricmc/fabric/impl/registry/sync/trackers/vanilla/BlockInitTracker.java#L48

[heavy zephyr]

last commit of https://github.com/FabricMC/fabric/pull/194

wtf really

[pallid quarry]

Probably that this was an issue back on 1.14 then, but not on recent versions?

[heavy zephyr]

this is the 1.14.4 code (thanks snowman)

[gloomy sinew]

does that RL really need to be cached

[heavy zephyr]

could be https://github.com/FabricMC/fabric/issues/210

[pallid quarry]

That issue is after the PR was created though.

[heavy zephyr]

but right before https://github.com/FabricMC/fabric/pull/194/commits/c6f6da87368c304f16577a92c9be8a568668660b

hmmm not actually the object.getDropTableId(); call was already there before the PR!!!

meh this dates all the way back to the very first implementation of fabric registry hooks

whatever

we'll fix it if it causes an issue in the future

[heavy zephyr]

wrote a blog post: https://pr-14.neoforged-website-previews.pages.dev/news/20.2registry-rework/

[heavy zephyr]

FYI

yeah vanilla does this... I was looking at the neoforge project

we should probably add this in an AddCallback

[lofty citrus]

[heavy zephyr]

[stuck lava]

question... do callbacks work for datapack registries?

[heavy zephyr]

good question

the answer is no because the modify event is not even called for them??

[stuck lava]

the getLootTable call should be in the bake callback and not the add callback

[heavy zephyr]

why? does it change?

[stuck lava]

the vanilla behaviour is in the loop so we should also do it like that

[heavy zephyr]

there are multiple bakes but a single registration, so no

vanilla doesn't have callbacks so they probably do it like that because it's the most convenient

[stuck lava]

is that so? shouldn't we then move the initCache to the add callback

[heavy zephyr]

initCache should definitely not be in bake at the very least due to its performance

[heavy zephyr]

the add callback doesn't fire for vanilla entries, that's why I added the static block in Blocks back

(and it's also good to keep the static block for discoverability because everyone forgot about it... :P)

[cerulean oxide]

We need to call initCache in add then

If vanilla does it in the static block

[heavy zephyr]

did that already

[cerulean oxide]

It's still a waste because vanilla rebuilds all the caches again when you join a server

But there is nothing we can do about that without integrating ModernFix patches, because it's how vanilla is designed

[heavy zephyr]

I checked and if you do Blocks.WATER.defaultBlockState().getFluidState() in a common setup handler you get an empty fluid state without that initShape call 😛

[cerulean oxide]

Correct

[ripe storm]

When does vanilla initialize states?

Oh, statically

[heavy zephyr]

we could probably do it in parallel if we wanted to

right now the bottleneck is still block creation I think

[ripe storm]

Shape setup, I believe, yeah

[cerulean oxide]

you can't, things explode

I tried it in modernfix a long time ago

better to do it lazily

[heavy zephyr]

most unfortunate

[stuck lava]

maty do the rest of the registry stuff so we can finish this

[keen thicket]

i'm busy

[stuck lava]

yes with the registry rework

[heavy zephyr]

what is the rest?

the only open discussions are the prefix in patches and the modify registry thread I just opened

[keen thicket]

https://github.com/neoforged/NeoForge/pull/257#discussion_r1392785646 the solution is to fix and fire it

i probably just missed the part where shrimp fired it (since not sure if anyone noticed but most of this is just shrimp's stuff copied over to 1.20.2)

[cerulean oxide]

Why should we fire it on datapack registries

Do we allow appending to those in code?

[heavy zephyr]

no, datapack only

[keen thicket]

bake for example can be useful for caching of values of dp regs. same as with any registry

[heavy zephyr]

you can probably do in that in one of the server lifecycle events

[gloomy sinew]

I don't think we should, what's the use case?

[keen thicket]

you would need a mix of listeners, and when dp regs can be reloaded it will break

[gloomy sinew]

yeah but you can already do that

[heavy zephyr]

I don't care about a hypothetical future 😛

[cerulean oxide]

You can already do this by computing on first access and then using a reload listener to invalidate your cache

[stuck lava]

I would like to use bake to validate the entries because I can't do that in the codec because it is dependent on itself

[gloomy sinew]

dpregs don't reload on /reload

they're finalized just before the server starts starting

[heavy zephyr]

it would be better to have a DataPackRegistriesUpdatedEvent or whatever IMO

[gloomy sinew]

why?

[heavy zephyr]

than support modifying datapack registries

[gloomy sinew]

ServerAboutToStartEvent already works for that

[heavy zephyr]

right

[cerulean oxide]

.... what is the point of them being datapack-driven then

[heavy zephyr]

add one of the client join events and you're good

[cerulean oxide]

I thought the whole magic was that they could be reloaded

[heavy zephyr]

they reload on server restart

[cerulean oxide]

mods could do this a decade ago

[past saffron]

If anything, it has to be called in bake. Doing it in add would defeat the supplierization of dropsLike(Block)/lootFrom(Supplier<Block>)

[cerulean oxide]

[cerulean oxide]

Ah, true

[heavy zephyr]

[gloomy sinew]

anyway dpregs don't need registration callbacks because you can cache your derived data when the server starts or lazily evaluate them on first use

[keen thicket]

#1170668658813575230 message I digress :P

[hard coyote]

technici4n

[Reference to](#1170668658813575230 message) #1170668658813575230 [➤ ](#1170668658813575230 message)it's not the worst thing, but it would be good to slowly start thinking about separating client-exclusive code properly instead of relying on lazy classloading

matyrobbrt referenced · Nov 14, 2023, 10:50 AM

[heavy zephyr]

it's not hypothetical, fabric already did it and it's in our hands

😛

[cerulean oxide]

It's fine, the method has effectively zero performance impact on the second call

[heavy zephyr]

I know, it's just stupid

[gloomy sinew]

I'm not hypothetical either, I already cache dpreg stuff without registry callback events https://github.com/Commoble/exmachina_engine/blob/main/src/main/java/commoble/exmachina/engine/ExMachinaEngine.java#L82

[cerulean folio]

It is really not!

[heavy zephyr]

it is "given sufficient effort"

which we are not gonna do now

[stuck lava]

what about my usecase?

[heavy zephyr]

but it is technically possible, is what I mean

[heavy zephyr]

hmmm?

you can do it ServerAboutToStartEvent

[cerulean folio]

It requires us moving away from source patches....

Which is like never going to happen

[heavy zephyr]

not at all

whatever, it is off-topic 😄

[stuck lava]

that seems like the wrong place to do it

because if mojang makes dpregs reloadable we have a problem

[heavy zephyr]

please, let's worry about that when it happens

[gloomy sinew]

yeah I literally use server start events to validate the big ol' glob of data I derive from my dpreg stuff

I'd like to do it in the codec itself but A) I can't and B) a registry callback wouldn't help that anyway

and if mojang did make dpregs reloadable I'd just validate-and-cache my data in the /reload event instead

[keen thicket]

a reg callback would help?

[gloomy sinew]

why would it

err

[keen thicket]

how would a reg callback not help with validation of big ol' globs of data you derive from your dpreg stuff

[gloomy sinew]

I mean it wouldn't help me with doing validation inside the codecs

as for other things it could potentially help me with, I'm not currently able to conceive of anything it would let me do that I'm not already doing

[ripe storm]

Are people debating not firing the registry event to attach listeners and whatnot for datapacks dpregs? IMO datapack regs and normal regs should be treated the same in every respect, except for letting people register to them (that is, firing register events). The events to attach callbacks should still fire for them. Otherwise it's an arbitrary restriction - you can't know everyone's use case, so unless there's a very good reason to limit it (not just "someone could misuse this"; that's true of everything) you shouldn't place arbitrary limitations on the API

Arbitrary restrictions on an API aren't cool

(unless there's a large maintenance burden or such accompanying them of course, or it's a substantially high ease of causing bad states that are difficult to diagnose without it)

[heavy zephyr]

it needs a number of new injection points yes, frankly I don't feel like researching that and I doubt it was already working before

we are already at version 20.2.96-beta-reg-rework

@stuck lava do we want to target this weekend for the reg rework?

(to give more time for review)

[stuck lava]

I'd say Saturday as a target

[lofty citrus]

to whoever does the final merge: don't forget to keep the co-authors

[heavy zephyr]

whoops

that's my bad for the 1.20.2 port 🙃

<@&1128776937410670663> the registry rework is targeted for this weekend, please try to review it beforehand

@stuck lava check the blog post draft: https://pr-14.neoforged-website-previews.pages.dev/news/20.2registry-rework/

[stuck lava]

I'd add a registerBlockItem example in the section for the specialised DRs

I think the Custom registries section is outdated

[lusty pendant]

Minor thing, but some of the tests have changed from using .get() to .value() (with the change to DeferredHolders) any reason for that? (ForgeSpawnEggItemTest for example)

[heavy zephyr]

@stuck lava we have problems

[heavy zephyr]

probably because they were changed to Holder first and then back to DeferredHolder?

[keen thicket]

hmm, can we get a HolderLike instead of Supplier. and said HolderLike would have value<T>

that way we can solve all of the ambiguïtés

ah yes, french autocorrect

[lusty pendant]

Oh wait I'm dumb, it's the references to the Attributes in NeoForgeMod which are Holders

[heavy zephyr]

ah alright

[heavy zephyr]

it doesn't solve anything, the problem we have is that DeferredHolder<R, T> is only Holder<R> but we want a Supplier<T>

so we thought it best to move implements Supplier<T> to DeferredHolder instead of Holder

maybe moving the supplier back to DeferredHolder is the cleanest

[keen thicket]

should we make an announcement about the "RFC period" until sunday

[lusty pendant]

Likely a good idea to do so

[ripe storm]

I'd say people should use ::get if they want a supplier of the specific type, or heck, a supplier at all, and call it a day, but that's just my opinion

[heavy zephyr]

Holder implementing Supplier is convenient for all the methods that would otherwise need two overloads

and yeah I think ::get is fine

[stuck lava]

I think the less ::get we need the better

[heavy zephyr]

I agree it's not great

[lofty citrus]

someone should update https://github.com/neoforged/NeoForge/issues/37 on the state of things too, as a general overview of what the rework entails

[heavy zephyr]

the general overview is the WIP blogpost

[lofty citrus]

assigns you to the issue

[heavy zephyr]

do I delete this? it's not used anymore

[gloomy sinew]

who originally added it

[heavy zephyr]

@atomic latch hello there

answer the question please 😉

[stuck lava]

for the supplier stuff please don't move it back to Holder because all of the suppliers (neo)forge adds require a more specific type and not a Supplier<BaseType> this is most annoying for entity types in spawneggs

[heavy zephyr]

you can still do ::get

or in this case I suppose we can just remove the Holder methods

but it's annoying, it means that if you have a normal holder you can't use it in supplier methods anymore

[gloomy sinew]

Why do we need a whole pair of deferredregister subclasses for blocks and items?

you can defreg a block+blockitem with a few lines in one static method helper

[heavy zephyr]

the answer is 1) to guarantee that custom DR and DH subclasses are possible 2) it adds like 5% of convenience and 3) why not

I wasn't also particularly enthusiastic but it doesn't hurt /shrug

[gloomy sinew]

like I use this and it does the same thing in so much less code ```java
private static <BLOCK extends Block, ITEM extends BlockItem> RegistryObject<BLOCK> registerBlockItem(
DeferredRegister<Block> blocks,
DeferredRegister<Item> items,
String name,
Supplier<? extends BLOCK> blockFactory,
Function<? super BLOCK, ? extends ITEM> itemFactory)
{
RegistryObject<BLOCK> block = blocks.register(name, blockFactory);
items.register(name, () -> itemFactory.apply(block.get()));
return block;
}

private static <BLOCK extends Block> RegistryObject<BLOCK> registerBlockItem(
    DeferredRegister<Block> blocks,
    DeferredRegister<Item> items,
    String name,
    Supplier<? extends BLOCK> blockFactory)
{
    return registerBlockItem(blocks, items, name, blockFactory, block -> new BlockItem(block, new Item.Properties()));
}

[lusty pendant]

tbh I'm not too big of a fan of it either

[heavy zephyr]

tbh I don't think ITEMS.registerBlockItem("name", FANCY_BLOCK::get) is that bad

[keen thicket]

i hate this all value and get tbh, but meh

[heavy zephyr]

wdym?

[keen thicket]

there's holder with value()R and DH with get()T from supplier

[heavy zephyr]

supplier is our "patch"

value()R will stay whatever we do (but we can override it to return T)

you want value()T?

[keen thicket]

yes i think we should get rid of supplier and have a custom FI with just value()T, and we can replace all of our patches of Supplier to that custom FI

and that way we don't need to ::get or ::value

[stuck lava]

should we replace all of the suppliers with a custom ValueProvider that has a value() method so everything uses it?

[keen thicket]

get hecked

[heavy zephyr]

is Holder gonna implement ValueProvider?

if yes then we're back to the same problem

[gloomy sinew]

I don't think it's important for holder to implement supplier

[atomic latch]

This is meant to be a hook for mods to be able to do filesystem changes (like backups) if the mod version changes

[stuck lava]

can we not change some of our patches to use Holder instead of Supplier?

[atomic latch]

If there's something that can provide that super early in the world loading process, this can be scrapped, but if not..

[heavy zephyr]

we are not tracking mod versions in the world file anymore at the moment (in the rework) because no registry needs to be persisted anymore

[atomic latch]

Because I use it when I do SavedData changes - there's no other way to prep those files

[heavy zephyr]

can't you add a version field at the beginning of your SavedData file?

[atomic latch]

Problem ran deeper than that - I had to sync multiple files in unison

So I had to rewrite portions without MC's systems

[cerulean oxide]

interesting. so the warning about "mod version changed" will be gone?

[heavy zephyr]

I suppose so, yes

[atomic latch]

I think removing any notification on mod version changes is gonna annoy people. A lot.

[cerulean oxide]

I think the rework removes the subsystem Forge used for storing that data entirely

[atomic latch]

"why'd my shit break? I only went from version 3 to 298?1?! One"

MME gives people an event to at least do their own logging and prep work

[cerulean oxide]

is the important part the timing of the event, because we could add back an event that fires at a similar time, but require people to track their data versions themselves

[atomic latch]

If you MUST get rid of it, please add a hook early in the world load process that has context, yes

[cerulean oxide]

that patch would be significantly less complex than having to actually store anything

[atomic latch]

I can take it from there, but otherwise there's no way in without mixins

People will have to store their own versions, but that's not necessarily a bad thing

[heavy zephyr]

in theory the code to store mod versions shouldn't be too bad?

[cerulean oxide]

mostly just putting back these functions and only doing the mod version part, not the registries

apparently the link doesn't function

great

the additional saved data methods in CommonHooks

[lusty pendant]

it only functions if GH chooses to open the file you linked to

[keen thicket]

you're right, damn it

[cerulean oxide]

why are we patching things into Holder actually

[keen thicket]

wait

[cerulean oxide]

I thought Holder already had a concept of deferral

[heavy zephyr]

it's not a supplier unfortunately

[keen thicket]

hmm no nvm

[cerulean oxide]

is that a problem

[keen thicket]

can we not do any magic to get the compiler not to complain and to inject a bridge

[heavy zephyr]

let's not

[heavy zephyr]

don't think so

can't we just let people do ::value if they want a supplier from a holder? and call it a day?

[keen thicket]

honestly i wouldn't even recommend storing Holder<T> even when it would be DH<T, T>

you're downcasting DH extends Supplier into Holder and then from that creating another supplier

[stuck lava]

yea we'll just remove the holder methods from the special dr

[heavy zephyr]

yeah I don't think storing a Holder makes sense if it doesn't even implement Supplier anymore

[keen thicket]

and y'all made me make all DH<T, T> into Holder<T> in neoforgemod

i want to be reimbursed for that wasted time

[heavy zephyr]

[stuck lava]

And me in the testmods

[keen thicket]

where's our money

@heavy zephyr i'll send you the bill smh

[stuck lava]

I'll send you my paypal maty

[hoary umbra]

No, it doesn't, the vanilla holder impls must be provided by the registry and are only provided once the object is registered

[keen thicket]

that's not necessarily true getOrCreateHolderOrThrow is a thing

[gloomy sinew]

vanilla holders can exist before the object exists (standalone reference holders are created with an id and have the value bound later), however they cannot exist before the registry exists

DeferredHolder exists to solve the latter, because of custom registries

[keen thicket]

well actually no, custom registries are created immediately

[gloomy sinew]

oh, neat

that's different

[keen thicket]

it exists because

1. all holders must be bound, so you can't have them "optional"
2. vanilla is annoying and you basically can't have Holder<MyBlock> hence the double generics of DH

[weary spear]

So if you use a regular holder you cannot have a type extending your registry type?

Then you would need defferedHolder?

[heavy zephyr]

yes

[weary spear]

So then why would I use holder?

Is that faster or something?

[heavy zephyr]

you probably don't need to use holder

@keen thicket should I update the blog post to remove the Holder example?

(so the choice will be between DeferredHolder and Supplier)

[weary spear]

Why would I use the deferredHolder over the supplier then?

I guess if you want the key but I don’t see why that would be needed since you are accessing it already then

[heavy zephyr]

because of these methods

if you need them you can have them, otherwise just Supplier is fine

[weary spear]

Oh fair

Wait holder is not a supplier like registryobject is?

That’s a bit annoying

Would it be possible even to convert a holder to a supplier?

[gloomy sinew]

Holder::value is a supplier

[keen thicket]

yes

[weary spear]

That works if you have a Supplier as parameter?

[gloomy sinew]

but yeah holder is essentially a mojang-flavored registryobject

[cerulean oxide]

it should because supplier is a functional interface

[keen thicket]

Holder::value is a Supplier<BASE>

[weary spear]

Oh I never knew that it worked like that

Thought it wanted the Supplier class specifically

[gloomy sinew]

supplier is a functional interface

[weary spear]

Any method that follows the supplier principle works?

[weary spear]

Well I know but I thought you needed specifically that implementation

Makes sense tho

Cuz you can also use lambdas

And that’s also not explicit

[gloomy sinew]

any method that matches supplier#get's method signature can be made into a method reference impl of supplier

[weary spear]

Ye

Ive been using that for like ever now that I think about it XD

Just a little brainfart

🤣

It’s a bit weird that they are optional tho

[ripe storm]

Custom registries are created immediately but holders may be made before them, by other mods

[ripe storm]

Holder is still worthwhile if you don't care about the specific type, so I'd keep it

[keen thicket]

it's not really no, DH extends Holder and more than that you can't use a holder directly in forge patches of ctors anyways

[ripe storm]

those forge patches or ctors should be fixed. But fair I guess? I don't keep track of the types of stuff I register so the ability to just yeet that type entirely is useful

Though ::value exists so no big deal

[keen thicket]

fixed as in how

a lot of stuff wants suppliers of specific types, like fluidblock, or entitytype<T>

and holders can't do that so there's no way the patches can be changed to a holder

[hoary umbra]

Simple™️
just patch everything that uses holder

[ripe storm]

ah right, that's not an issue for vanilla as it doesn't use holders there but just the block or entity type directly. I was thinking more anywhere expecting a Supplier<? extends Item>` or similar

But that's a good point. I'd still say there's plenty of reason to use just plain old Holder, so I wouldn't remove that example from the blog post

[hoary umbra]

I just wish I could convince someone at mojang to switch all the holder refs to <? extends T>

[ripe storm]

It's not how holders are meant to work from what I can tell. The idea is that if you have a holder, all you know is that it contains a member of the registry type - like, think in terms of making stuff (maybe eventually blocks) reloadable - holders could theoretically have the specific type they contain replaced without issue, because you can't depend on that specific type as all you have is the generic

[hoary umbra]

Ultimately you will need to have some guarantee that some particular object is known to be of a specific type, which is currently accomplished by vanilla's static fields - sure the holder generic doesn't guarantee that the type is of type T (rather than R), but this isn't an issue in practice (nor has it been since the introduction of RO)

Its a classic type safety vs usability argument

[ripe storm]

Eh, that's actually not needed that much in vanilla. There's a few places where it does but generally speaking they've done a pretty good job of yeeting dependence on the specific type

[hoary umbra]

Another thing here is that typically the only instance of a strongly-typed holder should be on instantiation, so the instantiator should know the type

[ripe storm]

It just pops up far more in modded

[hoary umbra]

Vanilla also has the luxury of adding new methods to Block to erase type-specific dependencies

[ripe storm]

Yep, definitely. I agree that it's definitely useful for modding (though still possible to avoid in many cases), I just unfortunately doubt vanilla will be switching stuff around to allow that as they've put a good bit of work into not needing that

[past saffron]

Does the registry add callback also fire for vanilla entries?

[heavy zephyr]

No

Because the ModifyRegistryEvent to register it fires after bootstrap

[past saffron]

Then we need to add back the loot table resolution in the static block in Blocks, it currently gets patched out

[keen thicket]

imho we should fire modifyregistryevent in the block ctor

registry ctor*

[past saffron]

How's that supposed to work? The event would fire for vanilla registries long before mods start loading

[keen thicket]

uhh

you have a point

[warped forge]

new task: move mod construction to before vanilla bootstrap

(that wouldn't break anything, right? right?)

[atomic latch]

Sure, let's break reality for a while

[warped forge]

the funny thing is, if mod construction and initialization happened before vanilla has even been added to the jvm, it would mean people physically cannot use static fields to register stuff and DR/Events would be required :P

[woeful mountain]

go make a PoC

[warped forge]

I don't have the time, energy, or patience XD

[heavy zephyr]

Is it still patched out? I thought I restored it

[heavy zephyr]

No we're not gonna do that for performance reasons

[ripe storm]

Quilt sorta does this by shoving it basically into a partially bootstrapped but unbaked state, and it works well enough. But it's not truly before bootstrap either. If you were before bootstrap you'd have to be very careful that mods didn't, like, classload the wrong thing and have everything crash and burn

[warped forge]

to be perfectly clear: it was a joke.

[heavy zephyr]

Quilt doesn't have parallel mod loading though

[past saffron]

On second thought, I may be wrong and just misreading the patch

[heavy zephyr]

Did you add all the comments tonight?

[past saffron]

Yes, the four comments are from 2 hours ago

[heavy zephyr]

Or were you looking at the code some time ago?

[ripe storm]

Yeah, wherever you decided to move the mod ctor injection point, you'd still need DR if you wanted parallel mod loading

[winged iris]

maty your description says it isn't going to make the DH change but like it is doing even more than the other one did in that regard by moving over rather than just deprecating

[winged iris]

well I will get back to trying to test this later... for some reason when building against it with it in mavenlocal... NG7 doesn't seem to be attaching the mc dep to my project?

[winged iris]

uhh what is the way to get the tags from a registry object now that the tag manager and reverse tag system are gone?

or is there no longer a good mostly cached way

[past saffron]

Holder#tags(), which the IReverseTag implementation directly referred to anyway

[winged iris]

ahh so need to get a holder, okay. Feels a bit odd to look up the holder for the object itself but thats fine

[past saffron]

The tag manager did nothing else

[winged iris]

shhh

that is just details

[keen thicket]

@heavy zephyr fix it

[winged iris]

well that's enough for tonight... have a local branch of mek that compiles against the rework but crashes when running due to NeoForgeRegistryCallbacks$BlockCallbacks.onAdd but too tired to look into why

[lofty citrus]

gimme log

oh wait. that was hours ago

still, gimme log

[keen thicket]

gibim log

[heavy zephyr]

so, what do we do about the conflict between Holder and Supplier?

Holder contains the ID, which is nicer for e.g. registerBlockItem

IMO: we should use Supplier unless we have a reason to prefer Holder, what do you think?

so we'd have registerBlockItem(String, Supplier) or registerBlockItem(Holder)

(but NO registerBlockItem(String, Holder))

@stuck lava these overloads will only give us problems

[ripe storm]

This makes sense - if you're storing a reference somewhere always use holder though, I'd say

But yeah that makes a lot of sense

[heavy zephyr]

Holder doesn't have the specific type

regex fun!

let's see how many generic conflicts this will cause...

[ripe storm]

Yeah, so if the specific type is needed, or no reference is stored, use supplier. Otherwise, if a reference is stored, or an ID is needed, use holder - maybe with an overload taking a supplier and RL if it makes sense

[heavy zephyr]

registerBlockItem is so convenient for the forge testmods... 😄

[heavy zephyr]

oh ffs Holder doesn't know its own ID, only DH knows it

actually no, unwrapKey().get() will work

        public DeferredItem<BlockItem> registerBlockItem(Holder<Block> block, Item.Properties properties) {
            return this.registerBlockItem(block.unwrapKey().get().location().getPath(), block::value, properties);
        }

not great, not terrible

[lofty citrus]

DeferredItem

[heavy zephyr]

missing two overloads for registerBlock

[lofty citrus]

for a second, I read "that implements ItemLike" as referring to "Items", rather than "Special DeferredHolder"

[stuck lava]

actually it does not contain the id because it is not a Holder.Reference necessarily

[stuck lava]

remove the supplier overloads if there is no specific reason to have them (always prefer Holder)

[heavy zephyr]

we should prefer Supplier, IMO

[stuck lava]

why?

[heavy zephyr]

cause it's a more general construct

a Holder<T> is basically a Supplier<T> (without the specific type) + more info

idk, we should decide what modders are supposed to use

[stuck lava]

in the case of creative tab we should remove the supplier ones

[heavy zephyr]

already did

I removed the Supplier<? extends ItemLike> overloads

the ItemLike overloads are still there

[stuck lava]

yea the ItemLike ones are vanilla so that is correct

[heavy zephyr]

for registerBlockItem I suggest keeping these only:

if you have (String, Holder) you can easily use ::value to get a supplier, but if you have a Supplier you cannot easily call a holder method

another inconsistency is that there are 4 registerItem overloads:

but only 2 registerBlock overloads:

should we add the other two overloads?

[lofty citrus]

here's a thought: why does vanilla not implement Supplier onto Holder

[heavy zephyr]

no idea, Forge used to patch that in, but we removed it in the rework in favor of having DeferredHolder<R, T> implement Supplier<T> (Holder would have been Supplier<R>)

[keen thicket]

registerBlock(String) doesn't really make sense without properties

modders shouldn't construct blocks without any property configured

[stuck lava]

do default properties for blocks make sense?

[lofty citrus]

I think blocks and items should always have their properties passed along, and never constructed in NF code on the modder's behalf

[keen thicket]

imo default new Item.Properties() makes sense, especially for block items

you rarely actually have to configure item properties

[lofty citrus]

does vanilla provide an Item constructor that doesn't have a Properties parameter?

[heavy zephyr]

we are not vanilla 😛

every single BlockItem in the testmods had no properties

(probably at least 20 call sites)

[lofty citrus]

we may not be vanilla, but that doesn't mean we should necessarily be different from vanilla in every aspect

[heavy zephyr]

yeah I agree, that is a lot rarer than needing to construct items with no properties

blocks usually have a sound group or map color or something else even if they don't have any behavior

[keen thicket]

strength is basically required

[heavy zephyr]

yeah that too

[lofty citrus]

I do see that vanilla-Items has a helper for registering blocks that's just the blank Item.Properties, so I'm revising my opinion to allow that no-properties item factory

(which btw I would think is a more constructive reason than just the basis of "not being vanilla")

[heavy zephyr]

sure - there's multiple ways one can be convinced 😛

[lofty citrus]

i believe "we're not vanilla" is particularly weak without supporting or derived reasons
by itself, it's just a facsimile of NIH syndome -- denying outside (in this case, vanilla) ideas in favor of orur own

and the NF test mods are hardly representative of the common MC mod

[heavy zephyr]

I used that argument against the "we shouldn't do this because vanilla doesn't do this" argument

[lofty citrus]

I was thinking more along the lines of "making the caller always aware of the properties of their item, in the same fashion that Items (and subclases) always require their properties being supplied"

[winged iris]

In places I have overloads that take a UnaryOperator parameter

[past saffron]

I would prefer using Holder#unwrapKey().orElseThrow() with a sensible exception message mentioning that unbound reference holders and direct holders cannot be used. Having it run into a non-descript NoSuchElementException("No value present") is awful for debugging

[lofty citrus]

I would assume the reason why Item and its subclasses always require an Item.Properties instance is is so the properties of any item is clearly visible at the item's creation point, rather than hidden inside the constructors of Item subclasses

[heavy zephyr]

you'd still get a stacktrace pointing to the registerBlockItem call

[lofty citrus]

which I then wanted to apply here, so modders calling registerItem know exactly (by virtue of needing to pass in the Item.Properties) what their item's properties are

[heavy zephyr]

I'll change it after I restore the mismatch event

[lofty citrus]

actually, this brings up a thought:
the above cited cases were for block items, which usually don't need to have specific item counts or other such info, which is a common enough usecase (as Item#registerBlock demonstrates)
but does that same logic apply for raw, basic Item instances? (no properties, no subclass -- just an new Item(new Item.Properties()))

[winged iris]

I will later, but tldr was me checking if an item was held in my getShape return (because of the block callbacks initializing the cache instantly and that being before items are registered). Though I think I fixed it by checking if it is a level and then only doing that client side (but need to evaluate it further to see if that is valid for my use case and also see if I should be overriding a different getShape

Though I did get errors in datagen about not being able to datagenerate for one of my custom datagen registries so I will need to look into that still

[lofty citrus]

i revise my opinion: yes for a block item factory which doesn't take in a Properties, nay for an item factory that doesn't take in a Properties

[stuck lava]

yes for basic crafting items

[heavy zephyr]

I mean this might be a problem with the rework

I can fix it quite easily

[winged iris]

I mean it might be both

as in it might be indiciative of a bug on my side as well

[heavy zephyr]

initializing the cache immediately is too earlu

[gloomy sinew]

I have this, so I can either register a basic blockitem or a special bunny blockitem ```java
private static <BLOCK extends Block, ITEM extends BlockItem> RegistryObject<BLOCK> registerBlockItem(
DeferredRegister<Block> blocks,
DeferredRegister<Item> items,
String name,
Supplier<? extends BLOCK> blockFactory,
Function<? super BLOCK, ? extends ITEM> itemFactory)
{
RegistryObject<BLOCK> block = blocks.register(name, blockFactory);
items.register(name, () -> itemFactory.apply(block.get()));
return block;
}

private static <BLOCK extends Block> RegistryObject<BLOCK> registerBlockItem(
    DeferredRegister<Block> blocks,
    DeferredRegister<Item> items,
    String name,
    Supplier<? extends BLOCK> blockFactory)
{
    return registerBlockItem(blocks, items, name, blockFactory, block -> new BlockItem(block, new Item.Properties()));
}

[lofty citrus]

are those common enough to merit having their own dedicated method?
I would personally think not, as the vast majority of items added by mods (in my opinion) add features to items that involve modifying its properties
(and in the minority of cases which are e.g. chock full of crafting items, chances are they've already made some sort of abstraction over their registration to ease having so many uniform items)

[gloomy sinew]

IMO norge shouldn't have the helpers for registering blockitems

[stuck lava]

I have 10 of them

I'd call everything > 5 common enough

[stuck lava]

why?

[gloomy sinew]

they're trivial to mods to write themselves and having them in norge itself foments bikeshed

[winged iris]

bigger thing is I don't think they have to be in neo for the initial registry rework release

[heavy zephyr]

consider that we already have them written and used everywhere by the test mods

it's only bikeshed if we let it be bikeshed

[winged iris]

ah

[past saffron]

Tech, you still missed something in the ModifyRegistryEvent docs 😄
It still mentions the DP reg event: https://github.com/neoforged/NeoForge/pull/257/files#diff-8fe9842091c045e7e1d0182809f68ca0fd5154724038d8096803c59912ead4d5R33
Is it intentional that the callback link doesn't have a "link text" to be displayed instead of the code reference itself: https://github.com/neoforged/NeoForge/pull/257/files#diff-8fe9842091c045e7e1d0182809f68ca0fd5154724038d8096803c59912ead4d5R24?

[heavy zephyr]

thanks

how many patches will this javadoc still need 😂

[past saffron]

[heavy zephyr]

alright the mismatch hooks seem to work

WTF

local diff after running unpackSourcePatches

[lusty pendant]

Think that's expected? We talked about it earlier

[past saffron]

That's my fault. I was reasonably sure that I had rebased the blending PR onto latest properly, but looks like I didn't: https://github.com/neoforged/NeoForge/pull/241/files

[heavy zephyr]

can someone open a PR to fix this?

this shouldn't go in the registry pr

[past saffron]

I can make a quick PR if you want to make a release for merely two patch headers. Though, I would prefer just including that in the larger patch cleanup PR I want to do later

[winged iris]

yeah making a release for two patch headers feels overkill

assuming it doesn't actually cause issues or bugs at runtime

[heavy zephyr]

as long as it's fixed in a reasonably related PR ¯_(ツ)_/¯

[heavy zephyr]

it just wastes the time of contributors every time they run unpackSourcePatches

[heavy zephyr]

turns out I already did this locally 😄

this is what I suggest for loot table and shape computation

[past saffron]

Looks reasonable to me

Though, I would describe it as "Init cache" instead of "Init shape" to avoid confusion because at the surface level it's not shape init, the actual shape init happens earlier when the block is constructed

[heavy zephyr]

yeah

question: why are the registries only frozen after common setup?

that sounds pretty stupid, they should be frozen right after the registry events

[past saffron]

Unfreeze and refreeze currently have dedicated ModLoadingStates and the freeze happens after ModStateProvider.COMPLETE, which means that the freeze happens after FMLLoadCompleteEvent. Breakpoints confirm this

[heavy zephyr]

indeed

but maybe the cache init should happen after common/client setup?

[past saffron]

Hmm, that's a tricky question. Doing it after common/sided setup allows computing additional stuff in said setup events which may be needed for the cache init. On the other hand, doing it earlier would make other computations in common/sided setup faster if those use stuff accelerated by the cache

[heavy zephyr]

hmmm, as long as the cache is not initialized blocks with a fluid state will for example return EMPTY for their fluid state

so we should probably initialize it early

(cause a variety of other things might depend on it)

[past saffron]

Yeah, that's true

[heavy zephyr]

why is FluidStack#fluid not final?

I'll make it final

[cerulean oxide]

can we document why it needs to be done this way instead of in onAdd

[heavy zephyr]

done

[winged iris]

well with this (plus me figuring out I accidentally removed the register deferred register for one of my registries) it seems like things are working. Was able to get into game with mek at least

[winged iris]

To mirror ItemStack we may want to add an overload FluidStack constructor that accepts a Holder<Fluid>

especially with the push the overhaul is making towards using Holders for things

[stuck lava]

that would be better in a fluid themed PR (the current fluid system is not bug free yet)

[heavy zephyr]

Yeah but that's gonna take some time 😛

Need to get caps well established before that can happen

[winged iris]

maybe, though this PR given how holder based isn't that far from being the wrong place for that addition. Hence why I was mentioning it

[heavy zephyr]

Yeah we could throw it in if it's just a few lines

[winged iris]

all that the itemstack version does

may want an Objects.requireNonNull around it

but basically would just be one (maybe two if also doing the nbt accepting one) overload constructors to just call value on the holder and pass to what already exists

why were the Tags.init methods removed? Is it that they aren't really forced to be loaded early anymore or what?

[gloomy sinew]

Tags are all loaded at the same time

I remember there was a time where some tags where loaded earlier than others, in vanilla

but that's not a thing anymore

[winged iris]

well this is about tagkeys

previously I think they were so that then they were validated as present?

in case nothing accesses the class before tags actually load

[gloomy sinew]

tagkeys don't do any validation of tags on construction

they're literally just three resourcelocations stapled together, and they're intended to be existable before tags do

[past saffron]

My guess is that the deterministic early init of the tag keys may have been needed for the default entry stuff (i.e. "optional" tags)

[winged iris]

ah right tag key creation isn't where the adding tag key to registry happens anymore

and nah it is that the declaration for tags a few tag impls ago used to basically mark it as a default/required tag (I think by interacting with the registry in some way. Sort of as if you called Registry#getOrCreateTag on them now)

[south mirage]

Not needed anymore

Used to be an issue. Long gone

Thank god

[winged iris]

nice

[winged iris]

do we want to allow mods to create intrusive holders for their custom registries? (if so it would require a way to pass the extra parameter to mapped registry)

[winged iris]

also uhh when trying to connect to a server with the client: https://gist.github.com/pupnewfster/fb850a424482e2f2956d14dc7b7db0b4.

Server dc'd the client (well really froze it on connect until I killed the process) after it ran into that on connection and also logged

[17:54:05] [Server thread/WARN] [minecraft/Connection]: handleDisconnection() called twice

Not trying right now in neodev without mods so there is a tiny chance something I did in mek broke it... but given we don't touch villagers and which registry is erroring out, I somehow doubt it?

[hoary umbra]

Probably not, intrusive holders are a design error

[stuck lava]

that will probably be fixed by #1170668658813575230 if it is a neo issue

[heavy zephyr]

We need to keep forge-forge connections working

[stuck lava]

@hoary umbra @lofty citrus @cerulean oxide @past saffron @winged iris @heavy zephyr @stuck lava @keen thicket
It's time for final review, all previous reviews are resolved, please do change request if there are any issues or a comment with if it looks good
(you are pinged because you left a review or committed to the branch)

[lofty citrus]

it's the FINAL COUNTDOWN

dear god, 208 files

[high raptor]

https://tenor.com/2qzD.gif

[heavy zephyr]

We still need to fix client-server connections apparently

[stuck lava]

if that is a us issue and not a mek issue

[heavy zephyr]

it most likely is

I bet nobody ever did any sort of client/server testing

[stuck lava]

did anybody do any testing?

[heavy zephyr]

if you and maty didn't then nobody did

(forgedev tests)

nvm I'm stupid I need to disable online mode 😄

Caused by: java.lang.IllegalStateException: Duplicate id 1 for ResourceKey[minecraft:villager_profession / minecraft:armorer] and minecraft:none

[stuck lava]

wtf

[hoary umbra]

I think we used to have some special magic code for assigning id 0

perhaps it is missing

[heavy zephyr]

lol the client has been "connecting to the server" for 5 minutes

why this doesn't use a CF, I don't understand

[stuck lava]

I think this is going to be replaced anyway so if this is the issue just do a bandaid fix

[heavy zephyr]

yeah you're right let's go for something that just works™️

[lofty citrus]

currently poking my way through the PR

test mods for last, of course

[stuck lava]

because it probably was written before enqueueWork returned a CF

[heavy zephyr]

yeah I suppose

CF is important to not deadlock the client

ok the problem comes from non-defaulted registries I think

hmmm no it is defaulted

🤔

I think it's not happy because it doesn't receive the values in numerical id order

[lofty citrus]

I know this is a fix by the registry rework PR, but how in the heck did this happen previously

[stuck lava]

I fixed the the deadlock

[heavy zephyr]

I already fixed it locally...

[heavy zephyr]

this is just a fuzzy patcher issue I think

[lofty citrus]

ah, I see -- it was moved down from an ealier call accidentally

[heavy zephyr]

ye

[lofty citrus]

yeah, sounds like the fuzzy patcher being stabbies

(in FlowerPotBlock.java.patch)

to review comment or not to review comment

[heavy zephyr]

wtf is this

[lofty citrus]

i'm not quite sure what "this" is referring to, there

[heavy zephyr]

() -> Blocks.AIR

[lofty citrus]

it's consistent with vanilla, at least

I think

anyway, I'm not trying to think too much about the many patches

[ripe storm]

A supplier shouldn't be needed there, right? As reg replacement is 🦀

[lofty citrus]

the registry rework solves https://github.com/neoforged/NeoForge/issues/60, right? "ForgeRegistry.add slows down with more entries"

[heavy zephyr]

yes

[lofty citrus]

the flower pot block maintains a fullPots map of resource locations to block suppliers

best to leave that sort of cleanup after the registry rework

as to not balloon the already-large size of the PR

[warped forge]

fwiw the purpose of suppliers in forge code is not primarily for reg replacement, but rather to support delayed registration

it lets you inject the RegObj before it has initialized

[stuck lava]

yea but IMO we should now use Holder for that if it does not need a specific type

[keen thicket]

either holder everywhere or supplier everywhere

mixing them up is just painful

[stuck lava]

for some things we can not use holders because the suppliers use the specific types

[lofty citrus]

i feel like that Refresh button after a commit is pushed is gonna be the bane of my reviewing experience

[lofty citrus]

slowly chewing my way through the PR

[heavy zephyr]

We should prefer Suppliers IMO unless we need holder methods

[lofty citrus]

why must (review) conversations be marked resolved without an answer to the question the conversation started with

review sent

[heavy zephyr]

OK snapshot application is fixed

[heavy zephyr]

@lofty citrus that's gonna be a nope for {@return} with nested tags if intellij can't handle it

[lofty citrus]

i usually write javadocs on the principle that the output in javadoc should be definitive, and that deviations of IDEs from that output are the cause of bugs or unimplemented behaviors in IDEs

[heavy zephyr]

yes but that's not how modders are gonna interact with it

[lofty citrus]

my preference is still to use the inline return tag

you may decide otherwise

[heavy zephyr]

I prefer to think of modders browsing the code via intellij

on top of that, I really don't want to bulk update all the javadoc in the PR...

I did remove the <br> usage however cause that is quite gross

[lofty citrus]

ah well, we can update the inline returns in the future, when IDEA cleans up its act

but thank you for yeeting those <br>s

[heavy zephyr]

I think it's waste of time to bulk update javadoc to add {@return 😛

I really don't see the value of the inline return tags

[lofty citrus]

they exist to remove duplication between a getter's body of Returns ... and the @return ... block tag
the body is important because the first sentence is used as the summary of the method in e.g. javadoc's method listings on the class page

[stuck lava]

tech why did you change the handleRegistryLoading again... join is the wrong method to preserve behaviour you should use get

[heavy zephyr]

I applied my patch to reduce the diff and log messages in the right order

what's the problem with join?

[stuck lava]

it hanles it differently

[heavy zephyr]

it just changes the type of the thrown exception

get throws a checked ExecutionException whereas join throws a CompletionException

I also tweaked the logic a bit so it would properly report the internal error (instead of failing due to an NPE in subsequent code and reporting that to the user)

[stuck lava]

join swallows the InterruptedException

we want to propagate it to the current thread

[heavy zephyr]

that's not what the code was doing

it was calling interrupted instead of interrupt

[stuck lava]

yes which clears the interruption

[heavy zephyr]

what is causing these interruptions in the first place?

[stuck lava]

it is a safeguard if anything kills the worker thread that it does not kill the network thread

[heavy zephyr]

if we are on the network thread this will be sent to execute on the client main thread

I don't see what could go wrong with interrupts there? if someone interrupts the client main thread it's probably gonna stop anyway, and if the client thread is interrupted I don't see how this propagates to the network thread?

[heavy zephyr]

OK then maybe we should not have a @return block? what do you think?

another question: what does "builtin" registry refer to? only the vanilla ones in BuiltInRegistries? or does it also include modded non-datapack registries?

[lofty citrus]

good question

@keen thicket care to elaborate

or would it be Shadows

or Shrimp!

[heavy zephyr]

I mean we choose the wording at the end of the day

my question is: what should it be?

[stuck lava]

for me builtin is all non dp

[heavy zephyr]

that is also what I would use

[lofty citrus]

we could perhaps go the route of a "static registry" term, in reference to registries which are defined on startup of the game (in current terms, non-datapackable registries)

[heavy zephyr]

static/dynamic is the fabric terminology, yes

[lofty citrus]

though built-in being the opposite of datapackable is what I would first think of, yes

[heavy zephyr]

but vanilla uses "builtin" now so we should maybe follow suit

I mean that's 3 people in favor of builtin

[lofty citrus]

sounds like consensus to me -- shipit

[keen thicket]

their javadoc tree needs updates in order to support that. I made a pr for fixing the inline return tag itself, but I did leave a note saying that nested tags are broken

[lofty citrus]

does JetBrains have a tracking issue for that? (nested tags being broken)

[keen thicket]

yes

[heavy zephyr]

what's the point of firing the ModifyRegistryEvent once per registry? we could fire it once to modify all registries

these == checks are pointless

should we go all the way and rename NewRegistryEvent -> NewBuiltInRegistryEvent, and also make the modify event be named ModifyBuiltInRegistriesEvent

maybe not

[cerulean oxide]

why

keys are interned

[cerulean oxide]

oh

I see

[winged iris]

So not a mek issue

[heavy zephyr]

indeed

[winged iris]

yay or nay to the holder fluidstack constructor overloads I mentioned yesterday? Seemed somewhat split on opinions of if it was close enough to this PR

[heavy zephyr]

why not imo

[winged iris]

that was my thinking given it is just a few lines and is related via holders

[heavy zephyr]

@winged iris gonna add

[winged iris]

Looks good to me

[heavy zephyr]

OK, we're back to no outstanding comments (except for my own question about isPresent)

[past saffron]

I'd say nuke isPresent(). AFAICT it serves the exact same purpose as isBound() and is just a leftover from RO

Unless of course there's a difference between being able to retrieve a holder from a registry (which isPresent() checks for, basically) and that holder actually being bound

[hoary umbra]

There should not be a semantic difference between what isPresent and isBound mean

[heavy zephyr]

for the N-th time this week: all requested changes were applied 😄

[lofty citrus]

i shall re-poke soon™️

[heavy zephyr]

I mean, do it today if you want to 😛

[keen thicket]

preferably don't so that we can merge it tomorrow /s

[lofty citrus]

by "soon", I mean "likely later, once I've actually eaten"

[keen thicket]

@heavy zephyr https://github.com/neoforged/NeoForge/actions/runs/6912798653/job/18808952077#step:6:94

[heavy zephyr]

I'll fix it tomorrow

[stuck lava]

I'm fixing it now since we want to merge tomorrow

[stuck lava]

is anything still open?

if not and no more change requests are coming in until <t:1700413200:f> I will approve the PR for merge

[keen thicket]

you cannot, you committed to the PR

you need 2 people that haven't

[past saffron]

I'm currently working on getting my FramedBlocks environment to compile and run against the registry rework and if that works fine, then it's from my side

[stuck lava]

I had to because you didn't

[lofty citrus]

if anything, it shall be me and pup who will approve and merge

[stuck lava]

I mean anyone that made a change request has to approve before it can be merged which are you, me and xfact

[lofty citrus]

we do have the power to dismiss reviews

in the extreme case

[stuck lava]

I know but it bad

if xfact and you approve and nobody requests changes until the time i sent I will merge it (because 2 approves means self merge is allowed)

[lofty citrus]

also,

[heavy zephyr]

19th 6 pm you mean? Kekw

[stuck lava]

yes CEST FTW

[lofty citrus]

imagine being in UTC+1
couldn't be me

[past saffron]

Works fine in my testing, including custom registries, on both SP and dedicated server. ID sync for the custom registry also worked on the dedicated server (server and client had different registration order for testing). That's a from me

[winged iris]

My minor testing in single player with mek also seemed to work (haven’t gotten around to testing server side since client server connections have been fixed, though also not sure if I will get around to it)

[stuck lava]

sci are you still planning on doing another review or does a 2nd review by any non committer suffice

[past saffron]

sci is probably asleep at the moment. He still has an open change request btw: https://github.com/neoforged/NeoForge/pull/257#discussion_r1398230553

[stuck lava]

ok then that is the last thing that needs to be done (but I don't even know what to put into the comment so probably not me)

[lofty citrus]

yes, was asleep
i shall do a second pass later, for changes since last review and the test mods

[cerulean oxide]

Once this gets merged I should make a PR fixing Mojang registries to not have an O(n) register method

[lofty citrus]

I was assured that was fixed by the registry rework

https://github.com/neoforged/NeoForge/issues/60 this

[south mirage]

@keen thicket EXPLAIN

[hoary umbra]

Well I think that issue is resolved by virtue of it not existing

if the vanilla registries exhibit the same problem, that may not be fixed

[cerulean oxide]

Forge & vanilla both have the problem for different reasons

So #60 will indeed be closed, but the performance issue will still exist

[heavy zephyr]

Ah we'll fix it after the rework then

[keen thicket]

<@&1128776937410670663> you have around 4 hours for the final review pass of the regs pr

[lofty citrus]

don't rush me

when it's merged (with co-authors), don't forget to also publish the registry rework blog post, with an accompanying announcement in #announcements

perhaps even an announcement in GH Discussions

[warped forge]

anyone has a link? the actual PR isn't pinned

[woeful mountain]

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

[warped forge]

found it in conversations

[heavy zephyr]

I'll be there around 6:10 to publish the blog post

[warped forge]

oof 208 files changed :P

I don't think I'll be able to contribute a whole review

my focus window is too smol for that

[lofty citrus]

what's the state of https://github.com/neoforged/NeoForge/pull/35?

[stuck lava]

not ported... stale

[lofty citrus]

close (the PR)?

[keen thicket]

superceded?

[lofty citrus]

sounds good to me, then

[warped forge]

could be merged into 1.20.1 if we make a branch for it?

[keen thicket]

@past saffron @lofty citrus please do a last approval

[lofty citrus]

ya ha ha

[keen thicket]

~1 hour and 30 mins left

[lofty citrus]

wouldn't it be 2hr30m

based on the timestamp last sent in here somewhere

[stuck lava]

2h22m

[atomic latch]

<t:1700413200:R> ?

[stuck lava]

yes

[keen thicket]

i prefer "in stab hours"

[heavy zephyr]

@keen thicket it's UTC+1 not +2 for the blog post

@stuck lava can you add yourself as an author on the blog post?

You'll have to supply some metadata (avatar, description)

[stuck lava]

if someone can tell me what exactly and which format

[heavy zephyr]

Just add yourself at https://github.com/neoforged/websites/tree/main/data/authors and in the frontmatter of the blog post

[stuck lava]

I have no idea what format those yaml files have are there any docs?

[south mirage]

is this panik time?

[winged iris]

No

[heavy zephyr]

Just look at what's already there

[winged iris]

We don’t get to panik until after hitting merge

[heavy zephyr]

You can test locally by running hugo server

(you'll have to install hugo first)

[stuck lava]

50minutes

[keen thicket]

T-48

[stuck lava]

@lofty citrus you approved but there is still an unresolved conversation

if sci doesn't answer I'll resolve and merge

[keen thicket]

T-19

[stuck lava]

yep I'm ready

should I use the default commit message or remove the commit titles from the contained commits

MATY ^

@keen thicket

T-12

[atomic latch]

<t:1700413200:R>

[stuck lava]

automerge is enabled

I just need to resolve the open convo

[keen thicket]

link the post in the commit message

let me update the post real quick

[stuck lava]

what should the commit message be?

T-3

T-00:00:30

[heavy zephyr]

Registry Rework or something

[cerulean folio]

<@&1168623741715107890> Can we merge this?

[heavy zephyr]

let schurli do it

[cerulean folio]

Yeah sure

[stuck lava]

the title but in the body to link the blog post

[keen thicket]

wrong role also

https://neoforged.net/news/20.2registry-rework/

it's being built

[heavy zephyr]

wtf you didn't even remove the .X

[cerulean folio]

WTF

[heavy zephyr]

let me take care of the post, just hit merge on github alreaady 😛

[keen thicket]

i changed it in one place

[shrewd garden]

There's a role for moderator+maintainers so the role icon is preserved between both :p

[keen thicket]

didn't notice the summary

[heavy zephyr]

good thing I had a TODO everywhere 😛

[keen thicket]

i changed the one in the body but not the summary

whoops

[stuck lava]

this ok

[heavy zephyr]

yes

[stuck lava]

Merged

[heavy zephyr]

IT'S LIVE

now we monitor #builds 😄

[south mirage]

New bug report! Registries deadlocks the game in prod

[ripe storm]

🎉

Any bets on how long till the first "this neoforged update broke all my stuff" confusion?

[south mirage]

Now I just need to decide if I should port my mods to 1.20.2 neforge or wait for caps work

well, i can do my cap-less mods

[ripe storm]

Looking forwards to updating some of my stuff now that this exists. Probably going to wait on networking though on second thought

[gloomy sinew]

ng already broke all my stuff

[atomic latch]

oh lawd it's live

[heavy zephyr]

lmao

[fierce wren]

I should probably write a trick for that

[heavy zephyr]

let's try again

OK it seems to work after running it manually for the second time

[stuck lava]

Oh lawd it comin #builds message

[heavy zephyr]

why is the author neoforged team

[stuck lava]

does the theme not like multiple authors?

[heavy zephyr]

apparently not

[hard coyote]

The Registry Rework

As of NeoForge 20.2.59, the **registry rework **has been implemented, which is a breaking change affecting many mods.

If you are a player whose mods suddenly broke, we ask you to wait for the modders to adjust to the new system.

If you are a modder, please refer to the blog post on the NeoForge website for more information.

[fierce wren]

this exists for as soon as people come and complain that their mods broke

[heavy zephyr]

[stuck lava]

IT HERE

[atomic latch]

[gloomy sinew]

!registryrework

[hard coyote]

The Registry Rework

As of NeoForge 20.2.59, the **registry rework **has been implemented, which is a breaking change affecting many mods.

If you are a player whose mods suddenly broke, we ask you to wait for the modders to adjust to the new system.

If you are a modder, please refer to the blog post on the NeoForge website for more information.

That trick is promoted. Consider using </support registryrework:1143596306917494804> instead.

[woeful mountain]

why did that only half work