Registries Overhaul

[high raptor]

I presume RegObjHelper has those 11 DRs though? 😛

[heavy zephyr]

I don't see the problem 🤷‍♂️

[hoary umbra]

Well RegObjHelper doesn't actually do registration, its just a literal RO creator factory

[ripe storm]

Modded registries exist

[hoary umbra]

The DeferredHelper thing I linked up there is the comparison

Nonetheless, no, it does not contain 11 DR's

[ripe storm]

I think that "specific DH types" idea is... generally just kinda ugly

[high raptor]

Yeah, I prefer the two-generic-types approach rather than a subclass-per-registry approach

[stuck lava]

it makes custom registries not fun to work with too

[hoary umbra]

it just contains protected final Map<ResourceKey<? extends Registry<?>>, List<Registrar<?>>> objects; where Registrar is record Registrar<T>(ResourceLocation id, RegistryObject<T> obj, Supplier<T> factory)

[ripe storm]

Yeah, exactly

[hoary umbra]

Well things are extensible, so people with custom registries could extend and create custom types as necessary
I imagine most of the time it wouldn't be necessary

honestly the main selling point is getting access to ItemLike, lol

but that can be done independently by modders (since DH non-final) now

previously it was impossible (RO is final)

[stuck lava]

it's a clear no from me for DR/DH subclasses

[heavy zephyr]

Nobody answered this 😦

[ripe storm]

I figure if people really want that they can implement it themselves. No need to shove a bunch of overloads that special case vanilla stuff into this

[hoary umbra]

Holder<T> has a contract that T must be the registered type

or are you asking "why are we using Holder at all"

[heavy zephyr]

The latter

[ripe storm]

Holders are useful for Item and Block and anything worldgen

[high raptor]

a lot of mojang's built-in datagen is based on holders now

[stuck lava]

mostly blocks I think for worldgen stuff

[hoary umbra]

all of that yeah ^

[heavy zephyr]

How does mojang do it then for blocks?

[ripe storm]

But most stuff doesn't take ? extends T, it takes T

[high raptor]

tag datagen uses holders for example

[ripe storm]

So you need a Holder<Item>

[hoary umbra]

mojang doesn't store Holder references

they do static init

[ripe storm]

Mojang does static init, but even then they erase the specific type most of the time and just store it in a Block

[hoary umbra]

There are 215 calls to Holder#value() in the mc codebase thus far

this is likely to increase as time goes on

[high raptor]

Mhm, where they use holder they don't actually care about the specific subtype

[ripe storm]

The useful holder type is Holder<Block>. People occasionally need access to a MyBlock. But 9/10 times, the nonspecific type is more than enough and you don't actually need the specific one

[high raptor]

That's why I proposed keeping RegistryObject with the specific subtype, but keeping DeferredHolder "clean" with just the registry type

[hoary umbra]

Really the best thing would be getting mojang to replace their silly usages of Holder<T> with ? extends T

but bleh

[ripe storm]

No need; DH can have both and if you only want the nonspecific you just assign it to a Holder

[high raptor]

Fair point, that works too 😅

[ripe storm]

See #1131130393144336385 message

[high raptor]

(I haven't completely woken up)

[stuck lava]

can we patch that?

[hoary umbra]

not usually

[ripe storm]

Cleanly? Not really. It's a lot of different places

Especially as vanillas design philosophy is that the specific type shouldn't matter!

[hoary umbra]

There are some DH-Specific methods you miss out on if you go to single type, but nothing I can't solve with a singly-typed interface that extends Holder, I think

[ripe storm]

They do quite a lot of work to make sure that you can erase those specific types without issue, at least in the cases of types normally used with holders (blocks especially)

[pallid quarry]

Can we not have a <S super T> Holder<S>coerce() method, or do the generics not work out? I can never remember when super is applicable.

[high raptor]

That's the current cast method, I believe

[pallid quarry]

Oh, that does exist already? Nice.

[ripe storm]

You can sorta but that's ugly. The nonspecific type should work by default, otherwise you're coercing every holder for the main thing they're useful for

It's a really bad solution

[stuck lava]

there even was a vanilla method that does basically that in the early stages of Holder

[ripe storm]

Because holders aren't meant to have the specific type

And frankly, I don't think I've ever had a need for a specific typed item instance

It should be possible to get but we shouldn't butcher holders for it

And that means dual generics

[high raptor]

Yeah. My suggestion would be that the DeferredHolder class be typed <TRegistry, TInstance extends TRegistry> and then it should implement Holder<TRegistry>

[deep lance]

it's also not possible to prove that a deferred holder created outside of registration has the correct type

[ripe storm]

That's the current plan I believe, yeah

[ripe storm]

That too, that's an important point!

[high raptor]

If somebody needs the exact TInstance, they can cast to DeferredHolder, if they don't they can just have the Holder

[deep lance]

like, ultimately there's always going to be some unprovable generic type ops involved, like the creation of registry keys, but it'd be best to minimize them

[cerulean folio]

Question in that scenario, what would calling the "get()" method return? the TRegistry or the TInstance?

[deep lance]

people can cast manually if they need the specific type

[ripe storm]

They don't even need to cast; it'll be a deferred holder by default which can be erased to holder

[ripe storm]

Specific type

That's an easy enough overload to do if it isn't don't already

[cerulean folio]

Okey thanks that solves my concerns, thank you

[ripe storm]

Because the specific type is only in the return and it extends the nonspecific

[high raptor]

If it's from Holder<TRegistry>, a TRegistry - if it's from DeferredHolder<TRegistry, TInstance>, a TInstance

[deep lance]

vanilla doesn't even use specific types for static fields

our use of specific types for registryobjects has been an unnecessary luxury

[cerulean folio]

Yes but modders have been since the beginning of time

[pallid quarry]

Fair enough. I've never actually needed to use holders, so not actually sure how often would need to cast anyway.

[ripe storm]

Yeah so we give them a way to still get it

[deep lance]

how much are people actually using it vs just using it because it's the style

[ripe storm]

But we don't butcher the vanilla system to do so

[cerulean folio]

yep yep yep yep yep

[cerulean folio]

I do nearly everywhere

If it is not a basic block

I need the specific type everywhere

[high raptor]

Yeah, I have some code in my mod which uses a specific block/item type to lookup some instance-specific properties

[deep lance]

by actually i mean using it to actually invoke a method that is not declared on the parent class, that would otherwise require an explicit cast

[ripe storm]

And that'll hopefully encourage people to stop depending on something they probably shouldn't have to begin with, while still letting them do so because it's sometimes very useful

[high raptor]

Like, I register multiple RockItems in my mod, which have a getRockType method

[cerulean folio]

Can we make the DH implement supplier?

So that we can directly inject into location where a supplier is needed

Or do we plan on replacing the supplier consumers with holder consumers?

[high raptor]

and for certain crafting recipes I have to check that the rock type is a specific type, which means checking if the item is a RockItem

[ripe storm]

Already does but it implements Supplier GenericType, and that can't be changed because Holder implements Supplier

[deep lance]

ok but if you have a stack of it you have to instancecheck/cast, how often are you calling it actually on the known item fetched from the static RegistryObject?

[cerulean folio]

That is fine

[ripe storm]

But you can use ::get to get a specific supplier off it

So that's fine

[cerulean folio]

I was thinking in the context of for example sounds, where we consume a supplier<TRegistry> type anyway

[ripe storm]

Ideally I'd say yes, this should be the goal

[deep lance]

like, I'm asking how often people directly literally call MyItems.ITEM.get().myMethod(), not just how often you use a method defined on a subclass in any context

since in other contexts you have to cast anyway and there's nothing we can do about that

[pallid quarry]

Quite a lot in my case, especially in datagen. And JEI integration, but same difference.

[ripe storm]

Yeah, it's got uses for datagen

[stuck lava]

yup mostly datagen

[cerulean folio]

I use it nearly everywhere

[high raptor]

I've tried to make most of my datagen work based on static types rather than methods on my registered objects

Like I have a Map<RockType, RO<RockItem>> in my item registration which I then just lookup based on RockType

[deep lance]

getting beside the point but why not a Map<RockType, RockItem> populated at construction time?

i do accept that there are resonable datagen cases for accessing specific class methods on a known instance though

i hadn't been thinking about datagen when i asked

[ripe storm]

It's also useful for block entity types to get at the generic type

But yeah, I think in the long run dual generics is best for that reason

[deep lance]

I still think it'd be nice to have the ro/holder on blocks and items implement ItemLike

enough to be worth the extra complexity it creates elsewhere

[ripe storm]

Eh, I'm not convinced. After all, it's at best a Supplier<ItemLike>

[deep lance]

ItemLike itself is a supplier. you can use ::get as an ItemLike

[ripe storm]

And the ugliness that creates is really bad

[deep lance]

well, for items anyway, not blocks so much

[hoary umbra]

I hate having to do new ItemStack(BLAH.get()) instead of new ItemStack(BLAH)

[deep lance]

this is what i'm saying!

[ripe storm]

Oh, then I'd say people should do that then

But I mean, this is the advantage of it all being extensible

[deep lance]

[in like recipe datagen too, you could avoid a ton of .get()'s this way]

[ripe storm]

If someone wants this sort of stuff it's trivial to add themselves

[high raptor]

I wonder if we could patch in an ItemStack(Holder<Item> holder) for this case specifically

[deep lance]

Holder<? extends ItemLike>

[ripe storm]

Ah yeah, for blocks too, sure

That would be a cleaner solution than a bunch of hardcoded overloads in DR

[hoary umbra]

the holder ctors already exist so we just have to un-fuck the generics™️

[high raptor]

gotcha, that should be fine then in theory

[deep lance]

there and Ingredient.of would cover 95% of the cases

[high raptor]

We should also look into other uses of ItemLike and see if we can patch-in equivalent Holder-accepting methods too

[deep lance]

Ingredient.of needs unfucking anyway, can't handle a mixture of items and tags currently

[cerulean folio]

on 20.2 it can

[deep lance]

cool

but anyway, a few small patches here or there to eliminate a ton of .get() noise would be useful - and probably good enough to not need restructuring the entire deferred registry system after all 😛

is there some transformer we can run at decompile time to simply change every Holder generic from <T> to <? extends T>

[cerulean folio]

No, and making that is not really possible either

[heavy zephyr]

That's probably not sound anyway

[hoary umbra]

its semantically always legal, I believe

[high raptor]

I wouldn't want that personally, either

[hoary umbra]

but it sounds like a disaster, lol

[cerulean folio]

It sounds like something that will go wrong 20 ways before we hit sunday

[pallid quarry]

If only Java had proper co/contravairant generics :(

[heavy zephyr]

Couldn't we DH<R, T extends R> implements Holder<R>, Supplier<T>?

[high raptor]

I'd be okay with that

[cerulean folio]

Nop

Since Holder already implements Supplier<R>

[hoary umbra]

Well that's us

[heavy zephyr]

Fuck

[hoary umbra]

We can make it not do that™️

[cerulean folio]

If it is our patch

Then sure

[high raptor]

DH is our type

[cerulean folio]

But if it is from vanilla, then it will become our type

[high raptor]

oh right Java doesn't have explicit interface implementation

I'm still waking up

[hoary umbra]

If we change the patch to implements Supplier<? extends T> can subclasses specify a stronger type? I forget what the rules are for that

it can't be ? extends T can it

bleh

scrims at generics

[heavy zephyr]

Can't do that

? can't be used in class type bounds

Holder<MyItem> doesn't make sense IMO

Since it accepts tag and resource keys with a MyItem type

[hoary umbra]

well yes, that's part of the problem

those should be ? super T

[heavy zephyr]

Yes and no

Perhaps it is meant to be == T

[ripe storm]

It's meant to be == T because all it's linked to is a registry

Like, think in terms of datapack registries and whatnot. You can't know that type

I'd say just keep two generics. People can use ::get if they want a supplier

[heavy zephyr]

there is no get

[heavy zephyr]

I think this is the correct way to go about it

then the user has the choice between:

public static final Holder<Block> MY_BLOCK = DR.register(...);
public static final Supplier<MyBlock> MY_BLOCK = DR.register(...);
public static final DeferredHolder<Block, MyBlock> MY_BLOCK = DR.register(...);

[high raptor]

Yeah, I'd tend to agree

[stuck lava]

but then you can't use vanilla holders in place of suppliers anymore

[high raptor]

It's a shame we can't define explicit cast operations, so that Supplier<MyBlock> needed an explicit cast, for example

[heavy zephyr]

are there such suppliers?

[high raptor]

since Holder<T> is a Supplier<T>, I imagine Java would get confused as to which type you wanted to cast to if it had both Supplier<MyBlock> and Supplier<Block> to choose from

[deep lance]

most places that need a holder or supplier need the less specific type, anyway, it's just that you want to be able to directly call get to get the more specific type

[ripe storm]

There is though? And if not there can be

If the superclass is Supplier Block, you can just override that get to return the specific type

And then people can trivially get a specific supplier

Otherwise, make holder no longer a supplier, and that would work too

[heavy zephyr]

can we please also delete IReverseTag?

[hoary umbra]

I think people use that

[heavy zephyr]

they can replace it with holder

or make their own interface

[deep lance]

with runtime mojmap we can rename the IReverseTag method to is and make Holder implement it

idk if it's really worth keeping though

[high raptor]

deprecate for removal? 🥺

[deep lance]

[i thought the whole of ITagManager was going away with the rest of forge registries though]

[heavy zephyr]

just delete it please

I want the full rework (all parts, not just this) as an updated 1.20.2 PR that can be reviewed thoroughly, considering all the aspects at once

because there's a lot of stuff that will be missed if we try to split this up into 2-3 parts

[high raptor]

I'd tend to agree, but I wonder how hard porting would be, since it's already going to be difficult enough with the gradle and runtime mojmap changes

[hoary umbra]

Well I can start looking at things once eclipse runs again

[heavy zephyr]

runtime mojmap will make no difference on mod code, and neither will gradle 😛

[hoary umbra]

Well gradle will :p

[high raptor]

Fair, I just mean the amount of work modders will have to go through as a whole 😅

[deep lance]

so is tag manager going away?

[hoary umbra]

idk, what does it even do?

[heavy zephyr]

/**
 * A tag manager holds information about all tags currently bound to a forge registry.
 * This should be preferred to any {@link Holder}-related methods.
 */

lol

[deep lance]

i've always hated the damn thing, it was added in 1.18.2

[heavy zephyr]

I guess shrimp might have reworked / removed some of it already in his branch

[deep lance]

it feels like an attempt to build an abstraction layer to insulate against future vanilla API changes - which is always going to be a "winning the last war instead of the next one" kind of thing

[heavy zephyr]

it's just more forge registry nonsense

[hoary umbra]

its one of Lex's "I hate holders" things, I see

[stuck lava]

the big stuff (this and caps) should get reviewed via the 6 eyes principle (a review by the creator and two non committers to the PR)

[cerulean folio]

Yeah any larger stuff should also not be merged by a committer

But we can see how we deal with this

[heavy zephyr]

Yeah 6 eyes isn't enough

[stuck lava]

you saying 8 or 10?

[warped forge]

depends on the size and time availability

[high raptor]

I think enough people have reviewed the concept of this that just having maintainers review the code should be fine

[warped forge]

could also be coupled with an assessment similar to the one for voting applications, where people can be "very certain" about the changes being good (counts as 2 eyes), "positive" (counts as 1 eye), uncertain (0 eyes), "negative" (-1 eye), or "strongly against" (-2 eyes). and the PR needs 2, 4, or 6 eyes to be approved depending on the size.

[heavy zephyr]

ideally we'd get the review of as many people as possible

[heavy zephyr]

let's not get tangled up with annoying processes 😛

[keen thicket]

ah yes, 0 eye review

[ripe storm]

Processes have a place and a use. This probably isn't somewhere a strict process is needed though, unless it becomes obvious in the future that things are struggling and leading to unresolvable disagreements without such a process

But general stuff like "particularly large PRs should probably have at least 2-3 reviews approving them and shouldn't have people actively requesting changes" isn't exactly an overly burdensome sort of process

(which seems to be giga's idea, rephrased a bit)

[heavy zephyr]

I think people requesting changes is fine

[ripe storm]

*when merged

[heavy zephyr]

well I mean that's kind of obvious imo

[ripe storm]

Yes, I just translated giga's idea to the version that didn't sound as procedural. It's the same thing in the end though

[warped forge]

eh it was just an idea to make it easy for at-a-glance knowing if someone's review was high certainty or just a passing "lgtm"

which is not meaningful on a small PR, but for a huge one, some reviewer may have only glanced over, while another went word by word

but yeah process meant to make things easier can easily backfire and become a burden

[high raptor]

Yup, that's why in the things I suggest now I've started going for making notes that are meant to capture the basic idea rather than a complete rigorous process

[quaint pagoda]

I don't think I kept it

It's useless

The API was to appease lex because he didn't want to depend on Holders since he thought Mojang would change them every update

I've barely used the API and the nullable annotation on tags() was silly. I should've left it unannotated cuz it's only null in certain cases

[quaint pagoda]

I believe it was this yes

[quaint pagoda]

yes

[heavy zephyr]

I see, thanks

[stuck lava]

I want to coordinate the major changes, who is doing the rebase and finishing touches for this?

[heavy zephyr]

@hoary umbra but it's far more than a rebase

[hoary umbra]

rebase, lmao, yeah

more like total reimpl over 20.2

[quaint pagoda]

really?

[heavy zephyr]

probably yeah

[hoary umbra]

None of the diffs are really going to be salvageable except for pure forge code and for those you have to disentangle class and package renames before they'll be applicable

[quaint pagoda]

Ah

Yeah that's fair

[lofty citrus]

soooo... I see the registryobject PR's still open

anyone working on that at the moment?

[heavy zephyr]

We should do everything in one PR IMO

[lofty citrus]

if we can push through the biggest dev-affecting change first, with the relatively-unobtrusive changes following, then I'd suggest that option

[heavy zephyr]

we can't

[hoary umbra]

I mean, we can

mega-pr's are bad for various reasons

[heavy zephyr]

yeah but I want to have a look at the full refactor in a single pass

[hoary umbra]

Like 90% of it will be churn

the "refactor" part is DH (the open pr) and moving two features to vanilla regs (aliasing and... something else)

id sync, i think

[quasi hornet]

Just wanted to check in here - has it been determined to officially kill off some of my mods?

I've been pondering how to not do reg replacement in a few cases and I'm not sure there are very elegant ways 😛

[warped forge]

what do you use registry replacement for?

[quasi hornet]

Enchantments

[warped forge]

what does your replacement do?

[quasi hornet]

Adding just extra base features to vanilla enchants - I might be able to get away with mixin in some places

btw it of course won't kill the mod, I'll just remove the feature if I can't make it work again

[warped forge]

a mixin should work anywhere a replacement does

[cerulean oxide]

even a straight overwrite would probably be functionally equivalent to registry replacement

[warped forge]

worst case scenario

you could @Overwrite all the relevant methods

it would be the least compatible, but it would work

in most cases, though, the mixin could be an @Inject or @ModifyArg or @ModifyVar

[quasi hornet]

I suspect as much. I do worry when multiple mods are going to try the same thing

[warped forge]

if it's Inject with conditional return, that's not an issue

ModifyVar and ModifyArg can also chain

[quasi hornet]

How about adding enchantability to an item?

[warped forge]

the issues are with Redirect and Overwrite

adding enchantability means editing the Item.Properties for it?

[quasi hornet]

I think I'd have to add the return method

[warped forge]

in short: it's going to be on a case-by-case basis

but I can assure you, the worst case scenario is not worse than registry replacement

or in other words, registry replacement is as bad as the worst possible mixin you could write

[quasi hornet]

I think reg replacement done properly was fine, but I get that a lot of people maybe didn't handle it well

[hoary umbra]

Overwrite mixins are "as compatible as" replacement

only one may exist

[warped forge]

registry replacement doesn't support the idea of multiple people replacing the same thing

the last one to replace wins

it's no different than an Overwrite mixin

and I'm someone who doesn't see registry replacement as the evil others see

but it does have this huge downside

[hoary umbra]

Also two replacements silently picks one to use

and one just stops functioning

[warped forge]

overwrite with require=false or whatever it was does the same no?

[quasi hornet]

I suspect I'm probably just going to end up deleting a bunch of features which is fine

[heavy zephyr]

you can just redirect the relevant bit in Enchantments if you want to replace an enchantment

or at least submit a feature request

[quasi hornet]

Not sure how that would work. I'm not going to PR a mod feature

[keen thicket]

if you desperately need to reg replace, you can overwrite the register call of the relevant object

[quasi hornet]

haha that's fair 😛

tbh I really don't think I will, it just keeps things simple and more elegant than people might like to admit

mixins will likely be fine in most cases

[ripe storm]

It is possible to redirect the actual enchantment creation in clinit with mixins. Doing so is far safer than registry replacement

And that's a worst care scenario

In most other cases there's other ways around it all

(Whoops, I was scrolled up and didn't see maty's message)

[ripe storm]

Yes-ish - I think it'll at least log a warning by default? Not entirely sure

Redirect conflicts certainly will

[heavy zephyr]

Hey @hoary umbra how are we doing here?

[hoary umbra]

Well, I'm on vacation, so nothing

[heavy zephyr]

Ah hmmm. When will you be back?

(maybe someone else should take care of this rework)

[hoary umbra]

6th

[heavy zephyr]

Hmmm @stuck lava what do you think?

[stuck lava]

I'm currently a little bit sick so I can't do it myself but if anyone has the time to start it before then, then it's fine

[lofty citrus]

are we doing the RegistryObject changes first, and then the rest of the refactors to follow

or are we going full force with everything all at once

[stuck lava]

if any <@&1128776937410670663> has the time to start porting this to 20.2 just say it here

[lofty citrus]

i would prefer the former, if only to lessen the impact to devs when the full refactors come later, and to hopefully make it more digestible to review

[stuck lava]

I think it does not matter since we are not splitting the changes over versions

[hoary umbra]

doing it all at once would create an unreviewable fuckfest

if someone wants they can begin by porting the DeferredHolder PR

[heavy zephyr]

Imo we should do everything at once

[shrewd garden]

Do we make it easier for ourselves or easier for everyone else on the planet

[lofty citrus]

consider:
if the RO (registry object) to DH (deferred holder) changeover is by far the most developer-affecting change we have, then getting it out quicker through the door gives a longer window of opportunity for devs to switch over to DH (by following the deprecation notice on RO)
which then leaves the bulk of the overhaul left, which would be less dev-affecting than the changes to RO/DH

it's an added bonus that it makes reviewing the overhaul easier since we have at least two discrete 'phases' for it -- the user/dev-facing changes, and the internal (i.e. the rest) changes

pushing for DH first also means we can get feedback on that new API, and consider potential improvements to side-along come the bulk of the overhaul in the next PR

we could push through with the overhaul all at once, but it means:

• longer time to port the changes, since now both the dev-facing and internal sides are being mucked about
• longer time to review, since both sides need to be looked over
• devs have less of a window to switchover to DH (and give feedback on it)

[abstract scarab]

I mean at this point whatever changes you can bring forward the better

since you're just spreading out breaking changes at this stage

I think the concern is less aout the size of a breaking change with neo

and more about how many are gonna come

people seem less worried about having to redo a bunch of stuff than they are about having to redo some stuff 8 updates in a row

[lofty citrus]

my opinion is that it's better to try accelerate (to production) the breaking changes that are likely to affect many modders, and leave the more... niche? breaking changes for later

[abstract scarab]

I'm not gonna say whether I agree or disagree on that specifically

but I will say that if you're trying to entice devs to move over from forge, the bigger, more 'wtf forge why haven't you done this already' changes tend to be more attractive at an earlier stage

rather than saying 'switch to neo now, and in 1-10 years we'll fix up registration'

especially right now
NG7 just came out and it's basically completely broken for a bunch of people - so there's no real reason for people like me to even bother investigating until it's long past the stable point, since there's nothing particularly redeeming about trying atm

[heavy zephyr]

sci we're gonna get rid of IForgeRegistry notably

that's gonna break a ton of mods

so let's do it all at once IMO, it's nicer for us because we can properly review the entire rework and nicer for modders because they only get one BC

[lofty citrus]

why would that break a ton of mods?

[warped forge]

everything that references ForgeRegsitries.*

[past saffron]

That and also any custom registries need to be converted

[warped forge]

eg, Eyes in the Darkness would break because these 3 calls: ```
private static final DeferredRegister<SoundEvent> SOUND_EVENTS = DeferredRegister.create(ForgeRegistries.SOUND_EVENTS, MODID);
private static final DeferredRegister<EntityType<?>> ENTITY_TYPES = DeferredRegister.create(ForgeRegistries.ENTITY_TYPES, MODID);
private static final DeferredRegister<Item> ITEMS = DeferredRegister.create(ForgeRegistries.ITEMS, MODID);

[lofty citrus]

ah, the use of the IForgeRegistry in making the DRs, rather than the keys

i didn't account for that

ah well

[warped forge]

yes and also the uses of the classes for other purposes

eg in Tool Belt I have,

[lofty citrus]

yea, that makes sense

onwards with the full refactor, then

[hoary umbra]

I don't necessarily mean holding the changes to a later cycle

but doing one mega fuckfest PR will be horrible

[lofty citrus]

that has me thonking on making a separate registries branch, and PRing incrementally to that branch

and then doing a final PR from that branch to the main branch, which would need minimal review on the basis that all its constituent PRs have been reviewed previously

[hoary umbra]

yeah works for me

[shrewd garden]

workgroups/registries

ship it

[gloomy sinew]

this is why people should have been using vanilla for registry keys and id<->thing lookups since 1.19

[high raptor]

yeah I've been slowly moving to accept RegistryAccess and holders pretty much everywhere, which is why I'm so excited for DeferredHolder specifically

[hoary umbra]

glares at deprecated on vanilla fields

[warped forge]

but this is old code, and I don't go around changing code to add more deprecated method/field/class usage warnings :V

[gloomy sinew]

that's also why I keep telling people to ignore Deprecated if the thing still works

[heavy zephyr]

usually it's deprecated for a reason

[gloomy sinew]

yeah but sometimes it's a really dumb reason

[warped forge]

the reason wasn't dumb when the deprecation was added

"we have better API don't use this one"

not the poor annotation's fault that it stopped being true :P

[heavy zephyr]

yeah agreed with both of you 😛

[deep lance]

right but the decision to deprecate the vanilla registry stuff was kinda dumb to begin with

[stuck lava]

it was a lexism

[keen thicket]

I think a pr we can make right now to main is removing all deprecations on vanilla regs and deprecating the forge ones

so people don't still use the forge fields that will be gone

[stuck lava]

we should even deprecate them for removal

[lofty citrus]

go go go

[hoary umbra]

Well, I see nobody has started on anything in my absence

I expected this, but still™️

[keen thicket]

hmm actually

there's a lot of uses of the ForgeRegistries.* fields

and mods will probably have tonnes too

i wonder if it's worth just changing those to point to BuiltInRegistries.X during the rework

[stuck lava]

no because the methods in Registry and ForgeRegistry have different names

[keen thicket]

get and getKey which is most of the uses don't

and the other lot of uses is for DR creation

[stuck lava]

there is no get in IForgeRegistry it's called getValue there

also keySet vs getKeys

[keen thicket]

aaagh you're right.. well the test sourceset will just be painful to migrate

i don't think it's worth fixing all the uses of the deprecations at the same time as deprecating the forge reg fields

that will be much easier to do when the code using the deprecated fields doesn't compile because the fields no longer exist

[keen thicket]

ah, progress

[high raptor]

[keen thicket]

more progress

[heavy zephyr]

are you doing the registry rework then?

[keen thicket]

yes

objections?

[heavy zephyr]

by all means none!

are we keeping a list of registered registry objects in the world?

[keen thicket]

no

unless there's still a need to, i remember a discussion but i don't remember the outcome

https://github.com/Matyrobbrt/NeoForge/tree/reg-rework

[heavy zephyr]

I don't remember the outcome either

[heavy zephyr]

even if we don't store an int<->resloc mapping anymore, it would be nice to at least remember the reslocs

[keen thicket]

okay so going through some of the patches

first of all, block/item colours

do we still need to replace the idmap field with a map<holder, color> or can we do map<object, color>

this probably applies to like 5 more instances

i don't think we still need a holder without reg replacement

[heavy zephyr]

we can do map<object, xxx>

[stuck lava]

open the PR as draft so we can follow it

[keen thicket]

before i do that i need a volunteer

i'm not saying why just yet, i first need a volunteer

[shrewd garden]

Not it

[lusty pendant]

Not it

[stuck lava]

I don't have time atm got an appointment in 1h

[keen thicket]

well the volunteering doesn't have to happen right now

just in the next.. one or two days

[stuck lava]

maybe I have time tomorrow (now tell me what you want )

[keen thicket]

i want you to get rid of ForgeRegistries.X uses in the test mods

[stuck lava]

that's not that bad

[hoary umbra]

Just refactor->rename the forge methods to match vanilla and update the field refs, poof done

[hoary umbra]

also do smaller prs to a main repo branch, the one you have open is already hell to look at

[keen thicket]

yeah.. no. you try to refactor and remove stuff while splitting it into multiple commits

• Remove X
• Fully remove X
• Shit, forgot one method and one call
• Replace X
• Fix uses of X

you'll have to deal with it, and use file filters to review it. because i'm sorry but incremental commits when you're ripping apart an entire system are just.. no; you'll deal with it as you'll deal with the caps rework pr

at least this part cannot be feasibly fragmented

defferedholder and concurrent block creation and stuff like that sure

but this

no

[hoary umbra]

It's really not as difficult as you paint it out to be, but w/e I guess

[keen thicket]

i tried and gave up. if you want it split up then you do it

reviewing the PR as it is right now isn't as difficult as you paint it out to be either :P

there's more removals than additions

[hoary umbra]

Sure but you aren't done yet, lol

[keen thicket]

well i kind of am, it's just a matter of adding docs in a few places and updating the test mods. and the test mod changes you can just hide when reviewing

[heavy zephyr]

yeah splitting this kind of stuff is generally a pain

the registry object stuff should probably go in this PR too since I thought we were hoping to get rid of RegistryObject in the first place?

[cerulean oxide]

why are we patching IDs back in?

[keen thicket]

may you elaborate

[hoary umbra]

We have to patch in id sync, is it regarding that?

[cerulean folio]

Yep

[warped forge]

registry id sync is needed in order to ensure IDs match. mod initialization order is not necessarily identical (diff mod versions, additional mods in client, undererministic initialization order)

id persistence shouldn't be needed though, if I remember? 1.20.2 removed the last thing storing Ids on disk?

[cerulean folio]

It still is required

Last time I checked

Because you can still get the stupid global palette

[hoary umbra]

It shouldn't be necessary

[peak wagon]

Fabric did remove id persistence so I'd expect forge to be able to too

[heavy zephyr]

Yeah ID persistence isn't needed anymore

[gloomy sinew]

what uses a global palette

(as opposed to chunk-local palettes)

[cerulean oxide]

there is a fallback if there are too many blockstates per subchunk

[heavy zephyr]

the last thing that used ID persistence was status effects

[cerulean oxide]

at least there used to be

[heavy zephyr]

oh yeah is that still needed then? if yes then we absolutely need to keep ID mappings... (or change the code so it doesn't use the fallback?)

[cerulean oxide]

the possibly naive idea I had at midnight was to sort resource locations on both sides and then assign them IDs in order

[gloomy sinew]

what's the threshold for blockstates per subchunk

[heavy zephyr]

the client might have more blocks

[gloomy sinew]

is that still allowed? or do we reject connections if the client has stuff the server doesn't

[cerulean oxide]

it would still be a problem for vanilla connections when mods that add blocks are installed

[past saffron]

There are apparently cases where the connection is not rejected, namely vanilla client connecting to NF server with content mods. But NF -> NF connections always disconnect on block registry mismatch AFAIK. They don't do so on state map mismatch though, which can lead to all kinds of wonky issues like blocks appearing as completely different ones on the client

[gloomy sinew]

I'm interested in this!

[cerulean oxide]

a cursory glance at the paletted container suggests it switches when the block count exceeds 8 bits (i.e. more than 256)

[past saffron]

Yeah

[stuck lava]

I'm working on fixing the tests for maty... wth is going on with gradle it can't find any dependencies (not even mojang)

[cerulean folio]

Yeah that is known

[cerulean oxide]

I have had a ton of issues getting Loom to work reliably the last two days as well, so it might be a Mojang maven issue

[gloomy sinew]

gahhh I always assumed it was fine with 4096-state palettes per subchunk

like I thought that was the whole point of 1.13

[ripe storm]

What does fabric do for this?

[stuck lava]

@keen thicket I did the thing

[keen thicket]

patpat

[heavy zephyr]

I think it just doesn't solve that problem

[warped forge]

wait how does the global palette work?

[ripe storm]

Then are we sure it's an actual problem? Do we see cases in practice where it is? I'm pretty sure vanilla shifts around the numeric IDs between versions unless I'm mistaken with how they add stuff to the various registering classes but maybe they're extra careful with blocks?

But otherwise, yeah, if this is definitely an issue then that's unfortunate

[warped forge]

keep in mind

this isn't block IDs

it's blockstate IDs

[gloomy sinew]

is the global palette actually global

[warped forge]

doesn't require the registries to be mapped

no it's not about it being global, it's about it using global IDs instead of local IDs

it uses Block.BLOCK_STATE_REGISTRY as the id mapper

and doesn't store the palette in the chunk section at all

[gloomy sinew]

ahh

[warped forge]

so neo would still need to serialize the blockstate id registry

"registry"

[ripe storm]

Yeah, okay. Still slightly confused why that doesn't break when vanilla adds new blocks but maybe the answer is DFU or something, or some other weirdness

[hoary umbra]

Could we just save an extended pallete to the chunk when / if that happens?

[warped forge]

I feel like that isn't any better than having a little thing to save the blockstate id map to disk

specifically that id map

[heavy zephyr]

Could be a big map ...

[warped forge]

true

wait I thought biomes didn't save IDs anymore

but they also use the global palette

if there's more than 3 bits worth of biomes in a chunk

or is that 1.20.3 when nothing else uses IDs?

no that code is the same in 1.20.2 and latest snapshot

[cerulean oxide]

Does vanilla only add blocks to the end of the Blocks class? If so the ordering of previous blocks is never really going to change

[warped forge]

the Blocks class currently doesn't seem to be ordered in the same order ancient minecraft had

granite is in position 2 (zero indexed)

no they added bamboo in the middle of it

comparing 1.19.2 vs 1.20.2

so no they don't seem to worry about order

[ripe storm]

Okay, that would imply that there's already some built in system for figuring out the global palette

Probably worth trying to track that down

Because if vanilla is doing that, they'd either have to fix it with datafixers or something - which I don't believe they do - or they already have a system to stop the global palette from causing issues, as vanilla has more than 255 block states. Unless this didn't become an issue till some even higher number?

[winged iris]

Unless they just use DFU and when chunk version is different they fix the id. And then maybe some helper in however they define the fixers to match registry names for that purpose

[cerulean oxide]

I don't see any datafixer that corrects block IDs

[past saffron]

I went and just tested it and it turns out the game still saves a section-specific palette, so the use of the global palette is just a memory optimization at runtime but doesn't affect the saved chunk data

The meticulously assembled test subject 😄

[stuck lava]

maty is this still missing something or did you port everything?

[deep lance]

AIUI the last time we checked this it turns out that chunks never use the global palette on disk

they may or may not be able to use it in memory

though what really matters is network sync

also if we're not persisting ids that's going to have consequences for... chisels and bits is the only mod i can think of offhand

[keen thicket]

there shouldn't be missing anything

[stuck lava]

why do we still use ObjectHolderRegistry if ObjectHolder is gone?

[cerulean folio]

DeferredRegister

[stuck lava]

yes it is only used for updating RegistryObject

maty why did you remove optional RegistryObjects?

[keen thicket]

I didn't. they're always optional now, read the docs smh

[stuck lava]

they are most certainly not... they now always throw when updating and the registry is not present

[warped forge]

ah, neat. that explains why I never saw any serialization for it.

[keen thicket]

oh you mean optional registry

[stuck lava]

Yes that is what RO#createOptional did

[hoary umbra]

the entire concept of RO#createOptional is junk

but if you nuke it without proplerly implementing the rest of DH you are left without the ability to target unbound objects

[heavy zephyr]

can we not implement DH and nuke RO?

in the same PR?

[hoary umbra]

probably

I'm letting maty do whatever he's doing before i go poke stuff

[stuck lava]

The plan is to have a branch on the main repo to collect those prs

[hoary umbra]

was*

[heavy zephyr]

I mean we can all push to maty's branch too

[stuck lava]

No that would be a review nightmare, matys alone already is

[hoary umbra]

I mean DH is already done with review

it just has to be ported

[stuck lava]

After the rebase it needs another review pass

[hoary umbra]

it needs a low complexity sanity pass

the design is finished

[stuck lava]

Still better to do the consolidation branch

[hoary umbra]

Schurli, maty has already gone forward not doing the branch, the bulk of the changes that would be benficial being batched into smaller pieces are already composed in that huge pr

so if that part is not being segmented, the branch isn't really useful

[stuck lava]

Ok then who is volunteering to do the DH stuff on matys branch

[heavy zephyr]

do we have such a manpower problem that nobody can do this or what?

[hoary umbra]

My point is there would be no reason to try and slam DH into maty's branch

let those changes finish and then bring DH

[heavy zephyr]

we're supposed to get rid of RO not add features to it

[stuck lava]

we still need DH because Holders are not lazy but registration is

[heavy zephyr]

yeah we should replace RO by DH

[stuck lava]

if nobody is doing it I'll pull the DH stuff into matys branch (can someone send me a link to the DH branch)

[heavy zephyr]

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

[stuck lava]

since the original DH PR didn't get merged can we nuke RO in the new stuff and replace with DH?

[heavy zephyr]

that is my entire point yeah 🙂

[stuck lava]

should we still do the change with the second type param?

[heavy zephyr]

we need two type params yes, and it is a Holder<registry type> not a Holder<actual type>

(in other words implements Holder<Item> and not implements Holder<MyItem>)

[stuck lava]

new type params are <R, T extends R>

[heavy zephyr]

looks good - is it a Holder<R> then?

[stuck lava]

the question is which type param the methods and fields should use

[warped forge]

this was easier when we had IRegistryEntry :P

[heavy zephyr]

maybe start by commenting out stuff and only adding the methods that makes sense

[warped forge]

would have been DH<R, T extends IRegistryEntry<R>> implements Holder<R> and then people couldn't make a mistake

[heavy zephyr]

do we expect people to instantiate DH directly?

[warped forge]

I wouldn't think so

RegistryObject couldn't be instantiated directly

you either got it from the DeferredRegister

or using RegistryObject.create

[heavy zephyr]

I'm thinking that if you instantiate it manually you'll have T=R

if you let DR instantiate it, it will be able to give the exact R that you wanted

[warped forge]

I assumed manual was like

[stuck lava]

yes for the cases where they don't use DR or want to grab something from another mod without hard dep

[warped forge]

DH.create(Registries.ITEM, "name")

so the base type would come from the registry key

[heavy zephyr]

yeah that is fine, the question is type you get for T (i.e. the specific instance type)

[warped forge]

and the effective type form the field

[stuck lava]

you can always just use a manual type argument

[warped forge]

public class DH {
public static <R, T extends R> DH<R,T> create(ResourceKey<R> registryKey, ResourceLocation name) { ... }
}

public static DH<Item, MyItem> MY_ITEM = DH.create(Registries.ITEM, location("my_item"));

wouldn't this work?

I'm not at home so I don't have a java/modding dev env

[stuck lava]

it would yes in this case it uses the types from the field

[warped forge]

alternatively, create() could return DH<R,R>

and then we don't run into issues where people use wishful thinking

and use a type that makes no sense whatsoever

like <Item, BlockItem> for something that really obviously isn't a BlockItem, but they don't understand how this works

[heavy zephyr]

that's gonna be a skill issue moment I think

[stuck lava]

should the inner holder be a Holder<R> or a Holder<T>? (because unchecked casts)

[heavy zephyr]

Holder<T> should never appear anywhere

[stuck lava]

so Holder<R> and ResourceKey<R> and unchecked casts for get and value

also wouldn't DH break everything that checks for a Holder.Reference?

[heavy zephyr]

you can forward get to value for a single unchecked cast

what uses reference and what uses direct again?

[stuck lava]

reference comes from a registry direct is unbound

ok I checked again the only place it does an instanceof is in MappedRegistry#bindTags

should getKey return ResourceKey<R> or ResourceKey<T>

[keen thicket]

I would also suggest we add DH subclasses for blocks and items. and make DR allow returning a O extends DH<R,? extends R> with a factory

T can only appear in get()T

everything else is R

[stuck lava]

what benefit does that have?

[keen thicket]

less generics. ItemDeferredHolder<BlockItem> is better than DeferredHolder<BlockItem, Item> and people can more easily subclass DH so they can implement stuff like ItemLike on their instance (which a default impl of DH for items should)

[stuck lava]

what do you think about the name DeferredItem (shorter than ItemDeferredHolder)

also your idea would require DR to have an additional type param for the DH

[heavy zephyr]

R because it's the registry type

[heavy zephyr]

not necessarily, the item DR could override a few methods with a more precise return type

ItemLike sounds good but idk if we want to be adding a lot of special cases like that 🤔

because honestly people can just .get() it if they want the item

[stuck lava]

then we would need a subclass for the DR too and I think that is too much specialcasing

[heavy zephyr]

people can do ::get if they want a supplier

[ripe storm]

We could also always return a normal old DH and people can ::get if they want only one generic param

Ope, got beat to it

[heavy zephyr]

hehe

[stuck lava]

Holder is a supplier we patch that in

[heavy zephyr]

yeah but it's a Supplier<R> not a Supplier<T>

[stuck lava]

right... that is a problem

[ripe storm]

People can still use ::get to get a supplier of the more specific type from the DH though, even if the DH is a supplier of the registry type

[heavy zephyr]

yeah exactly ^

[keen thicket]

@heavy zephyr what are the chances you stop spamming one-comment reviews and do it all at a time

there's a reason review comment queues exist

your questions may even be answered to changes below at which point you can delete the questions (i.e. max id and custom display contexts)

https://github.com/neoforged/NeoForge/pull/257#discussion_r1386623132 also yes it's a hack, half of the game is a hack, i'm not sure what point you're trying to make

[heavy zephyr]

I am making observations

[keen thicket]

mkay

you can still observate in bulk

if you're going to go through it all, why not send reviews all at the same time too

[heavy zephyr]

alright alright

[stuck lava]

didn't we also want to change all of the Supplier stuff to Holder

[ripe storm]

I'd say you definitely should

[heavy zephyr]

if supplier works leave it as a supplier

[heavy zephyr]

there you go - more comments! 😄

[stuck lava]

DH is here

[hoary umbra]

Remove the supplier patch from Holder and move it to DH so DH can be Supplier<T>

holder being Supplier<R> is sorta not useful

[stuck lava]

eh I think this is something we either need to check waifu for or ask the community if they used it

[hoary umbra]

Holder is trivially convertable to Supplier

[ripe storm]

I mean, by that argument so is DH

[true tangle]

I think mods should be using Holder instead of Supplier after these changes. Are there cases where using a Supplier is the only option?

[ripe storm]

Anywhere you care about the specific, exact type of the thing, not the general registry type

But even then a DH would work, it would just have more info than is needed

[stuck lava]

there are a few places where suppliers with a more specific type are used which don't work with the holders or DH directly

[true tangle]

That makes sense

[high raptor]

I'm of the same opinion WRT holder-all-the-things but fortunately it's not too hard to turn a holder into a supplier and vice-versa

(the backwards conversion needs a lookup in BuiltInRegistries, or preferably a RegistryAccess/HolderLookup.Provider)

[heavy zephyr]

I think I would prefer getting rid of the supplier patch in Holder

[stuck lava]

rebased and addressed some of techs comments
maty do the rest

[keen thicket]

@heavy zephyr so.. how do client-side tags work on fabric

[gloomy sinew]

does fabric have clientside tags?

[heavy zephyr]

https://github.com/FabricMC/fabric/tree/1.20.2/fabric-client-tags-api-v1/src/client/java/net/fabricmc/fabric

[keen thicket]

i think such feature on neo should probably go as a different PR after the rework

[keen thicket]

tho until then we should probably deprecate/remove the lapis tag and replace all of its uses with the vanilla codepath

[ripe storm]

Given the experimental-ness of neoforge 20.2, and given that it's planned to be added in a separate PR, might not be a big deal

Especially if you'd just be un-deprecating it in just a little bit

[cerulean oxide]

I skimmed the PR briefly, the removal of ForgeRegistries patches everywhere is nice and it should obsolete several ModernFix optimizations

[ripe storm]

It'll also obsolete a mixin into ForgeRegistry in Excavated Variants, so I'm quite happy with it

[stuck lava]

3 review comments left

[heavy zephyr]

I'll do a second pass, hopefully today

[stuck lava]

do the thing pup asked you in #1135328752658829312 first tho

[stuck lava]

for @cerulean oxide and @past saffrons review comments we need some research

[past saffron]

For my remaining comment it's mostly a case of deciding whether we want to do that. The root registry can provide the IDs of the registries, though that would of course require that we also sync the root registry's IDs in the registry sync packets

[heavy zephyr]

@keen thicket don't resolve conversations that are not actually resolved 😛

[stuck lava]

which one?

[heavy zephyr]

the ones I just commented on (and unresolved)

[heavy zephyr]

I think registry registration needs a bit more thought

[keen thicket]

sorry not really sorry

if you wannt to "just comment" for reasons, when reviewing say it's a "comment review"

[heavy zephyr]

yes resolution is required but it's not like we're gonna merge the PR instantly

[past saffron]

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

This might not be thread-safe.
Does it have to be explicitly thread-safe though? Field writes are atomic AFAIK and doing the registry lookup a second time concurrently shouldn't matter

[keen thicket]

yes so why keep the convos unresolved if you're going to end up needing to resolve them

[heavy zephyr]

because some stuff needed more discussion!

I am just a bit annoyed at the NewRegistry event and the related comments

maybe you thought it was resolved and we should keep the event, but I disagree 😛

[keen thicket]

@heavy zephyr I think you're misunderstanding [this](<https://github.com/neoforged/NeoForge/pull/257#discussion_r1389220085): the registry of registries is not thread safe

[heavy zephyr]

the misc comments at the beginning were fine to mark as resolved

you can add the pending registries to a static threadsafe list somewhere

and modify + register them all at once

that is what I have in mind

[heavy zephyr]

hmmm yeah the object never gets modified so the reference assignment should work fine

[heavy zephyr]

so, who is gonna go through the review comments and implement them?

[heavy zephyr]

I can do it tomorrow if nobody does it by then (at least for the simple stuff)

[stuck lava]

for the ones that don't require discussions I can do them the rest is a maty issue

[cerulean folio]

How is this coming along?

Debating what to do with the network logic for registries

Currently I would say that I just port it over from the ForgeRegistry state

But I ma not sure how far along this PR is?

[past saffron]

The network sync in the PR looks basically identical to the old one: https://github.com/neoforged/NeoForge/pull/257/files#diff-1bd6b88990e5f55408fda2fab0d0b2c2ad0a1bcd68cd03c6bf4238f8e7754457

[cerulean folio]

Hmm

I have not looked at it

But this feels heavy......

But okey....

[stuck lava]

The open discussions are:

• NewRegistryEvent
• IRegistryExtension#addCallback
• initCache in NeoForgeRegistryCallbacks.BlockCallbacks#onBake
• IFriendlyByteBufExtension#writeRegistryId

[stuck lava]

@heavy zephyr @cerulean oxide @past saffron @hoary umbra please elaborate further on your comments

for NewRegistryEvent:
I don't know if removing NewRegistryEvent is a good thing since the event at least guarantees that the registries are there before any other mod does any registration and they are not just created right before the mod creating it needs it

[heavy zephyr]

that's why I suggested locking registry creation right before the new registry event is fired

I don't get the point of having the event if the registry can be created statically

(same as the cap rework)

[warped forge]

if NewRegistry event is removed that will majorly impact JsonThings, because I use that event to ensure all my data is loaded. it's the only thing that runs between construct event and registries XD

I know it's a silly use case

[stuck lava]

hmmm could we technically not just fire a RegisterEvent for the root registry?

[ripe storm]

That's what I would do unless there's a good argument not to

[cerulean folio]

That is actually a very good idea

RootRegistry -> Blocks -> Items -> Rest in alphabetical / Random order?

[ripe storm]

Rest in vanilla order

Except modded ones, which should be in order of registration - given that that's determined by mod ordering and event priority

[warped forge]

order of registration could be inconsistent

[ripe storm]

It can't be, because it's the order stuff is fired in

[warped forge]

I mean between runs

[ripe storm]

And that's mod ordering + event priority

Which is consistent

(registry events aren't parallel)

[keen thicket]

this hasn't been the reg order for ages. we haven't been enforcing an order other than the vanilla one

blocks and items haven't been the first since at least 1.18

[cerulean folio]

Okey

Well good to know

Thanks for the update on that 😄

[warped forge]

they haven't?

first time I hear of that

I guess the vanilla order still requires that blocks go before items

[stuck lava]

vanilla moved some things before blocks

[ripe storm]

I'd say:

• root registry
• vanilla registries in vanilla order
• modded registries in order registered - which should be deterministic based on mod ordering and listener priority
  This would also let you trivially register new registries with a deferredregister

And should be easy to do as it'll just be the way it's ordered in the root registry anyways

[warped forge]

oh that's more interesting than I thought

I guess it makes sense that fluids fire before blocks

[stuck lava]

yep and mobeffects before items

[warped forge]

I didn't think biomes were a static registry?

[stuck lava]

eww ordered.addAll(BuiltInRegistries.REGISTRY.keySet().stream().sorted(ResourceLocation::compareNamespaced).toList());

[warped forge]

yes modded registries are sorted alphabetically

but that's fine

mods shouldn't rely on registry initialization order

[stuck lava]

we are currently not even respecting mod load order for registries but alphabetical mod order

[warped forge]

we have holders and "RegistryObject" (until that becomes a holder too)

so for all we care it could be fully randomized every run, and it shouldn't change anything

[stuck lava]

RegistryObject is gone in this PR

[warped forge]

the same is not true for vanilla because mojand isn't using deferred holders

[ripe storm]

Eh, I'd say play it safe and have it happen in a deterministic order, and as it'll already be registered in mod load order / priority from a registry event for the root registry, using that is simplest

[keen thicket]

they aren't

FORGE MADE THEM STATIC FOR SOME UNKNOW REASON

caps

[stuck lava]

huh... mojang is moving to deferred registration

[heavy zephyr]

firing a regular RegisterEvent event for the root registry is shit design, IMO

because registries are special, you can only use the forge builder, and we have a modification event for registries

[stuck lava]

RegistryBootstrap exists

which are stored by registry name in BuiltInRegistries#LOADERS

[heavy zephyr]

anyway

[stuck lava]

can't we patch the registration event to happen in the supplier in BuiltInRegistries#LOADERS?

or in the forEach

[heavy zephyr]

no

mods aren't constructed by then

we can of course change this, but I think it is preferable to keep mod construction happen concurrently with minecraft's own bootstrap

(for startup speed™️)

[stuck lava]

yea Bootstrap#bootstrap happens way too early

[heavy zephyr]

https://github.com/neoforged/NeoForge/pull/257/commits/050f0965a1712105954ead8a48a60c6ffc25997a

was this really necessary??

it's just bloat but whatever

not a hill I'm gonna die on

[stuck lava]

someone requested the item variant that implements ItemLike

[heavy zephyr]

they can just call .get() if they want an itemlike...

this just adds even more lines to a class that's already pretty long

[stuck lava]

get resolves tho

[heavy zephyr]

and when do you want an ItemLike that is not resolved yet and that will not be resolved immediately by the function you pass it to?

[stuck lava]

Idk right now I just implemented a suggestion (I guess those 2 classes can be in their own files but I'd need names for them)

[heavy zephyr]

we shouldn't blindly implement every suggestion though

at least I usually don't

[stuck lava]

I thought it would be nice (also reduces the number of helper functions the modders need)

[heavy zephyr]

I mean sure I guess... the BlockItem function is a bit much though

soon we'll get a PR that adds a method to register a block from some block properties

[stuck lava]

I mean that one is more useful than the one for the block tho

[stuck lava]

there is a helper for registering a block from just the properties

[cerulean oxide]

Right now the registry rework still appears to have the loop which calls initCache on every state, I believe vanilla does it already and it's very wasteful to do so when not necessary because of how slow it is

[pallid quarry]

Having a blockItem which takes a DeferredHolder<? extends Item> rather than a String + Supplier would also be pretty nice. I feel most modders are going to end up implementing that helper function anyway.

[heavy zephyr]

I hate that we'll end up with 50 helper functions in neoforge instead of each modder making the 5 they need or just using the methods directly

[pallid quarry]

Isn't Forge's whole purpose to be monolithic here?

[warped forge]

that's why lex really didn't want oneliner helpers

[heavy zephyr]

the point of forge is to facilitate mod compatibility with other mods and with the base game

not write every trivial helper function for you

[cerulean oxide]

I checked and at least in 1.20.1, vanilla already rebuilds the shape caches whenever tags update

[keen thicket]

we can limit ourselves to just blocks and items as those are by far the most common

[heavy zephyr]

even then you need 20 helper functions

[keen thicket]

everything else people can do themselves now that we allow DH and DR extension

look, it's fine, those aren't high maintenance methods

[cerulean oxide]

1.20.3: renames every class used in those methods

[keen thicket]

ah yes, the big Item rename

Item -> Thing

Block -> WorldThing

[stuck lava]

we don't we only provide the absolute basics... Block -> BlockItem, BlockProperties -> Block (via ctor ref), ItemProperties -> Item (via ctor ref)

[heavy zephyr]

yeah but now you're missing a lot of equally important helpers

whatever ¯_(ツ)_/¯

the IRegistryExtension#addCallback is easy to resolve

(just add a bit of javadoc)

(and decide whether the extra niche method is worth it or not)

[past saffron]

The DeferredRegister specializations are nice. I'm not necessarily a fan of the helper methods for blocks and items, but I'm not gonna die on that hill. What I have a gripe with is the inconsistency in the naming of these methods, the factory methods for the block and item specializations of DeferredRegister should be called createX() instead of just x() (the DeferredHolder specializations already do that) and the registration helpers should likewise be called registerX() instead of just x()

[heavy zephyr]

any reason why DeferredItem has a createItem method instead of it being called create?

[stuck lava]

because generics (the superclass has a static method with the same signature but different generics)

[past saffron]

Static method shadowing should work fine and may be preferable there to avoid confusion

[stuck lava]

I get the error that it does not hide the method

[past saffron]

I at generics

[ripe storm]

Why the fuck is static method shadowing even a thing... I at java

[heavy zephyr]

well it's used in static contexts

so you can do B.staticMethod() and it will call the method on A

don't ask me why 😛

[ripe storm]

Yes, that's cursed and a good way to get weird bugs. Gotta love java

[past saffron]

Looks at every GLXX class in LWJGL extending the next lower one so you can call everything through the class of the highest version you need, no matter the version that actually introduced what you need

[heavy zephyr]

wtf is that 😄

[past saffron]

Jank is what that is

[heavy zephyr]

shouldn't it be register for everything? idk

[stuck lava]

XFact said create so argue with them

[past saffron]

I meant createX() to create the DeferredRegister specialisations and registerX() for the registration helpers in those specialisations

[stuck lava]

[heavy zephyr]

would a plain register not work?

[past saffron]

At least for some of them you'd get generics collisions again on the suppliers

[heavy zephyr]

would you?

if it's an item supplier it might override the parent correctly?

[past saffron]

Stuff like register(String, Supplier<? extends I>) and registerBlockItem(String, Supplier<? extends Block>) in DeferredRegister.Item would collide

The former is the overridden register method from DeferredRegister itself, the latter is a helper

[heavy zephyr]

ah yeah

[stuck lava]

also the Function from RL to thing and Properties to thing

[heavy zephyr]

alright

[gloomy sinew]

I don't think we need registerBlockItem or similar in norg

[heavy zephyr]

OK so not needing a DR makes NeoForgeRegistries a lot cleaner

https://github.com/Matyrobbrt/NeoForge/commit/1725879534a02bbd6b097881320baeda8a7a5cfc

[gloomy sinew]

what do you do to register new registries now

[past saffron]

You just create them with the builder

[gloomy sinew]

public static final Registry<EntityDataSerializer<?>> ENTITY_DATA_SERIALIZERS = new RegistryBuilder<>(Keys.ENTITY_DATA_SERIALIZERS).sync(true).create();

ah, that's sufficient?

[heavy zephyr]

yes

[past saffron]

Yes, calling create() on the builder will enqueue the registry for registration to the root registry and the rest is handled in the background

[pallid quarry]

Side effects in static initialisers is pretty nasty though :/.

[heavy zephyr]

DR is all about side effects 😛

[pallid quarry]

Mmm, but they're all localised within the class. None of the side effects leak outside the static initialiser

[heavy zephyr]

via DR they leak to the registry itself

see the diff:

- static final DeferredRegister<EntityDataSerializer<?>> DEFERRED_ENTITY_DATA_SERIALIZERS = DeferredRegister.create(Keys.ENTITY_DATA_SERIALIZERS, Keys.ENTITY_DATA_SERIALIZERS.location().getNamespace());
- public static final Registry<EntityDataSerializer<?>> ENTITY_DATA_SERIALIZERS = DEFERRED_ENTITY_DATA_SERIALIZERS.makeRegistry(registryBuilder -> registryBuilder.sync(true));
- NeoForgeRegistries.DEFERRED_ENTITY_DATA_SERIALIZERS.register(modEventBus);
+ public static final Registry<EntityDataSerializer<?>> ENTITY_DATA_SERIALIZERS = new RegistryBuilder<>(Keys.ENTITY_DATA_SERIALIZERS).sync(true).create();

[past saffron]

But only at a later point, not immediately in static init

[heavy zephyr]

creating the DR registers an event handler which is already a side effect

[past saffron]

Unless I'm misunderstanding what you mean

[gloomy sinew]

which event? can I use the event to register a registry instead

[heavy zephyr]

no you can't

[past saffron]

No, the registration doesn't happen in the DR constructor, bus registration is manual

[heavy zephyr]

ah yeah ok

well regardless of this theoretical argument about side effects and whatnot, let me know what you think

one of the reasons for this change is to make sure that people can't just create a modded Registry but forget to register it

[pallid quarry]

I don't think this is a theoretical concern about side effects. It leads to really ugly patterns where people end up accessing fields just to call the static initialiser.

I don't see how this is any better than the current approach - depending on where the registry goes its just as easy to forget to register.

[gloomy sinew]

I just cram all my defregs and robjects into the main mod class anyway

[high raptor]

personally, I'm not a fan of the side effect like that

[heavy zephyr]

I can see this being a problem if you declare a registry but don't register anything to it

[high raptor]

I'd rather a .register(bus) method that also creates it at the right time

[pallid quarry]

Or use the registry key instead of referring to the registry directly.

[heavy zephyr]

you're not supposed to have a mod bus in a static context

[gloomy sinew]

yeah but I don't static init my defregs either

[heavy zephyr]

OTOH most of the DR makeRegistry call etc is just noise

and the namespace of the DR is useless if you only use it to create a registry

so DR is definitely not the right way to create registries

[high raptor]

Yeah, I honestly don't know

[heavy zephyr]

what we can do is:

• go back to the NewRegistryEvent
• make sure that all registry builders were registered by the end of NewRegistryEvent

[past saffron]

Then you'd still need to keep a list of active builders around, at which point you're back to side-effects again, making it a moot point IMO

[heavy zephyr]

sure but the side-effect will only matter if you forget to register your registry but still access it

at least you don't need to worry about it for the common code path

we can do this to clean up neo's own registries

[past saffron]

The point they are making is that there shouldn't be any side-effects at all, no matter when those side-effects actually matter since the side-effect itself always happens

[heavy zephyr]

I can see it in two ways:

• side-effects for registration are problematic because they rely on modders getting classloading right
• side-effects bad without further reasoning

in fact, what I am proposing is exactly what mojang does with its intrusive holders - they must be registered to the registry otherwise you get an error when the registry is frozen

[high raptor]

100% about classloading

[cerulean oxide]

good example of side effect-based construction of registries going wrong (albeit with a library mod): https://github.com/Shadows-of-Fire/GatewaysToEternity/issues/29

TL;DR registry gets initialized unintentionally by getting subitems for the creative tabs, if a performance mod skips that the registry falls apart

[heavy zephyr]

yeah that is really bad, we shouldn't let that happen

OK I'll push a revert, then add the safeguard for people forgetting to register their registries

[pallid quarry]

Probably missed some earlier discussion here, but what's the root problem trying to be solved here? Is the main problem that if you create a registry but then never register it then there's no property safety checks, and so things just start throwing confusing errors later on?

[heavy zephyr]

that and also forge's own registry registration being very verbose

throwing a plain exception crashes the game 😒

[heavy zephyr]

@pallid quarry this should be acceptable: https://github.com/Matyrobbrt/NeoForge/commit/e9ef0465625a1e95c08e911414892d098e344ec5

and https://github.com/Matyrobbrt/NeoForge/commit/3011de89b90c777118ead8e0752b49582e9cff42 for forge's own registration

[ripe storm]

I have use cases for creating a registry and not registering it the forge way

Please don't break that

[heavy zephyr]

What now

[ripe storm]

I specifically want to use a registry and not expose it in mod registry events, but end up registering it to the root registry myself later

(That is, not using the registry event; though in theory I don't need to register it at all)

Yeah, this bit: https://github.com/Matyrobbrt/NeoForge/commit/e9ef0465625a1e95c08e911414892d098e344ec5#diff-c258df19a753efb47398b50af9af10482d5682dd066f1dae0cd03547423150bf... that's not great. Is there a reason people can't use a registry for their own stuff, without registering it to the root registry?

[heavy zephyr]

Why?

[heavy zephyr]

No idea tbh - I can only assume it might cause issues

[pallid quarry]

Okay yeah, this looks better, thanks! <3

[ripe storm]

Because I don't want people registering to the registry manually, because I register stuff to it during mod init and handle everything then, including firing the right services for other people to interact with it

[heavy zephyr]

Well I suppose you can create the registry manually instead of going through RegistryBuilder, that should still work

[ripe storm]

I just don't see the reason for this limit on RegistryBuilder...

it'll error anyways when someone tries to register to the non-registered registry, right?

[heavy zephyr]

Same reason why mojang checks that intrusive holders get registered

[heavy zephyr]

Hmmm there's a chance

[ripe storm]

No, it's kinda not - there's additional checks afterwards that handle that for you here... namely, trying to register to the registry

The error should be on registering to an unregistered registry, which in theory should be an error anyways, not on failing to register a registry

[heavy zephyr]

If you can query from an unregistered registry that's already a bad enough problem IMO

[heavy zephyr]

And honestly this is a very strange use case that I'm not sure is worth supporting explicitly

[ripe storm]

My point is that you should be doing a check preventing registering to an unregistered registry anyways, right?

Because otherwise people could, like, accidentally register to a resourcekey for a registry that's never constructed

So if that check already exists or should/will exist... there's literally no advantage to this intrusive holder like check except arbitrarily limiting what people can do with the built in tools

[heavy zephyr]

There is still some merit if people don't register any entry

[ripe storm]

If nobody registers an entry, there's no danger to having an unregistered registry sitting around...

[heavy zephyr]

It's still a programming error that should IMO be caught

That could happen in a library or in forge itself (arguably caught by proper testing but so are a lot of bugs)

[ripe storm]

You're assuming it's necessarily an error though. That's the flawed bit

Given that the case of "nobody is registering to it, and it isn't registered" is pretty rare all things considered... I wouldn't worry about it

Generally that "error", if it is an error, would be caught by someone trying to register to it

[heavy zephyr]

I'll let the others decide on this

[past saffron]

Hmm, thought: could an option on the builder (i.e. untracked()) with a huge warning in the javadoc be an option? I personally think making sure modded registries are actually registered to avoid spurious errors later on is a very good idea and something like this would be the only realistic option in my eyes

[heavy zephyr]

yeah that sounds reasonable

[cerulean folio]

Not just an warning in a jdoc

I would prefer a good big fat warning in the log as well

[heavy zephyr]

might be a bit excessive 😄

[past saffron]

That would have to be dev-only though, in production it would only serve to confuse users

[hoary umbra]

Lets not have a repeat of "potentially unwanted prefix", I've had enough of stupid ass log messages

[heavy zephyr]

that one was quite useless yeah

OK let's remove the blockstate re-caching it doesn't seem useful

what is this for?

[past saffron]

It resolves the lootTableSupplier, which is patched in to support the lazy implementation of BlockBehaviour.Properties#dropsLike()

[heavy zephyr]

why does it have to be resolved immediately?

[cerulean oxide]

concurrency issues maybe?

[past saffron]

No clue whatsoever

[heavy zephyr]

that was added in https://github.com/neoforged/NeoForge/commit/a7df63e1a102363250e0d278b44442fa0f3eb8ee with no explanation

[cerulean oxide]

along with initializing the shape cache

[heavy zephyr]

yeah lol

ok let's remove both

if it turns out it was needed then we will add a nice comment explaining why

[cerulean oxide]

I doubt it was needed since Fabric does none of this

and works just fine

[past saffron]

The supplier can take any of the following forms:

• Supplier of a table name that was explicitly set
• Supplier of a table lookup from another block (vanilla case of dropsLike())
• Supplier of a table lookup from a supplier of another block (modded case of dropsLike() - though the method for that is called lootFrom(), we should fix that)
• Supplier of a name lookup of the block from the registry to construct the default table name (vanilla case, which is moved to the constructor from BlockBehaviour#getLootTable())
  I don't see how any of these could run into concurrency issues, so it should be fine to just have that resolve on the first actual call

[heavy zephyr]

👍

[cerulean oxide]

man it feels good to actually get stuff fixed

[heavy zephyr]

    /**
     * Disables safeguards that ensure this registry is registered to {@link NewRegistryEvent} in due time.
     * <b>DO NOT USE THIS METHOD UNLESS YOU KNOW WHAT YOU ARE DOING.</b>
     */
    public RegistryBuilder<T> untracked() {
        this.untracked = true;
        return this;
    }

does this seem clear enough?

maybe disableRegistrationCheck

[past saffron]

Looks good to me. And, yeah, that sounds like a better method name.
@ripe storm opinion since you brought this up?

[heavy zephyr]

pushed already, lmk if this is not OK

this leaves us with this single todo

[past saffron]

Should basically come down to deciding whether we want to also sync the root registry or not and then adjust the reader and writer to write the registry's ID instead of its name

[hoary umbra]

This particular todo gets into weird territory if we have unregistered registries

[past saffron]

Those registries can't use that method anyway since their entry IDs won't be synced

[heavy zephyr]

I think it's best to keep using the full resloc because 1) it's simpler and 2) if sending the registry name is a bandwith bottleneck then change your code to not need that (or use a more optimized version where you batch entries in the same registry, if you really can't know the registry)

[deep lance]

honestly i'm unconvinced we need the registry id read/write methods at all

[past saffron]

That's fair, yeah. There are "write/read entry unsafe" methods for when you know the registry already which only sync the entry ID

[deep lance]

vanilla has holder read/write methods now

[and the need to write the id of the registry to ensure that it's "safe" is something i've always been skeptical of - we don't prefix primitive types with something describing what type it is, after all]

writeVarIntSafe() { writeByte(TYPE_VARINT); writeVarInt(value); }

just seems masturbatory

[past saffron]

That's a good point. Might as well nuke the registry entry sync helpers entirely then

[deep lance]

vanilla's writeId/readById helpers do everything we need, i think (they do require the registry entry ids to be synced of course)

[past saffron]

The Forge helpers do as well, so no difference there

[heavy zephyr]

vanilla has readById and writeById

[ripe storm]

Sounds good to me, though I feel like the javadoc should actually explain when to use it (i.e., "use this if you don't plan on registering the registry to the root registry normally") so people who need to use it don't have to do as much deep code path searches

[deep lance]

like i can't imagine the use case for "i want to send/receive a registry entry but don't know/care what"

[ripe storm]

The big all caps warning is a bit overkill but eh, not something I care about enough to argue it in the grand scheme of things

[heavy zephyr]

why are you like this

[deep lance]

I guess it made sense when IForgeRegistryEntry was an interface so the sender didn't need to explicitly supply the registry

[past saffron]

On second thought, it may even be dangerous in that it would cause a classcastexception if the registry is not the one you expect

[heavy zephyr]

writeById doesn't work

[deep lance]

I mean, when you are supplying the registry, the object returned is always going to be from that registry and therefore of the type of objects in that registry, the helper just throws if the registry id in the packet isn't the same as the one you supplied

[past saffron]

You need to call asHolderIdMap() on the registry

[heavy zephyr]

writeId works

[deep lance]

do we want a helper that does that? i assumed registries implemented it directly

ah it's IdMap<T> vs IdMap<Holder<T>>

[past saffron]

Right, I'm an idiot, I completely missed the difference between writeId() and writeById()

[deep lance]

and the latter supports direct holders

and both are called writeId on 1.20.1

[heavy zephyr]

me too lol

is this even useful?

it convers resloc -> id for you

[keen thicket]

you can be idiots together!

[past saffron]

\o/

[keen thicket]

||(/s if it weren't obvious)||

[deep lance]

is that gonna silently write the default value's id or throw if supplied a resloc that doesn't exist?

[heavy zephyr]

it's gonna write -1 for non-defaulted registries and 0 for defaulted ones lol

[deep lance]

i don't believe the default entry is actually required to have id 0, but same difference 😛

[heavy zephyr]

let's get rid of it's pretty useless

[deep lance]

i know air has id 0 but i don't know if pigs do

[past saffron]

Yeah, agreed, I can't see many cases, if any, where you don't have the object itself

[past saffron]

It writes the ID of the default entry in the case of a defaulted registry

[heavy zephyr]

we are left with these 3 methods

do we yeet this as well?

[heavy zephyr]

yeah you're right

[past saffron]

Yeet™️

[heavy zephyr]

alright!

[deep lance]

oh i see the classcastexception now

i see the point of view where technically that's "safer" than just silently getting the wrong object, but all sorts of things can happen that we can't do anything about when the packet decoder doesn't match

[heavy zephyr]

@keen thicket @stuck lava with this all the review threads are now closed - please review my changes and if it's all good ping the maintainers for a final round of review

I can already draft a blogpost with a rundown of the modder-facing changes if you want

[deep lance]

so do we want to try to sync the blockstate id map, or at least somehow detect if it's fucked? right now we sync the block registry and rely on the client and server to have the same blockstates for each block, and it can be hard to find what mod is even to blame if they've messed something up by changing the number of blockstates that a block has

[gloomy sinew]

could we do that during the login configuration

[heavy zephyr]

most likely yes, but that should be for another PR

[past saffron]

We totally could, yeah. We probably should consider doing it, but IMO it's out of scope for the registry rework itself and more fitting for the network rework or an entirely separate thing

[heavy zephyr]

probably separate

[past saffron]

Tech, what's your opinion on this expansion of the doc? I'm indifferent about it

[heavy zephyr]

I'd rather be vague about it, there might be more (equally crazy :P) use cases than just that one

[past saffron]

Fair enough 😄

[heavy zephyr]

for the blog post, I have the following changes in mind:

• intro
• use BuiltInRegistries instead of ForgeRegistries
• neo's own registries are renamed to NeoForgeRegistries
• neo's own registries can be used without the .get() call
• registryobject -> deferredholder
• registry creation is a bit different

[hoary umbra]

I have some qualms about the ModifyRegistryEvent but not really enough to block

like the name is weird (it isn't being fired due to modification of a registry) and it fires inconsistently (after bootstrap for normal regs, but before for datapack regs)
It seems like its purpose is just for adding callbacks, and should be named appropriately, and maybe fixed timing if possible (though iirc we can't fire before vanilla bootstrap so I guess ppl will just have to deal)

[heavy zephyr]

Timing is just about the entries, not the registries themselves

It's named that way because you use it to Modify Registries 😛