#Registries Overhaul

1 messages · Page 3 of 1

ripe storm
#

Pretty sure it's illegal because it makes it trivial to get unsolvable type bounds, which messes with inference

hoary umbra
#

then don't write bad code idk™️

ripe storm
#

I suspect there's some more complicated reason about it making type solving exponentially harder or something

#

Anything with super like this can be rewritten with extends, generally, so it's no big deal

quaint pagoda
#

ok so I left and came back, what was decided

hoary umbra
#

dual generic is good idea but bin breaking

#

so we can either put the whole thing off, or leave it single generic and add the second later

quaint pagoda
#

how did you confirm it was binary breaking

ripe storm
#

Another option is to add the second generic to both RO and DH

#

Which would be source but not binary breaking

#

Which may be alright

hoary umbra
#

type erases to DH which is bin break

#

sure I could fix it with a coremod but ew™️

quaint pagoda
hoary umbra
#

that one is source breaking™️

#

idk if we care though

#

@shrewd garden @outer pulsar do we have a policy on source-breaking changes thonk

shrewd garden
#

as long as it's damn near trivial to fix

quaint pagoda
#

well

#

you have to add the base type for every single registry object

#

which depends on what registry objects you have

#

so I wouldn't call it trivial

#

Since you can't do a mass find replace

hoary umbra
#

it would be RegistryObject<T> -> RegistryObject<R, T>

quaint pagoda
#

it's context dependent

hoary umbra
#

or we can just keep single generic with a Holder<X> downcast method for now

#

I guess its an upcast

#

not a downcast™️

past saffron
hoary umbra
#

So yeah, either double generic on both or I add this to DH for now

# 
    /**
     * As a workaround, this method is provided to allow upcasting the generic type of this DeferredHolder to properly use the Holder methods.
     * @param <X> The target type. Should be a supertype of the current type.
     * @return this
     * @apiNote This method will be removed in a future release, and DeferredHolder will have two generic types.
     */
    @SuppressWarnings({"unchecked"})
    public <X> Holder<X> cast()
    {
        return (Holder<X>) this;
    }
true tangle
#

Could the vanilla methods that do not support subtypes be patched to support subtypes, if I am understanding this correctly?

hoary umbra
#

some but not all of them

#

i.e. is(X) can be

#

unwrap(), unwrapKey(), canSerializeIn and tags() cannot be

ripe storm
#

That basically means that we have to extend Holder<Block> otherwise we have issues

hoary umbra
#

in the interest of compat (which was the goal, anyway) I'll just add cast()

#

like

#

this is kinda ugly™️

#

if(BLOCK.<Block>cast().is(BlockTags.STAIRS))

#

granted the vanilla alternative is not much better

#

this.getBlock().builtInRegistryHolder().is(pTag);

#

oh well

#

its a stopgap anyway

deep lance
past saffron
#

That's what I said, yes harold

deep lance
#

it's the first thing on the line so it'll all be at the indent

#

ah sorry i was scrolled

hoary umbra
quaint pagoda
#

one thing i don't like is how disjoint the todos are, how are we supposed to ever find them again?

#

we should standardize a tag for todos for the next breaking change period or something

#

or at least standardize them across this pr so they can easily be found with a project wide search

winged iris
#

TODO - BC: <message>

#

Imo and use that in place of a bunch of todo mcver

quaint pagoda
#

sure

#

as long as its consistent exactly like that

#

I just found a really crufty old coremod

#

"getLanguage_bouncer": addBouncer("net.minecraft.network.play.client.CClientSettingsPacket", "func_149524_c", "getLanguage", "()Ljava/lang/String;"),

#

it's still setup to be loaded!

#

but this class and method obviously don't exist anymore

winged iris
#

Iirc 1.14 to 1.15 timeframe

quaint pagoda
#

should we automatically create a world backup if a user loads and entries are deleted?

ripe storm
#

If there's some system to delete it later automatically if there were no issues - then sure

#

But otherwise, saves can take up a lot of space in general

hoary umbra
#

we used to show a warning before joining the world

#

just bring that back

#

(it got turned into a log message at some point)

ripe storm
#

Warning with a button to make a backup maybe?

hoary umbra
#

the thing is there's already substantial data loss in the current system

#

because it only keeps blockstate data

#

honestly from my experience the user expectation is that removing a mod, loading the world, and then adding it back should nuke all of its content

quaint pagoda
#

at that point I'll just leave a TODO and maybe add it once I guess the rest actually working

hoary umbra
#

yeah honestly dw about it

#

warning screen is nice to have but i think people are aware of the consequences

winged iris
# hoary umbra just bring that back

It went away because mojang made world loading a lot more complicated and no one has had the time/could figure out where to put such a hook back. So thank you for volunteering

#

The bigger issue imo is when game errors but you still somehow make it to the main menu, and you don’t realize it errored so you open the world without the mods present/loaded

quaint pagoda
#

Consumer<Map<ResourceLocation, Map<ResourceLocation, IdMappingEvent.IdRemapping>>> remapsConsumer

#

I feel yucky

lofty citrus
#

...what

#

is that a map in a map screm

#

at what point is it better to switch to a Guava Table thinkies

quaint pagoda
#

this is legacy code jank that I don't want to entirely overhaul rn

lofty citrus
#

ah

hoary umbra
#

what is it? I might be able to de-spaghettify it

#

if this is for the fuckin missing mappings event just delete it

#

aliasing solves that problem

#

this code is haunted Map<ResourceLocation, Map<ResourceLocation, Integer[]>> remaps

#

honestly what even is this for, who needs this event

#

might be something @keen thicket should waifu

deep lance
#

iirc some of the missing mappings code is half broken but in a way that doesn't matter because it was meant for the era when it was important to persist the numeric id map

#

whereas these days that isn't important at all

#

[well, mostly - i think chisels & bits might still rely on it]

quaint pagoda
#

yeah waifu data is good

#

@hoary umbra you can make your own queries btw

#

I might later

#

for now I will just comment out missingmappingsevent it's garbage and too spaghetti to revive

hoary umbra
quaint pagoda
#

I feel a little evil

# 
Iterable<? extends Registry<?>> registries = registryAccess == null
                ? BuiltInRegistries.REGISTRY
                : () -> registryAccess.registries().<Registry<?>>map(RegistryAccess.RegistryEntry::value).iterator()
hoary umbra
quaint pagoda
#

I had to do a little bit of thingies to get biome ids to save

#

I went through and looked at every single vanilla registry manually to find the ones that need syncing of int ids / saving int ids to disk

#

I found that only mob effects and biomes need saving to disk

# 
            Registries.MOB_EFFECT, // Required for MobEffectInstance serialization
            Registries.BIOME // Required for chunk Biome paletted containers```
#

and I documented each one

#

ForgeRegistriesSetup

#

what I forgot to do is make it so I can serialize non-builtin registries

#

so I had to fix that

#

now ModifyRegistryEvent fires for all builtin & datapack registries :)

#

my code needs a lot of testing

#

@hoary umbra is the mods button being back new?? is that from upstream?

hoary umbra
#

ye its upstream

#

are we saving to json or nbt?

quaint pagoda
#

this is in the level.dat

#

so nbt

quaint pagoda
keen thicket
#

yes I hate it too

heavy zephyr
#

What's the problem with it?

keen thicket
#

muscle memory

heavy zephyr
#

would it be possible to have a DR register method that doesn't return a supplier, but rather directly constructs the object?

#

working on the capability rework, I have the following code in a test mod:

    private static final Supplier<AttachmentType<ItemStackHandler>> HANDLER = ATTACHMENT_TYPES.register(
            "handler", () -> AttachmentType.serializable(() -> new ItemStackHandler(1)).build());

the supplier is kinda unnecessary here because constructing the objects is safe

#

it might be cool to have something like

    private static final AttachmentType<ItemStackHandler> HANDLER = ATTACHMENT_TYPES.registerEager(
            "handler", AttachmentType.serializable(() -> new ItemStackHandler(1)).build());
#

just an idea

quaint pagoda
#

not worth it imo

#

there's no way to reliably tell people to not do that for intrusive objects

#

which is like, most of what people register anyways

heavy zephyr
#

yeah I guess it would cause hard-to-diagnose crashes

quaint pagoda
#

you can already do this for your mods and it's called hold the instance in a static field but we wouldn't officially recommend it to anyone cuz it could theoretically break at any moment in a new update

heavy zephyr
# 
    private static final AttachmentType<ItemStackHandler> HANDLER = AttachmentType.serializable(() -> new ItemStackHandler(1)).build();
    static { ATTACHMENT_TYPES.register("handler", HANDLER); }

yeah that works

quaint pagoda
#

@hoary umbra well my registry overhaul works and runs

#

and at the very least doesn't crash with forgedev and saves the ids properly to disk and syncs the ids thru network

true tangle
hoary umbra
quaint pagoda
#

yeah that's already done

#

it also allows chains of A -> B then B -> C but I'm not even sure that's useful

#

oh actually it is because we save aliases to disk

cerulean oxide
winged iris
#

Honestly may just userdev and some bug that makes it rarely not include the mods or something

heavy zephyr
#

could we make intrusive holders thread-safe and unfreeze registries at mod construction time?

#

that might be beneficial for performance because instantiating blocks is quite expensive due to their shape calculations

#

that would also allow us to get rid of suppliers 🤔

gloomy sinew
#

noodogthonk I should put all my voxelshapes in my main mod class

#

I already put everything else there

cerulean folio
#

Do you mean that would allow you to construct blocks outside of registration time?

#

I don't think that is a great idea

#

Cause it will cause several hard to trace issues

gloomy sinew
#

if you have so many voxelshapes that classloading your block classes on the main thread is causing problems, then you can init the shapes on mod construction instead of in block class static init

ripe storm
#

Yeah, that seems like a Bad Idea™️. The unfreezing early or whatnot. The making intrusive holders threadsafe option would eventually be necessary for parallel construction during registration events if that's eventually a thing

cerulean folio
#

Yeah

#

For parallel registration

#

Which we will eventually look into

#

We need the combination of Suppliers

#

And ThreadSafe Holders

heavy zephyr
#

I am thinking about parallel registration in the context of an eventbus rework, and it is a big PITA

ripe storm
#

Things that take a Block and aren't the method registering it generally expect it to be already registered

#

So violating that assumption by passing around unregistered Blocks or Items is a bad idea

#

If you were to allow stuff like that... Why would you even need the registration event? Like, it would be better to do fabric style registration than to keep the registration events but throw out suppliers

#

And that's got a slew of issues, as we all know, even if it comes with some advantages too

#

The suppliers are the advantage of registry events. They let you avoid classloading order issues, dependency issues where you can't make items till blocks exist, etc., in a way where the assumptions vanilla makes still hold true

heavy zephyr
#

I mean, the end goal is to make mod loading faster - I spent some time thinking about parallel registration, but it stops working the moment someone tries to read a registry from the registration event

ripe storm
#

Not necessarily

cerulean folio
ripe storm
#

Eh, you can read the registry during that event just fine

cerulean folio
#

But realistically if your mod is doing that

ripe storm
#

And there's reasons you would have to

heavy zephyr
#

right now you can; but that might change indeed

cerulean folio
#

Then you are already on the wrong path

ripe storm
#

As in, I have a mod that does and there's no alternative

cerulean folio
#

Cause what in gods name would be a good reason to do that?

heavy zephyr
#

he's registering copies of some blocks, kind of

cerulean folio
#

You can do that with a single block

ripe storm
#

Blocks whose stuff depends on other blocks, and you're locked into needing it at block construction due to vanilla constraints

cerulean folio
#

That you always register.....

#

See C&B

#

You don't need to know what is in the registry

ripe storm
#

Hmm? I don't see how that's related

cerulean folio
#

Or going to be in the registry

ripe storm
#

Because for my use case I do

#

Regardless of what C&B does

cerulean folio
#

And if taht is the case

heavy zephyr
#

yes but that is a very complex approach - in theory we could provide a "tail register" event that would allow you to register extra registry entries that depend on previous entries; and that event could run on the main thread

cerulean folio
#

Then we need to patch it out of vanilla and into a supplier

cerulean folio
ripe storm
cerulean folio
#

Cause that is a viscous cycle

cerulean folio
ripe storm
#

But all of this aside, we don't really need parallel registration. We just need parallel construction

heavy zephyr
#

how so? we need to make the common use case parallel, but I think that registering block "copies" is a valid use case

ripe storm
# cerulean folio ?!? Example

The blockstate properties of a block must be known at construction. Period. In my case, those properties are dependent on the properties of other blocks

cerulean folio
#

So I am not sure what the issue is

#

You can pass in a default for the properties

#

And then override the method in the block

#

And pass it along for handling there

ripe storm
ripe storm
cerulean folio
heavy zephyr
#

assuming we fix intrinsic holders, I suppose DR could instantiate the objects in parallel in the backend and then register them serially

cerulean folio
ripe storm
#

Those properties must be provided at block construction

cerulean folio
#

Why would you need some other blocks STATICALLY DEFINED properties in your own?!?

#

All properties are statically defined in vanilla

ripe storm
#

I don't copy them. I add or don't add various properties based on what certain target blocks have

#

And I don't know what those blocks will be at compile time so...

cerulean folio
#

Are you mimicking or what?

#

Cause C&B does all of that....

#

Granted not stuff like Facing

ripe storm
cerulean folio
#

Okey

#

Interesing idea.....

ripe storm
#

I can't work with a single block; I have to register separate blocks for everything

heavy zephyr
#

well luke's stuff would work if we made DRs register instantiate in parallel in the background

ripe storm
#

Also keep in mind that if someone is doing something where they need to access the same register that they're registering to during construction, and expect to get stuff, they're probably working with some sort of registry callbacks anyways

#

Like, I'm doing this with registry callbacks, mostly, except for stuff based only on vanilla blocks

#

(I mixin my own registry callbacks at present cause forge doesn't have them)

cerulean folio
#

I think your idea could indeed work much better with something like a registry callback

heavy zephyr
#

couldn't you instead iterate the block registry right before it gets frowzen?

ripe storm
ripe storm
cerulean folio
#

Then we don't need to care for the seperation of registration and instantiation

#

Cause it means that they should be considered to run in parallel anyway....

#

Or am I seeing this wrong?

ripe storm
#

My thought was that ctors would be parallel, and registration serial, and within registration callbacks would be parallel, to offset the potential performance issues of callbacks

cerulean folio
#

If the callback is already in parallel

#

I see no reason for the actual registration to not be in parallel as well

ripe storm
#

Also, keep in mind that people will still want to read from the register during construction. And that's fine, as long as they only read vanilla entries

ripe storm
#

So, no huge benefit

#

But the big issue with parallel registration is that the contents of the register, during construction, will not stay the same run to run; and, you can't (and shouldn't !) stop people from reading the registry during registration, which would cause risks of modification while reading causing issues

#

Hence, why I'd separate out the ctor stuff

cerulean folio
#

Again, I don't think that reading the registry is a good idea.....

#

I think poeple should use callbacks to do it

ripe storm
cerulean folio
#

No

ripe storm
#

Orion: I can't use callbacks to listen for vanilla stuff to be registered

#

Cause it's already registered

cerulean folio
#

Due to registry replacements, when you are running in parallel, there is no guarantee

ripe storm
#

Registry replacement is dead anyways

#

Thank god

cerulean folio
#

Well there were some reasons to use them, for example with cauldrons

ripe storm
#

But fundamentally, callbacks won't work for stuff where I need to inspect vanilla registry entries

ripe storm
cerulean folio
#

I mean we could offer a snapshot of the state from before the registration process......

#

And that way you can at least see the vanilla blocks

keen thicket
#

registry replacement is gone for good, forge regs existed 60% just to half-ass the buggy registry replacement and it's a lot of patches

cerulean folio
#

Okey

#

Then yeah luke

#

What do you think

#

A snapshot of the registry before the registration process starts

ripe storm
#

Basically, my current flow, for Excavated Variants, is:

  • during the normal registration event, register anything where the dependent blocks are already registered
  • register everything else in callbacks
    I have to do this cause
  • during the normal registration event, lots of stuff I need isn't registered yet in all likelihood
  • callbacks will never catch vanilla entries
cerulean folio
#

Plus callbacks

ripe storm
#

What benefit does that bring over just separating out the ctors?

cerulean folio
#

Not sure

#

Just discussion different approaches

#

I think a dual stage process, is a bit meh

ripe storm
#

It's also... Workable but painful for multiloader mods

#

Cause now I have to handle grabbing that snapshot and whatnot which could be painful, but eh, doable

#

Also, I really want to move away from neoforge providing its own version of registries - ditching the forge registries is a good idea. I worry that a snapshot would be, once again, basically just recreating a vanilla system that already exists

cerulean folio
#

No i mean it should be trivial to get you a nice Map<ResourceLocation, T>

#

Trivially enough

#

It would not be a full registry

#

Just a copy of the contents

#

Cause that is what you are interesting in, is it not?

ripe storm
#

The other option is to make registries backed by a concurrent map of some sort. That way this all sorta becomes a moot point

#

But I also don't know how intrusive that would be

cerulean folio
#

My point is that during parallel processing of the event, the view of the registry might not be consistent from run to run, or even have problems with Concurrent modifications during iterations.

ripe storm
#

Or what sort of performance issues might arise from it later on

keen thicket
#

that sounds slow

#

synchronizing maps with thousands of put calls isn't exactly efficient

ripe storm
cerulean folio
#

You can make the map concurrent during registration and then readonly / immutable during freeze

ripe storm
#

Eh, seems iffy to me

cerulean folio
#

We have the relevant methods already:

#

freeze and unfreeze

#

Simply change the backing map

ripe storm
#

You're poking around in vanilla registries too much

#

We want to try not intrusively modifying the system beyond recognition

#

So yeah, I'd say a snapshot could work, though I don't see a benefit over separating out ctors

#

(also yeah, concurrent backing map has issues of non-deterministic-ness)

cerulean folio
#

I guess that is a good reason to make several PoCs

#

And run tests

ripe storm
#

Separate ctors leads to less possibilities for weirdness in my mind - it's impossible to screw up and access the registry in a block ctor when you shouldn't - something which would lead to difficult to reproduce bugs

#

And neither would really have many user facing changes

heavy zephyr
#

indeed we should run some tests but separate ctors sound promising to me

quaint pagoda
quaint pagoda
#

And also in internal classes

stuck lava
#

why not go through the vanilla entries before the register event and run the callbacks manually

quaint pagoda
#

well that is a possibility I guess

ripe storm
ripe storm
#

Oh wait, you mean forge doing that?

#

Hmm, maybe. That could be interesting

#

Though honestly I don't think it's too necessary

quaint pagoda
#

the parallel registration is entirely outside of my current rework, although it could be added with proper discussion

#

I just feel that's a separate topic altogether

ripe storm
#

Yeah, I think it's a future issue

heavy zephyr
#

I brought it up because I am working on EventBus

deep lance
stuck lava
deep lance
#

idk exactly when would be good

hoary umbra
#

I had an idea™️

#

what if instead of the dual generic that we need to fix holder, we reduce to one generic for the base DeferredHolder class, and introduce a DeferredHolder.Strict subclass with two generics

#

then DeferredHolder instances may be referred to with less verbosity, and those who need stronger typing can use the subclass

stuck lava
#

why? even vanilla does the hacky casting

hoary umbra
#

Vanilla does no such casting, vanilla holders only match the registry type

#

that's why were in this mess in the first place

stuck lava
hoary umbra
#

I assume you just mean why in general?

#

I don't want to manually downcast all my things

keen thicket
#

I have a feeling youre talking about that Class<T> cast back when forge regs had a class type

deep lance
#

vanilla doesn't have this problem because vanilla has statically typed fields [of the correct type rather than the registry type] that are used 99% of the time

#

so maaaayybe the solution is to rework ObjectHolder

keen thicket
#

how exactly does object holder help in any way

deep lance
#

because then mods can use correctly typed static fields too

#

you don't need a Holder<MyBlock> if you can have a MyBlock

heavy zephyr
#

or... go back to static fields 😄

cerulean folio
#

We are not going back to static fields

#

At least I would not......

#

We are planning on doing parallel instantiation

#

With delayed serial registration still right?

#

So....

#

Holder<X> is way better

stuck lava
#

why exactly does the registration need to be serial?

warped forge
#

the registries and some constructors aren't threadsafe

#

you'd have to patch all of that to be threadsafe

#

or you can just have registration happen in the main thread

abstract scarab
#

Even though it does sound like trouble

#

It is an excellent target for parallelised optimisation

#

Its definitely worth at least thinking about

stuck lava
#

I think synchronizing the register method would be the easiest (a single patch)

keen thicket
#

construction would benefit more than registration probably

abstract scarab
#

Disagree

keen thicket
#

that defeats the purpose of parallel

abstract scarab
#

IO will be a bottleneck on its own

lofty citrus
#

i've had the thought of a parallelized registry event holding the registered objects in itself, and then later they all get registered serially

abstract scarab
#

Registration is all in memory objects created from all in memory objects, isolated from each other

#

Its about as perfect a target as we could ask for

lofty citrus
#

technically possible right now with changing the impl for RegistryHelper

abstract scarab
#

The only exception is mods that dep on other mods, but that’s workable

heavy zephyr
heavy zephyr
stuck lava
#

class loading would benefit most

abstract scarab
#

Construction is inherently limited by io, which for many small objects is often a bottleneck of its own

heavy zephyr
#

block construction is also quite slow

#

wdym by IO here?

abstract scarab
#

Disk io

keen thicket
cerulean folio
stuck lava
#

registry stuff doesn't do any IO

abstract scarab
cerulean folio
abstract scarab
stuck lava
cerulean folio
#

Like yeah it creates the default values on construct

#

But the actuall parsing of the file happens way later

abstract scarab
#

Oh construction of the objects..

#

Ok nvm then lol

abstract scarab
#

Well, construction

cerulean folio
#

Given that all jars are loaded into memory by NIO

#

The IO is not really such a problem for classloading.....

stuck lava
abstract scarab
#

I thought they meant data loading

#

Like asset loading

keen thicket
cerulean folio
#

I am going to check

#

But I am like 90% sure that they are loaded later

abstract scarab
#

Then how does default state definition work lol

cerulean folio
#

There is no file needed for that

#

Additionally the blockstate files only exist on the client

heavy zephyr
#

pretty sure it's the block ctor

abstract scarab
#

What..?

keen thicket
#

blockstate jsons aren't state definitions?..

cerulean folio
#

The state definition is not loaded from disk

#

It is done in code

#

There is no IO involved here

abstract scarab
#

I dropped io ages ago and explained it was a misinterpretation

cerulean folio
#

See

#

They are fully in code

#

The Client then loads the blockstate file later during resource reloading

heavy zephyr
#

so, that part should be parallel

cerulean folio
#

Yeah

#

The entire block construction can run in parallel

#

Without issue

abstract scarab
#

I feel like we have different people talking about different things

#

Lol

keen thicket
#

yes.. and that's why we've said block construction is slow. not sure where you're trying to get at thonk

heavy zephyr
#

we all agree that block construction needs to happen in parallel

abstract scarab
#

^

cerulean folio
#

Yes

#

There are several reasons that should indeed happen

stuck lava
#

yes but the question is if and when we need the synchronisation point

cerulean folio
#

createIntrusiveHolder needs to be made parallel capable

stuck lava
#

yea that is step 1

cerulean folio
#

But that is trivial

#

Just turn the map into a concurrent map

#

And that is it

lofty citrus
#

lets get rid of intrusive holders

cerulean folio
#

There i no real performance degradation

keen thicket
#

you basically need to make the holder map concurrent. which is a problem since it slows reads

cerulean folio
#

Becaue the map only holds unregistered holders

#

And as the blocks are registered they are drained

#

And then not used at runtime anymore

heavy zephyr
#

and yes what Orion said is correct

cerulean folio
stuck lava
cerulean folio
#

So the performance on them really does not matter

cerulean folio
#

A concurrentmap is much cheaper

#

That is the line that needs to be patched

#

And done

stuck lava
#

is there a concurrent variant of the identity hash map?

heavy zephyr
#

sadly no

lofty citrus
#

i believe the IdentityHashMap in unique in that regard

cerulean folio
#

What is the key property of that hashmap?

heavy zephyr
#

it replaces equals by ==

#

as long as Blocks etc don't override equals or hashCode we'll be fine]

stuck lava
#

he meant the key type

lofty citrus
#

IdentityHashMap goes counter to the Map interface by comparing with == (identity) rather than with Object#equals

#

ah, map key thinkies

#

see, this is why English is trash

keen thicket
#

T

quaint pagoda
#

let's just trash intrusive

#

:p

keen thicket
#

which is why it should probably still be a identity map

quaint pagoda
#

disable writes entirely and just give it a holder later

cerulean folio
#

No

#

Technicians answer was correct to my question

#

😄

keen thicket
#

otherwise a concurrenthashmap is a further perf degradation

stuck lava
cerulean folio
#

I asked for the key property that makes it special

keen thicket
#

so collections.synchronizedmap?

cerulean folio
#

The answer for the reference comparison is the right one 😄

heavy zephyr
#

and we'll have high contention

keen thicket
#

both of them are right answers stabolb

heavy zephyr
#

what we can do is patch a final equals and hashCode that call super in intrusive holder types (Item, Block, etc) to ensure that mods don't override them

keen thicket
#

the duplicate checks of a normal hash map are going to bite us back

quaint pagoda
#

this sounds like a lot of overengineering when you can just turn off intrusive holders and add them back during registration or someplace later when it's serial

heavy zephyr
#

it's a single patch + optionally some safeguards for mods not to get things wrong... I personally think we'll be fine with ConcurrentHashMap

keen thicket
lofty citrus
#

time for a record ValueHackery<T>(T data) { @Override public boolean equals(Object other) { return other instanceof ValueHackery<T> otherVal && otherVal.data == this.data; } }

#

cursed solutions for cursed problems

#

(fun thing is I did write that kind of code in ParchmentJAM; don't remember if I kept it)

quaint pagoda
keen thicket
#

and if you don't create them on construction they're completely useless since you can't check it the object was registered

#

the only uses of the intrusive holder is to check that the object ended up being registered, that you don't construct them when you shouldn't, and that there's always a holder available in the deprecated builtin holder methods

cerulean folio
#

I think the types that currently use intrusive holders

quaint pagoda
cerulean folio
#

Are reference equals anyway....

heavy zephyr
#

yeah that's what we need to check

cerulean folio
#

So it does not matter whether it is an identity map or not

quaint pagoda
#

well, I guess yes the registry validates all intrusive holders get registered but that's not very useful

keen thicket
#

the registry can't be frozen with unregistered intrusive holders

#

that makes them useful since you at least know somewhere you did something wrong

#

and with debugging you can figure out where

quaint pagoda
#

intrusive holders should be on their way out anyways

#

if the perf hit for the concurrent maps is significant (see: benchmarking) then it should be turned off

#

the features that intrusive holders provide are simply insignificant

keen thicket
#

I disagree with that but w/e

quaint pagoda
#

Personally, I think parallel registration is entirely out of scope until event bus changes are finalized, and the core rework is finalized. That includes moving everything to vanilla registries and reworking all the registry sync/save code to work off vanilla registries aswell.

heavy zephyr
#

The more I think about it the more it seems that eventbus won't change a lot

#

It should only change to support new features imo, to limit breakage

ripe storm
hoary umbra
#

Yeah, DeferredHolder<Block> with something like DeferredHolder.Strict<Block, MyBlock>

ripe storm
#

I don't like Strict

#

It's still a Holder<Block>, right?

#

The only difference is that the get method has a more specific return type

#

But yeah, sounds good to me I guess, especially as DeferredHolder won't be final or anything

#

So the register function returns an instance of the specific one, but that doesn't have to be kept around

stuck lava
#

this will break stuff because for my datagen I use Supplier<? extends X> where X is not necessarily the reg type but that is not possible since Holder is a Supplier (it can not be a Supplier<Thing> and a Holder<RegType>)

#

and no I won't do ::get since the whole point of the suppliers is to be able to just use the RO

ripe storm
#

I think we're talking breaking changes anyways here

#

Since for now there's no alternative to doing unsafe casts somewhere until breaking changes are made

#

Because yeah, at the end of the day it must be a Holder<Block> to avoid tons of vanilla issues, and that means letting people use ::get if they really need a more specific type

stuck lava
#

yes but that would make any DeferredHolder for blocks a supplier for Block and not the more specific type which is a problem for things that require Supplier of a more specific type (one example is FlowerPot i think)

ripe storm
stuck lava
#

then it no longer makes sense to even implement Supplier in the first place if you have to do ::get anyway

keen thicket
#

well yes

ripe storm
stuck lava
#

that is not vanilla but modded stuff

ripe storm
#

Sure, and for those cases ::get would exist

stuck lava
#

::value exists too for vanilla Holder

ripe storm
#

Oh, hmm, then yeah, I say we drop the supplier from holders altogether

heavy zephyr
#

hmmm, assuming we make intrusive holders threadsafe, instantiating them directly seems to end up being the simplest solution

heavy zephyr
#

does the registry rework get rid of the ModLoadingContext.get().getActiveNamespace(); calls?

lofty citrus
#

@hoary umbra and @quaint pagoda? thinkies

#

wait, I didn't need to pong, since they'll read this anyway kek

ripe storm
#

I'd suspect that it does get rid of it, since if I remember right that silly namespace check was done in the forge registry stuff

quaint pagoda
heavy zephyr
#

these two:

keen thicket
#

the 3rd one is so unneedlessly expensive..

#

thread local lookup for every registered object

heavy zephyr
#

well that's just a map lookup

#

the registration does some of these already, so it isn't a big difference

#

also fun fact: the third one is not even used??? help_me

#

nothing is using that RegisterHelper, right?

#

(well, unless some mods decided to)

ripe storm
#

Is there any reason that the new registry system actually needs the mod context stuff at all?

quaint pagoda
quaint pagoda
#

at least in my version of it

#

is this feature going away or what?

heavy zephyr
#

maybe, I don't know

#

it forces any event using it to be dispatched on the mod event bus

quaint pagoda
#

Oh, true

shrewd garden
#

Some people think that the current system (that being, registries locked until register event, and unlocked forever after) is a better system than unlocking until the register event, and locking after.

We should discuss this before we go any further with an overhaul (further than the current non-breaking stage, that is)

#

It's worth noting that mods' static initializers run in dependency order, so mod registry ordering is still preserved with unlocked-for-static-init.

heavy zephyr
#

Surely you inversed locked and unlocked?

keen thicket
#

registries are not thread safe you cannot open up registration at random points during mod loading

shrewd garden
#

Yes I did

#

This is what I get for wanting people to discuss stuff while on holiday

#

There's a very clear disparity between how I remember these discussions going and how they actually seem to have gone

#

So clarity is great

heavy zephyr
#

The first step would be to investigate making intrusive holders threadsafe

#

Once that is done, I think static init as an option would be fine

keen thicket
#

static init yes, you can do it for any registry w/o an intrusive holder (which in 1.20.2 is basically none lol), Registry.register on the other hand

heavy zephyr
#

I think we'd want to keep registries locked, but allow intrusive holder creation

#

That would remove a lot of the current supplier-based patches

shrewd garden
#

Maybe I confused static register and static init

quaint pagoda
#

How it works right now is vanilla registers everything and locks itself before mods can do anything. Then, register event unlocks every registry, runs all the register events, then locks every registry.

#

It makes the most sense to me to keep it like this

cerulean folio
#

To me too

stuck lava
#

me too, I also see no point in changing except being more like fabric

heavy zephyr
#

A big advantage is to remove the supplier-based patches for many things

#

And we also get parallel object construction for free that way

#

The suppliers only existed to support registry replacement in the first place, then it turned out they accidentally helped with the 1.18.2 holder changes; neither of these things is a serious limitation: RR is going away and we need to patch intrusive holders to make them threadsafe already

stuck lava
#

that would technically make DR and RO obsolete which I don't think I like

heavy zephyr
#

DR will still be useful since registration will remain locked to the registry event

stuck lava
#

so DR.register would return the object instead of RO

heavy zephyr
#

RO doesn't need to be mandatory, maybe do two overrides for DR so that people who want RO can keep it

#

Idk if mojang plans on replacing Blocks and Items fields by holders

stuck lava
#

wasn't there a discussion about making entry construction parallel

heavy zephyr
#

Indeed - but enabling static construction makes some things nicer and kinda comes for free once you have the patches for parallel entry construction

stuck lava
#

but it removes the parallel aspect since it would happen in dep order

ripe storm
#

The benefits of static init are, in my mind, twofold: first, it's possible to make construction parallel at some point in the future, though that's honestly not a huge worry for me. The biggest advantage is that it guarantees that stuff is constructed and registered in the same order as vanilla does

#

Which can help avoid all sorts of weirdness

#

And also makes using stuff from other mods easier as a weak dependency

#

Like, if stuff is allowed to be statically registered - and I want to register an item that needs someone else's block, which currently I can do just fine - then there's no guarantee that that is registered when my mod loads. Heck, there's no guarantee that they do it in static init instead of instance init. And it requires a hard dependency, not just getting the block by location

#

As for static construction - maybe

#

The risk there is that Mojang makes a lot of assumptions that things being passed around will be registered

#

And heck, mods do too

heavy zephyr
#

Static construction was fine before 1.18.2

heavy zephyr
#

Well in any case I don't care much - you can fake static init using the block register event if you want 😛

deep lance
#

The suppliers only existed to support registry replacement in the first place

It was also needed for e.g. spawn eggs before we fixed the registration order to do entities before items which I think wasn't until 1.18

also I don't know if registration order is reliable for things like stairs [blocks that depend on other blocks] - something to think about if we're considering removing them

quaint pagoda
#

this idea sounds a bit concerning to me and that mods could stumble upon static init loops that break all assumptions and cause NPEs

#

it would only take two mods for things to go wrong.
Mod A could have a block which depends on a block from Mod B.
Mod B could also have a second block which depends on a second block from Mod A.

Depending on the ordering of the fields and assuming only one class for block init per mod, this could just cause NPEs and people would be forced to move to suppliers anyways.

#

and this probably could get more complicated across registries where mods add new registry ordering which does not exist in vanilla

#

say, requiring an Item definition in a Block constructor, for example

#

you could run into territories where it is impossible to linearly sort registry order

deep lance
#

i mean the most common case is going to be that a mod has a block that depends on another block from itself, i.e. i register a solid block and a stair

#

but i suppose an addon to a mod could register a stair of a solid block from that mod

#

we could have a guarantee that mods are called in dependency order [within each registry, i.e. mod A blocks mod B blocks mod A items mod B items], and that blocks in each DeferredRegister are called in the order they are added

#

and possibly also that multiple DeferredRegisters belonging to the same mod are called in the order that their event listener was registered, if that's not too difficult

hoary umbra
#

I don't believe listeners are ordered in registration order, at least I was having issues with that

deep lance
#

possibly the event would have to be replaced (or augmented) with a system that provides more ordering guarantees

#

like I don't think it would be impossible to get rid of the supplier patches, but it'd need to be done carefully

#

like i am not saying we should try to make all kinds of dependencies possible, just that we should make it predictable which ones work or not. People who need items to construct blocks can use suppliers, the important thing is that nothing in vanilla does it

quaint pagoda
#

that seems like a slippery slope

#

how are modders supposed to know what they can and can't use without a supplier?

#

you would have to effectively memorize the order of registries + how classloading of static fields work and check that with other mods you depend on

deep lance
#

i am not advocating for static init

#

just saying that removing the supplier patches from vanilla classes is worth looking at independent of that

quaint pagoda
#

Entirely fair, actually

heavy zephyr
heavy zephyr
abstract scarab
#

uh..

#

it does though?

#

the vast majority of blocks need to be registered before items

#

entitytypes are also an oddity cus you tend to end up registering egg items for them too

stuck lava
#

except it does because I use my DRs registry order to construct my creative tab

abstract scarab
#

blocks need fluids

#

etc.

stuck lava
#

I think tech has spent a bit too much time in fabric land

#

because "it's not happening on Fabric" is not a valid argument in forge sence we have so vastly different systems

abstract scarab
#

yep

heavy zephyr
heavy zephyr
heavy zephyr
rocky estuary
abstract scarab
#

that is the general implication for eventually™️

stuck lava
heavy zephyr
#

I am talking about the underlying system... I would hope that the high-level API (i.e. DR) isn't changed too much to make migration easy

heavy zephyr
#

the mapping (in Block#asItem) is lazily computed

#

the only thing is that you should not call Block#asItem before both the block and the item are registered

heavy zephyr
past saffron
#

This might be an unpopular take, but I significantly prefer the fully deferred nature of the current implementation, especially when registration helpers come into play to make registration of large amounts of blocks not even more of a chore. One of the cases where the lack of deferral is especially annoying is registration of items referring to multiple blocks (i.e. signs and torches) because it causes field order to matter when it shouldn't IMO. The amount of BS it took to basically replicate deferred and ordered registration and then jam that into things Forge patches to take suppliers was absurd when I dabbled a bit with ML.

rocky estuary
# past saffron This might be an unpopular take, but I significantly prefer the fully deferred n...

Yeah same. I hate having to balance classloading order since many registry entries need other ones to be registered beforehand and so on. I like just registering stuff and forgetting about it. Plus having dedicated steps at which you are sure all blocks have been registered allows for nice things too. Definitely in favor of not doing things the vanilla way of it means keeping deferred registries

#

So I'm strongly advocating to keep the supplier based stuff also because migrating would be a pain otherwise

#

Intact what would even be the benefit of not having that? Seems like just removing something useful just for the sake of removing it

heavy zephyr
#

not having it would significantly reduce the patch surface

#

is it just a matter of personal preference? assuming you split your objects in files by type (i.e. all items in one file, all blocks in one file, etc...) static creation should just work™️

rocky estuary
#

I like having all on one class or generally not having to worry about it. Tbh one of the main things I like about forge

heavy zephyr
#

actually, having all in one class will also work with static definitions because javac will yell at you if you try to refer to a field that hasn't been initialized yet 😛

#

so in that sense it is more robust than just doing supplier.get() in the block constructor and hoping that the supplier is already ready

keen thicket
#

I don't think anyone was suggesting static registration

#

registration isn't construction

past saffron
past saffron
heavy zephyr
#

well these patches don't currently exist for everything - and they sound annoying to maintain

past saffron
#

I can't think of any relevant case where such a patch does not exist

heavy zephyr
#

it doesn't seem to exist for StandingAndWallBlockItem

past saffron
heavy zephyr
#

but if you had static construction javac would enforce the correct dependency order for you

past saffron
#

It would not because it can't see through the indirection of the item construction

heavy zephyr
# 
class MyBlock extends Block {
    public MyBlock(..., Block dep1, Block dep2) {
        this.myItem = new MyBlockItem(..., dep1, dep2);
    }
}

or am I missing something?

past saffron
#

That's not what I'm doing, my implementation is closer to this:

registerBlock(BlockFactory factory) {
  Block b = register(factory.get());
  register(b.makeItem());
}
heavy zephyr
#

ah yes I see

#

could be refactored to not use a supplier with static init 🙂

#

in any case I don't think the existing supplier patches should be removed for now; but maybe the recommended pattern is worth reconsidering

past saffron
heavy zephyr
#

I am saying that you would rely on field order, and javac would check it for you

#

worst case you get a startup crash if you get it wrong

past saffron
#

Javac cannot see through the indirection of the item factory method, making it blow up on startup instead. Field order should not be relevant for this

heavy zephyr
#

"Field order should not be relevant for this" I disagree, this is just your opinion

#

it sounds completely arbitrary to me... maybe it's not the cleanest but it is also really easy to live with / work around

past saffron
#

It is just my opinion, yes, but it has benefits in terms of simplicity and safety

cerulean folio
#

I kind of agree with XFact here.

#

I like the lazy init / deferred init with suppliers

#

I like it even so much that I implemented it Fabric

heavy zephyr
#

hmmm, I have the impression that it doesn't change much on the user side overall, it's just a small matter of convenience - if you're already working with suppliers it's nicer to be able to pass a supplier directly, and if you're working with direct object references it's nicer to pass the reference

#

the thing is that the former requires a number of patches to add supplier support, whereas the latter matches vanilla and thus doesn't require patches

rocky estuary
#

I did have many issues when I tried this stuff on fabric. Sure nothing you can't get by without but I think the matter of convenience is nice. Plus stuff feels less fool proof as you don't risk ending up with null stuff in your object if you clasload wrong

heavy zephyr
#

for me personally (as in for my mods) I don't care as long as we fix intrusive holder creation to be thread-safe... then I can do the static init which I prefer and everyone else can keep doing whatever

heavy zephyr
rocky estuary
#

Having holders instead of registry object sounds like an awesome change tho

rocky estuary
cerulean folio
heavy zephyr
#

Didn't you have to make your own supplier-based versions of some block/item classes?

cerulean folio
#

Where needed yeah

gloomy sinew
#

ANY time a registrable needs a reference to a registrable of the same registry, it should use a supplier/holder to reference it (and we can patch vanilla stuff where necessary)

#

though, how many places does vanilla still have that have blocks having direct references to blocks (or whichever registry)

past saffron
#

The one remaining case that comes to my mind without an IDE is the stairs block

ripe storm
#

In my opinion, the use of static fields for construction/registration - either deferred registers or otherwise - is not the best practice in general because it relies on classloading to have side effects. At least with deferred registers you have to add the register to the mod bus so it's not quite as egregious, since the side effects are then dependent on registering the register, but that's a particular practice that I think vanilla has accustomed to even though it's not too great.

#

But that's also a very opinionated view and I recognize that writing stuff that way is not always convenient

heavy zephyr
past saffron
#

Yes, I was referring to it in general, not in the sense of being an unpatched case

cerulean oxide
#

Isn't static init impossible to ever run in parallel for a single mod?

#

It's limited to parallelizing several mods

heavy zephyr
#

that sounds fine 😄

cerulean oxide
#

Macaw's mods would like a word with you

gloomy sinew
keen thicket
#

for the record, mod bus in static init is completely safe to do as long as you don't go through the thread local container

gloomy sinew
#

which one's the thread-local container thinkies

#

is FMLJavaModLoadingContext.get().getModEventBus() safe or unsafe

keen thicket
#

unsafe

ripe storm
#

How do you get at the mod bus without going through the container?

gloomy sinew
#

what's the safe way to get it

keen thicket
#

get the container from ModList

gloomy sinew
#

ooh neat

#

that'll clean up my code slightly

ripe storm
#

That makes sense

heavy zephyr
ripe storm
#

The paradigm I've been trying to switch my stuff over to is:

  • set up deferred registration on instance init, in instance fields if I want to reference them other places
  • store the mod instance in a nullable static field on the class after construction
keen thicket
#

cast the container to FMLModContainer

gloomy sinew
#

and that exists before my main mod class static inits?

keen thicket
#

no because the container static inits your class

heavy zephyr
gloomy sinew
keen thicket
#

why

gloomy sinew
#

because you said no?

#

I interpreted that as "I can't get the container during my main class's static init"

keen thicket
#

wait nvm the container doesn't use the forName overload that inits the clas

gloomy sinew
#

can I get the mod bus via the container in static init main or not thinkies

keen thicket
#

yes

#

but my words mean nothing if you don't try it stabolb

gloomy sinew
#

right but I wanted to know whether it was likely to work or not before I refactored everything :p

deep lance
#

oh, that's been covered

#

anyway i don't think "jvm enforces it through class loading order" is a valid solution, too easy to fuck up, especially between mods

#

at least if we have a deterministic order then when you fuck it up it crashes in the same place every time and it's clear who is wrong

#

(on the other hand, if we did do static init, most cycles could be safely broken by separating problematic constructs off into separate classes instead of having one singular ModItems etc class - but that relies on modders being able to correctly identify what is problematic)

high raptor
ripe storm
#

But if construction or registration were done in clinit, that's where you start seeing the paradigm that I think isn't the best approach

high raptor
#

Yeah, definitely

#

The main reason I use clinit is because I hate mutability and there's no sane way to make an immutable field and stuff without it 😜

ripe storm
#

I mean, if you do it on instance init and pass around your mod instance to whatever needs it that'd be immutable

quaint pagoda
#

I just pushed a new commit to https://github.com/SizableShrimp/NeoForge/tree/1.20.x-overhaul-registries-cherrypicked which entirely removes registry serialization to disk.

It was actually rather easy because I coded the system in such a way that it is mostly modular and only requires a few hook points to delete, being unified with other codepaths in a sane way.
Basically: fuck the legacy registry system!!

I removed this feature cuz of 23w32a which officially makes vanilla no longer serialize registry int ids to disk. Mods should follow suit.
Since this overhaul is targeting 1.20.2 or later, it obviously makes sense to just get rid of now.

#

Basically the only thing left for me personally is fixing up MissingMappingsEvent / RegistryRemapHandler to actually do something.

#

However, MissingMappingsEvent is kinda awful. I would much rather just delete the system now and readd it later if someone needs it with a properly designed API.
Using IForgeRegistry#addAlias is just such a better system and should be preferred in all cases. I don't really see how people would need more than that, but it's likely someone out there does /shrug

#

so basically: review time maybe?

#

I guess another important thing is waiting for Shadow's PR to be merged with DeferredHolder, and rebasing on that, as that will probably fuck up a good bit of my work

hoary umbra
#

That pr should be good to merge iirc

#

just go review it™️

hoary umbra
quaint pagoda
#

well not in 1.20.1

#

but in 23w32a, yup!

#

even confirmed by mojang staff

#

And this has also been verified by me personally

hoary umbra
#

potions were the final holdover?

#

always fuckin potions screm

quaint pagoda
#

not potions, mob effects

#

there's a difference

hoary umbra
#

same same

#

i know there's a difference

#

but mcp name is entrenched deep in my mind

#

Potion/PotionType™️

#

vs moj MobEffect/Potion

#

I believe missingmappings can be axed entirely

quaint pagoda
#

I agree, for now

#

I wouldn't be surprised if some modders still need it for whatever reason

#

But I'm almost sure that the scope of it does not need to be as large as it currently is

#

and the implementation is just fucking awful

hoary umbra
#

i mean it just lets you do the same thing that aliasing does, unless you have some other need to react to the presence of a saved registry entry being lost

#

but, you aren't saving the registries, so you can't know that anyway

true tangle
#

Do aliases allow a renamed entry to be mapped from its old key to the new key on world load?

hoary umbra
#

aliasing allows you to place an entry in the registry such that, for a resource key A, all Registry#get(A) calls will return value B, which can have any name

true tangle
#

That seems to cover the use case I described then

#

Do aliases already exist or are they something that will be added as part of the registry refactors?

hoary umbra
#

they've existed for like

#

forever

true tangle
#

I cannot find them

hoary umbra
#

until a less-forever-ago PR by me they have been useless

#

and even now they are not api-accessible

#

See ForgeRegistry#addAlias

true tangle
#

Are mods not supposed to use them?

hoary umbra
#

lex wanted it to be "properly" exposed and thus wouldn't let me just make it available via IForgeRegistry

#

I would say its probably stable api

#

actually what the hell let me just go expose it

#

I don't think I want to deal with making it available in defreg because bleh

#

don't want to introduce merge conflicts with the DH PR

true tangle
#

I assume after the registry refactor it will be accessible from all places that regular registration is accessible

quaint pagoda
hoary umbra
#

well you could isolate that out, if you want

#

that's a phase-1 kinda change

keen thicket
#

if you remove saving to disk how can you prompt the "missing registry entries, do you want a backup" screen?

quaint pagoda
#

hrm

ripe storm
#

MissingMappings should be reimplemented in a way that makes sense with the new system eventually but isn't necessarily the most important for now

#

Unless aliases would work for that use

hoary umbra
#

aliases cover everything the mappings event does except notifying you about which entries are missing
but you are the mod whose entries are missing, so you know which entries are missing

cerulean oxide
heavy zephyr
#

indeed

keen thicket
#

yeah the fabric pr is what reminded me lol

#

oh also

#

can we hook into the datafix for effecttype int -> RL and use the data that's saved to disk by forge if exists

#

otherwise you wreck any world updating

keen thicket
#

unless we're registering a built-in biome registry for reasons?..

gloomy sinew
#

slap a deprecated on it and delete in in 1.20.2

keen thicket
#

slap a deprecated on all vanilla "redirects" in that class right now

#

and delete in 1.20.2

#

it's fully binary breaking and the vanilla API doesn't match the old forge one anyways

#

also make sure to test simple forge client/server w/ vanilla server/client

#

also shouldn't this be 257

#

nvm it's 0 indexed

heavy zephyr
#

shouldn't it be 255

#

shouldn't the whole ForgeRegistries class be deprecated?

#

ah no, not Forge's own registries

cerulean oxide
shrewd garden
#

Yeah, lol

#

Being able to modify the game code directly through patches is neat for this kinda stuff

keen thicket
#

hmmm that shouldn't be much worse with a mixin tho

cerulean oxide
keen thicket
#

nope, it's a map lol

#

tho aagh, at the same time you need a world context

#

that's not going to be fun

cerulean oxide
quaint pagoda
#

so I guess I do need to fuck with this more, ugh

heavy zephyr
#

welcome to writing PRs

quaint pagoda
#

all the registries in ForgeRegistries were just a holdover so I didn't have to fix as many compile errors at that moment in time

#

they can be removed whenever

heavy zephyr
#

we can either mark them for removal or delete them

keen thicket
#

imo same field different type will just cause more confusion

quaint pagoda
#

it's binary incompatible

#

so it's pointless to keep

keen thicket
#

mhm

quaint pagoda
#

I did it purely for the writing process

#

IForgeRegistry means something completely different now

#

I wonder if the data fix runs before or after we load registry snapshots from disk

keen thicket
#

we should probably nane it IRegistryExtension

heavy zephyr
#

it should be named the same as other extension interfaces imo

quaint pagoda
#

the name is pretty pointless for mods

#

given that Registry extends it

#

that's all anyone will use

keen thicket
heavy zephyr
#

was that agreed upon by the whole team?

#

I like IForgeXXX 😄

quaint pagoda
#

it can be put to a vote by maintainers /shrug

#

renaming it would maybe be less confusing for modders porting

#

So I might just sway to that

heavy zephyr
#

feels a bit pointless renaming the extension interfaces to me idk

#

probably doesn't affect source code though

true tangle
#

Are the I prefixes staying? I really dislike them.

quaint pagoda
#

yes

heavy zephyr
#

I like them sad

keen thicket
quaint pagoda
fierce wren
#

Can't we do a renaming pass on 1.20.2 for all of them? Once we determine which naming scheme to use, obviously

quaint pagoda
#

With how my rework is currently designed, IForgeRegistry should almost never be touched by mods and everyone should just pass around a Registry instance

heavy zephyr
#

probably best to rename then yeah

quaint pagoda
#

therefore it probably makes sense to rename and explicitly inform modders to use Registry in the documentation of the new class

#

the more pressing matter is the mob effect datafix thing

#

Assuming the datafix runs after the level.dat is loaded (I think it would have to in order to know if datafix needs to run), THEN it would not be too hard to revive that data and inject it into the running copy of registries

#

rather than patching the datafixer to hell

#

And it is also apparent to me that I need to rework RegistrySnapshot to store the list of entry names for every single registry to store to disk.... that's a lot of fucking data

keen thicket
quaint pagoda
#

oh god

#

of course it is

keen thicket
#

lol

cerulean oxide
#

technically that map could be replaced each time a world is loaded?

hoary umbra
#

Generally mods don't support version upgrading anyway

#

plus if the registry is no longer on-disk we can't fix the ID->name mapping

#

because that ID->name mapping is level.dat dependent

glacial galleon
#

so let me get this straight, are we using some form of suppliers now as a registry output or are we just going vanilla style and registering like Fabric/vanilla?

high raptor
#

no, DR gives out holders is the main thing, so it works like vanilla registries

#

but the actual registration is handled as before using the registry events

fierce wren
#

The goal is to move to more vanilla-style registering, but not fully vanilla-style

heavy zephyr
#

you get a holder which is a super-supplier

glacial galleon
high raptor
#

yeah, the forge way of doing registration still exists, it's just more compatible with vanilla now

hoary umbra
#

Yeah I should probably pin this here, it's easier to digest

hoary umbra
indigo pine
#

So is the end goal of the registry overhaul to do away with the forge registries entirely and register things the normal way they're done in vanilla?

i.e.

public interface ModItems {
     Item MY_CUSTOM_ITEM = register("my_custom_Item", new Item(new Item.Settings()));

     private static <T extends Item> T register(String name, T item) {
           return Registry.register(Registries.ITEM, new Identifier("modid", name), item);
     }
     
     public static void bootstrap() {} // called somewhere else
}
``` (how I normally do it)
gloomy sinew
#

nah, the deferredregister apis will still exist

hoary umbra
#

It would basically be that pattern but defreg, you might be able to use static init like that in some cases

#

but objects which depend on other objects existing gets tricky when using static init

fierce wren
#

But will we still be forced to use defreg, or will that end up being a helper?

gloomy sinew
#

and some objects do crash if constructed at the wrong time due to mojang (so defreg helps there anyway)

indigo pine
#

So long as I'm able to have static consts containing my registered objects I think it should be fine.

#

I agree registration order is a concern, though I've rarely every had issues with it.

#

I do have some places where a bootstrap() method calls bootstrap method for other things, but in general it all just sits in the initializer, and since in fabric mods are initialised after the rest of the game, so long as I keep my stuff straight in terms of which references the other, it's okay.

#

As far as vanilla is concerned, I have seen them doing some deferred registration and it largely involves using RegistryKey<?> in place of the public constant values. Particularly anything that's loaded dynamically from datapacks is done that way.

#

(wouldn't be surprised if we see more registries move in that direction in the future, tbh)

#

So if forge's defreg stuff made use of RegistryKey as well, I think that would be something I'd accept

stuck lava
ripe storm
#

The forge registries and the deferred register system are two entirely different things

hoary umbra
#

Importantly you will be a lot more free with how DR works because RO is non-final

keen thicket
#

if the mod is solely about mob effects or whatever then it won't have its data nuked ahem BEs

#

something that we need to test is if the datafix runs with server context

#

because if so we can definitely provide old ids

quaint pagoda
heavy zephyr
#

some numbers on why registry object creation should be parallel

image.png
#

for DR usage (see bottom of the trace) we have 7.5s to construct the objects and 0.4s to register them

#

note that it's not a single mod being extremely slow, but just a bunch of mods doing a lot of work each... no good solution other than parallel creation

image.png
hoary umbra
#

You mean the actual instantiation of the registered things?

heavy zephyr
#

yes

rocky estuary
#

Hm so what's the catch? Or is it just a free load time decrease?

heavy zephyr
#

same catch as parallel setup events: be careful if you access game state otherwise it's free yes

rocky estuary
#

Well so doing stuff like accessing some other mod object to say copy its properties won't work anymore. Previously it was possible since mod load order is a thing

#

Guess not much of a big deal. Would parallelization still be done in steps? Say first register all blocks in parallel and then items and so on

heavy zephyr
#

probably yes

heavy zephyr
#

are mods actually relying on that?

rocky estuary
#

Copying properties? It's nice in some cases for addons and such but we can live without

#

I suppose

#

Performance improvement on load times sounds exciting

heavy zephyr
#

in principle we can respect mod dependencies while still doing stuff in parallel

#

independent mods could still construct their objects in parallel

rocky estuary
#

So like grouping mod dependencies in blocks and running those sync?

heavy zephyr
#

more like building a graph out of completable futures

#

you might run on a different thread than your dependency but you will run after it is complete so it will still be ok

#

details TBD

rocky estuary
#

Maybe would be better to just enforce the new requirements for the sake of performance

heavy zephyr
#

depends what is necessary

#

I'll probably look into it around the 1.20.2 release (assuming we decide that we start BCs there)

shrewd garden
#

do note that parallel events do follow mod dependencies

heavy zephyr
#

no, right?

warped forge
#

I have heard that mentioned but I have also heard the opposite

#

last time I checked parallel events were just conceptually foreachmod(post).joinAll()

#

but that was a few years ago

shrewd garden
#

i remember putting a lot of work into making it work

#

so i sure as hell hope it does lol

warped forge
#

👌

ripe storm
#

Or yeah, mod dependency ordering which would work too

rocky estuary
#

oh true that is also nice

stuck lava
#

is this far enough to just need a rebase once 1.20.2 lands?

quaint pagoda
#

it should really probably be ready before 1.20.2 drops

#

since it is a huge change that we don't want modders to have to switch to mid version

heavy zephyr
#

Same situation for the capability rework

#

Can't we announce the first 1.20.2 builds as "very unstable" or similar? Maybe not even a public release on the maven... To give us some time to land a first set of overhauls

wary delta
quaint pagoda
#

Those are the same thing lol

#

and that's pretty weird to do

stuck lava
#

my idea is you doing the rebase on rc1 next week so it can be merged as a batch (with the update and caps) before anything is released

wary delta
quaint pagoda
#

in my personal opinion

cerulean oxide
quaint pagoda
#

part of testing a mod is putting it in prod/modpacks. Also players would probably be mad if modders can play but they can't and they see modders preparing builds and shit

#

I just don't think it's a good idea to stagger releases between userbases

quaint pagoda
#

if we were to do anything like this, my opinion is that it would have to be the one where we keep it only on GitHub/discord while we refactor things

quaint pagoda
#

the overhaul can wait or if someone wants to take it over

keen thicket
#

the refactors shouldn't come squashed with the port anyways

stuck lava
#

not squashed but the build doesn't run for every merge so if we merge it in right after the update the first build will be a .1 and include the registry overhaul

heavy zephyr
#

Can't we disable publish for a few days when 1.20.2 is released?

cerulean oxide
quaint pagoda
#

this update definitely has some tooling that needs to be done

#

Like turning off SRG and maybe writing a compat layer like maty said

#

And making sure NG6 supports NeoForm

#

And fixing S2S

#

like all that should be done before release

ripe storm
true tangle
#

The same reasoning can be applied to making breaking changes at any point, so I don't agree

ripe storm
#

Nah, because people expect it early on in a release and neoforge doesn't make any guarantees of "this won't break" early on

#

Like, if your stuff has been stable for 3 months and forge decides to break something - that's different

heavy zephyr
#

shadows does your 1.20.1 PR make any sense to have in that version?

#

(probably no)

#

does it make sense to review it anyway?

#

(probably yes)

hoary umbra
#

It is fully workable for 1.20.1

#

Technically it would be nice for something I have in placebo, as currently I'm reflecting into RO to accomplish something that would just work™️ with DH

heavy zephyr
#

am I correct in saying the following:

there are 3 types of holders:

  • direct holders, used by dynamic registries
  • intrusive reference holders, used by the 5 intrusive object types
  • stand-alone reference holders, used by the 5 other static registries
keen thicket
#

the intrusive holder count is probably bigger in 1.20.2

ripe storm
#

Oh god did they start using those for more stuff?

keen thicket
#

yes

hoary umbra
#

"intrusive reference holders" are just ref holders that are a field on the registered object

keen thicket
#

(and we have to make them thread safe if we ever want to be able to construct blocks in parallel)

hoary umbra
#

So there are only two types, direct and ref
and direct ones are worthless because they return false to all the key checks or smth

heavy zephyr
warped forge
#

I still have no idea what direct is useful for

#

like, it can only be used to pass into something which ONLY cares about value()

hoary umbra
#

Yeah, idk
holders seem rushed

#

DH is what I would think holders should be

#

which is a ResourceKey that is bound if present

heavy zephyr
#

yeah it's quite confusing

ripe storm
#

Which is possible, because of wacky codecs and inline definitions

hoary umbra
#

Seems contrary to the point of the holder

ripe storm
#

And registry-or-direct codecs

#

Which give you a holder, that's either a reference to the registry or a direct

#

And can encode the same way. Yes it's weird. I don't try to think about it too much

#

Technically you could probably inline features in a biome modifier or something silly like that if you tried

heavy zephyr
#

needing two generic types is unfortunate

#

won't that be awkward for modders?

hoary umbra
#

datapack regs are fairly cursed

heavy zephyr
#

RegistryObject<MyItem> vs DeferredHolder<MyItem, Item>

hoary umbra
#

like if you deserialize an object with dependencies and discard it, it has side effects that will require the dependencies be resolved even if the object is not retained

#

Did I keep the two types?

heavy zephyr
#

it's a TODO

ripe storm
hoary umbra
#

I thought I just went with cast <X>

ripe storm
#

I'd definitely keep two types

heavy zephyr
#

the cast is gross yeah

hoary umbra
#

right two types is binbreaking

ripe storm
#

If you're going to erase one, it should be the more specific one in my opinion, leaving the generic Item

ripe storm
ripe storm
heavy zephyr
hoary umbra
#

it doesnt matter in most cases

#

Usually the specifics of an item type are just overrides

#

or other registry type

heavy zephyr
#

the generic type of BlockEntityType is quite useful for example

#

would it be sensible to cast the Holder<T> to Holder<R> where R super T?

#

although, having a Holder<MyItem> doesn't make sense anyway

#

what types are holders useful for again?

ripe storm
#

If you need the specific type only -> grab a supplier
If you need the generic type only -> grab a holder
If you need both -> grab the normal old deferred holder

heavy zephyr
#

well there should only be one recommended one way to say declare items or blocks or BETs

ripe storm
#

The others are for the people who don't want two generics because they think it's too much to type or whatever

quaint pagoda
hoary umbra
#

Yeah that's weird

#

the existing intrusives are all deprecated

#

and from discussions with Boq I thought the intention was to remove them entirely

high raptor
#

though this might be a reason to keep RegistryObject, so we can have RegistryObject<RegistryType, RegisteredType> implement DeferredHolder<RegistryType> to make the holder API cleaner, while still exposing the registered type for people who need it

deep lance
#

i'm sorely tempted to say that we should have e.g. BlockDeferredHolder<B extends Block> extends DeferredHolder<Block, B> implements ItemLike

quaint pagoda
#

that's not really possible without providing an overloaded BlockDeferredRegister and etc. in every part of the API that would return DeferredHolder

#

which is pretty yucky

hoary umbra
#

This would give us such capability

quaint pagoda
#

that just looks like overload spaghetti to me

hoary umbra
#

I mean, it is

#

but it means I don't need a DR per registry type

#

which is very nice

heavy zephyr
#

Why not use a DR per registry type though

hoary umbra
#

kinda pointless when I can do that :p

#

I don't need an extra field holding a DR per registry type that I want to register

#

I would need 11 DR's in here