Holders in any sense for datapack stuff make no sense until worldgen registries exist
#Registries Overhaul
ripe storm
hearty urchin
You shouldn’t even need to have to store a reference of biomes. In fact you really shouldn’t. Otherwise the Holder would have to be rebound and it becomes complicated. If someone needs an instance of a biome for whatever reason you grab it with its resource key from a registry access.
Reference the key of the object, not the object itself
hoary umbra
Yeah, in that instance it is fine that DH does not work for datapack regs
those should be retrieved from the reg once available
(if you even ever need those for anything)
hearty urchin
Vanilla does it often. Look at how rabbits determine their variant on spawn based on biome
or foxes
gloomy sinew
DR objects use holders to autowire themselves to each other
hoary umbra
well the rabbit doesn't need a Holder<Biome>, it could just use Biome
gloomy sinew
perhaps
hearty urchin
tell you the truth, that example was pretty terrible
because rabbits only need to know what biome it is (aka the ID of it). They don’t need to know anything about the biome
Perhaps rain was a better example
Rain doesn’t happen in hotter biomes
hearty urchin
or snow in colder
POINT IS. Datapack objects you just need a resource key. No more, no less
quaint pagoda
no that doesn't make any sense to do given this is purely for builtin registries and worldgen registries have their own non-infinite lifetime
gloomy sinew
POINT IS. Datapack objects you just need a resource key. No more, no less
no, sometimes you need both
placedfeatures need the configuredfeature itself
and yeah worldgen registries never get to the root reg
even if they did, like
let's say you make a static deferredholder for a biome that somehow gets the biome value when the worldgen registries are created
hearty urchin
placedfeatures need the configuredfeature itself
yes but configured features aren’t built in are they?
gloomy sinew
if you're playing in singleplayer, does it hold the client biome or the server biome
(they're not the same biome)
hoary umbra
uh
ripe storm
I do have to ask though, what is the process for making a new registry going to look like and how are we going to make it more similar to the vanilla system?
hoary umbra
are all dpregs synced?
gloomy sinew
they are
hoary umbra
then probably the server
hoary umbra
ripe storm
then probably the server
I mean, but then you compare it to the biome or whatever on the client and suddenly they're not equivalent when they ought to be. Gets dicey fast. I'd just say no deferred holders for datapack registries
gloomy sinew
also the biomes on the client don't even have the same data as the biomes on the server
gloomy sinew
I mean, but then you compare it to the biome or whatever on the client and sudde...
but yeah that
hoary umbra
I mean, but then you compare it to the biome or whatever on the client and sudde...
Yeah I believe that would be the correct approach
DH == "core" regs only
gloomy sinew
so, registryobjects but holders
ripe storm
So, anything owned by the root registry
hearty urchin
especially since /reload can be invoked and the new datapack data could delete entries and then weird stuff happens with DH
hoary umbra
so, registryobjects but holders
I mean yeah, the initial goal here was just to make RO implement Holder<T>
but then I was like 
ripe storm
*especially since /reload can be invoked and the new datapack data could delete ...
Datapack registries don't change with /reload, but yeah, otherwise yeah
hoary umbra
that part makes me screm
ripe storm
i mean the entries
Yeah. Datapack registry entries aren't reloaded with /reload
hoary umbra
the fact that DPR doesn't /reload and no mechanism exist to makes them do so is a bit spaghetti
glossy vault
That's only half true. The order only matters for new worlds if forge feels like it
For existing worlds, order is based on order mods originally were in with new ones appended
hearty urchin
Yeah. Datapack registry entries aren't reloaded with /reload
o-O am I going nuts? Thought I’ve had that happen before
ripe storm
Other datapack stuff does but not registries
hoary umbra
You may have for mods that use reload listeners for datapack data
but not for things that are "proper" datapack regs
hearty urchin
perhaps. Alright thanks for clearing that one up
glossy vault
If forge switched to a compound datapacks for all mods like they do for resource packs, it could always matter and save a lot of pack maker headache though
ripe storm
Still a bit curious about this though. Can we somehow make new registry creation use Holders? #1131130393144336385 message
Or otherwise be more vanilla like?
glossy vault
The only downside is you cannot reorder mod datapacks with commands, which no one wants to do anyways
ripe storm
Not quite sure how vanilla does it actually
glossy vault
I should make a difference thread about that probably
hoary umbra
new registry creation is just new MappedRegistry for vanilla
I would implore keeping it that way
hoary umbra
and then registration to the root reg™️
NewRegistryEvent can stay as an early "register your new regs"
but ideally™️ reg creation is
public static final Registry<Spaghetti> MY_REGISTRY = new MappedRegistry<>(...);
syncing and saving (both forge features) will need to either be done via extension methods which raise flags (and become locked at registration time) or via ctor params (annoying for updating vanilla)
ripe storm
but ideally™️ reg creation is
```java
public static final Registry<Spaghetti> MY...
Yeah. Heck, could we use the deferred holder system to build creation and registration in to the same thing? Unless the ctor already does that
hoary umbra
DefReg iirc already has that capacity (to make the registry for the underlying type)
but the return type of that method isn't Registry
ripe storm
Hmm. But I mean, if you're using def reg you're not really using the vanilla system anymore?
But yeah, I guess that makes sense
hoary umbra
I mean in general I would never advise using a pure vanilla system unless you have absolute classloading control (i.e. you declare the registry and all entries in clinit for a particular type)
quaint pagoda
syncing and saving (both forge features) will need to either be done via extensi...
I currently have opted for package private extension methods that you have to use a builder to touch. Making registries as early as possible makes more sense after the conversation I read thru. But people will definitely still do it wrong (as in not register it). At least we can make it error though.
hoary umbra
I mean that's fine
public static final Registry<T> REG = Registry.builder().blah().saved().build();
How would that error exactly?
quaint pagoda
It's easy
hoary umbra
it doesn't need to error if its only accessible via the builder, lol
ripe storm
Like, we should still allow creation of registries unlinked to the root registry. Heck, vanilla uses that for world gen. I'd say it would error when someone tries to register to it
quaint pagoda
new RegistryBuilder<>(KEY), check if KEY is registered to root registry later
ripe storm
new RegistryBuilder<>(KEY), check if KEY is registered to root registry later
Oh, you mean with the builder. Yeah, fair
I figured even that might not be necessary as we could just watch for people trying to register stuff to registries that don't exist (not registered )during the registry events
quaint pagoda
better safe than sorry
ripe storm
But yeah, using the builder is clean
hoary umbra
you don't need that many sanity checks, DH will just explode if the registry isn't registered
ripe storm
Especially if you expect most people to use that
hoary umbra
and it will say that explicitly "no registry exists"
ripe storm
(is this all package private in vanilla normally?)
hoary umbra
I don't think many things in vanilla are package private
ripe storm
What is the access modifier on making a new registry in vanilla?
hoary umbra
well, the helpers are private, but you can just copy them
ripe storm
But the actual constructor is public then?
hoary umbra
so in literal terms, public™️
ripe storm
Then yeah, we don't need any special checks in the builder, if DH would error anyways and if they can always just use the constructor straight away
hoary umbra
In general I always disliked the excessive amount of defensive programming
ripe storm
Yeah, I'm with you there
Doesn't ever protect as much as it claims to, and makes it way harder for the one guy who actually has a reason to dig under it
hoary umbra
Unless its in netcode
keen thicket
also, slight detail.. has anyone checked the thread safety of holder creation
which I'm going to assume isn't safe
hoary umbra
normal holder creation? no, not a chance, lol
gloomy sinew
getOrCreateHolder is a write operation on the vanilla registries and vanilla registries are not threadsafe
could frozen registries be made threadsafe?
I guess if all write operations are forbidden then they already would be
ah no, reloading tags is a write operation on... something
ripe storm
They're thread safe during common setup and after registry reloads though, right?
gloomy sinew
maybe, unless somebody's cheesing things into the registries post-registration
(which I suspect is something that both of us do)
okay, tag reloading is an atomic operation on the registry's tag map, but mutates all holders iteratively
ripe storm
(which I suspect is something that both of us do)
Nope! I do it via a mixin to forge registries to do it during the proper registration. Stuff like content tweaker does do stuff afterwards? But I think they do it on freeze, before common setup. Not entirely sure
gloomy sinew
I unfreeze the dimension registry during game runtime 
https://github.com/Commoble/infiniverse/blob/main/src/main/java/commoble/infiniverse/internal/DimensionManager.java#L156
ripe storm
Heh. Well, yeah, fair enough
I'm excited to hopefully get registry callbacks as a way around that rather ugly mixin I currently use
gloomy sinew
half the reason I originally wanted this to be an official forge feature was so people wouldn't assume the dimension registry was immutable (because immersive portals was caching it for some reason)
quaint pagoda
getOrCreateHolder is a write operation on the vanilla registries and vanilla reg...
hrm
looks to me like people should always use DeferredHolder then
ripe storm
Within a parallelizable context I'd say yeah, it's probably safer
hoary umbra
looks to me like people should always use DeferredHolder then
I'm a little bit worried that DH's excessive bind() calls could result in a CME if they are reading the holder map from the reg during holder creation
quaint pagoda
I haven't looked at your implementation at all
hoary umbra
if we get registry callbacks we can register all DH's as callbacks and then that won't be an issue
technically™️ the registry step is singlethreaded so it shouldn't be an issue™️
quaint pagoda
if we get registry callbacks we can register all DH's as callbacks and then that...
not really
even though people can statically create registries
those registries won't be registered immediately
therefore there won't always be a registry to lookup to register to
hoary umbra
all registries will be available after NewRegistryEvent, which DH's can listen to and bind
quaint pagoda
that's exactly what ModifyRegistryEvent is for
quaint pagoda
PoC is pain
I'm thinking phase 2 would be a nice idea
do a role reversal and have forge registry be a wrapper on registry<?>
keen thicket
PoC is pain
I hope that's not used with the other meaning
quaint pagoda
stupid to mention that
queen notch
Phase 2 will be great, just lets not repeat Thor: Dark World okey?
hoary umbra
https://github.com/neoforged/NeoForge/pull/35 is ready for merge unless anyone else has any reservations
winged iris
I will try to take a look at it in the next couple days, but I think you need to rebase as the jar compat check is failing (as things have been added that aren’t in your pr due to older base)
hoary umbra
screm
compat check should probably run a local rebase first
or, well, wait actually
doesn't the compat check failure on any method removal, not just api-breaking changes?
wondering if its picking up removal of the package-private methods or smth
keen thicket
I will try to take a look at it in the next couple days, but I think you need to...
thing is, I technically fixed it doing that
hoary umbra
oh well, rebase is clean, so that took 3 seconds
CI at my work runs a rebase before doing any pipeline things, but I'm not sure how difficult that is for our infra to handle
winged iris
thing is, I technically fixed it doing that
I thought so too, but like why else did it fail and claim those things?
past saffron
<https://github.com/neoforged/NeoForge/pull/35> is ready for merge unless anyone...
Looks good to me. The only thing I think is not ideal is the constant pointless registry lookup in value() (https://github.com/neoforged/NeoForge/pull/35/files#diff-23a6a6d322a2b82410f4bf00fdc3fb70ec82e2956980dbf6c896a06b36a0b342R90). I think a throwOnMissingReg flag passed to bind() would be better
hoary umbra
hm, yeah
quaint pagoda
doesn't the compat check failure on *any* method removal, not just api-breaking ...
No it's set to api mode
hoary umbra
oh, neat, didn't realize that was a thing
hoary umbra
I think I recall it complaining about removing a non-api method at some point
quaint pagoda
it should respect api status internal now I think
hoary umbra
bleh, I've realized I want to write something but it depends on defholder existing
winged iris
just write it against maven local and then wait a day or two to commit 
quaint pagoda
@hoary umbra why isn't there a factory method that simply takes a resource key?
hoary umbra
uh
idk™️
because you can't make a strong resource key on your own, its a huge pita
and the expectation is that your DH is strongly-typed to your object type
quaint pagoda
the docs probably shouldn't be using <br> but we haven't standardized that yet
and also I don't think there should be spaces between the doc and the method or class start
hoary umbra
is <br> illegal now 
deep lance
because you can't make a strong resource key on your own, its a huge pita
what does this mean?
quaint pagoda
is <br> illegal now <:thonk:932605815221788672>
nah
deep lance
can't you use a dangling generic?
hoary umbra
I've tried everything I can think of, the issue is in the signature of ResourceKey.create
it only returns a type of the registry type
deep lance
registry keys are ResourceKey<Registry<T>>, not ResourceKey<? extends Registry<? super T>>
hoary umbra
they're ResourceKey<? extends Registry<T>>
quaint pagoda
yeah it's certified awful
quaint pagoda
registry keys are ResourceKey<Registry<T>>, not ResourceKey<? extends Registry<?...
r u sure
that doesn't sound right
deep lance
hoary umbra
anyhow we can leave the javadoc formatting up to #1131782966989828156
quaint pagoda
I thought vanilla used ? extends Registry<T> for everything
hoary umbra
it does
deep lance
i think maybe they used to have that wildcard mess in, like, 1.18.2 when i first looked at it
hoary umbra
obviously the declarations aren't
quaint pagoda
anyhow we can leave the javadoc formatting up to <#1131782966989828156>
I find it unlikely that we will be autoformatting everything esp javadocs but yeah
quaint pagoda
obviously the declarations aren't
meh nothing in this system is obvious
the declarations could be if Mojang wanted
hoary umbra
The javadocs in there are autoformatted by my eclipse profile
well the decls are decompiled, so who knows
quaint pagoda
true I guess
deep lance
honestly
i'd be half tempted to make DeferredHolder take two types
the registry type and a more specific type
hoary umbra
yeah but that achieves nothing, lol
it doesn't fix the unchecked cast and it just makes all declarations longer
deep lance
it lets DeferredHolder<Block, MyBlock> implement Holder<Block> so it can be part of a set of Holder<Block>s
hoary umbra
it already can be, wew type erasure
deep lance
i still don't understand the unchecked cast
hoary umbra
I left the compiler error in the pr
deep lance
or why the type can't simply be ResourceKey<Registry<T>>
hoary umbra
It could be, but that doesn't fix it
? extends Registry<? super T>> is just the biggest "umbrella"
deep lance
how doesn't it fix it
voila fixed. only the ? super is problematic, even. and I don't understand why it has to be there.
hoary umbra
now get me a ? extends Registry<MyBlock>, lol
you've just moved the cast location to outside the method
deep lance
but Registries.BLOCK's type is ResourceKey<Registry<Block>>
hoary umbra
yes, so now create only makes DH<Block>
deep lance
public DeferredHolder(ResourceKey<? super T> resourceKey) {
// ...
}
public static <T, U extends T> DeferredHolder<U> create(ResourceKey<Registry<T>> registryKey, ResourceLocation valueName)
{
return new DeferredHolder<U>(ResourceKey.create(registryKey, valueName));
}
bam
hoary umbra
Hm, I suppose we can bypass the type on the key like that
deep lance
like ultimately the type of the key is block
this does mean you have an unchecked cast in the value method, but that's the method that actually has the potential of returning the wrong type and will have a checkcast in its caller
like, ultimately the question to consider is
what actually happens if someone creates a deferredholder for a type that doesn't match the actual type placed in the registry
no matter how the unchecked casts are arranged, at the end of the day the return value of value() is caller-checked
though an unchecked cast is a little less ugly than a raw type at least
quaint pagoda
I could just make the ctor public if you want a reskey one
No just make it a factory like it should be
quaint pagoda
```java
public DeferredHolder(ResourceKey<? super T> resourceKey) {
...
I see u stealing what RegistryObject already does
hoary umbra
top 10 illegal generic techniques
abstract scarab
We got a summary on the end-goal for this?
Like what an average user’s registry process would look like
hoary umbra
Well, I was writing some stuff up in placebo that will be cleaner when DH is present, but I was thinking just like this https://i.imgur.com/vIWbqfG.png
currently I have to do some fairly ugly reflection to make this possible, lol
hoary umbra
pretty much
this change makes RO a Holder<T> so it can be used in Holder places
RO is currently really bulky
its just a huge pile of overengineered spaghetti
abstract scarab
I see
hoary umbra
this is basically the first step in "delete ForgeRegistry"
abstract scarab
I was expecting something actually useful from a registry overhaul in neoforge
hoary umbra
as it also makes all registries vanilla-backed
well there will be™️
but not yet
because it requires changing so many internals
surreal turret
one step at a time
hoary umbra
we are still at non-breaking-change phase
abstract scarab
Is there a reason it needs to be not yet?
hoary umbra
non-breaking-change phase™️
abstract scarab
Then defer the change for breaking so you can do it properly?
abstract scarab
This seems rushed for the sake of it
hoary umbra
why would I defer this part until later?
abstract scarab
Idk I’m just coming in off the periphery
hoary umbra
Ultimately this is mostly an internals thing
abstract scarab
If this is 100% non breaking then I guess its ok from a dev perspective
hoary umbra
the registry system is a horrible unmaintainable mess
abstract scarab
If theres any amount of change required from the dev side for this its an outright disappointment from my perspective
hoary umbra
probably not significant differences to the typical end-user
abstract scarab
Not significant isnt none..
hoary umbra
I mean the class ForgeRegistries will vanish
abstract scarab
So this specific pr is typically 100% binary compatible with current mods (ignoring edge cases)
?
hoary umbra
yes
abstract scarab
Ok cool
hoary umbra
this is outlined in https://github.com/neoforged/NeoForge/issues/37
winged iris
If theres any amount of change required from the dev side for this its an outri...
I mean as long as you don’t count “deprecated for removal” warnings in your ide as change required… there shouldn’t be any… I mean unless you are reflecting into registry object internals… but then that is kind of on you
rocky estuary
Without looking at the internals what consequences will this change have for most everyday uses beside not referencing forge registries anymore?
high raptor
you'd be renaming RegistryObject to DeferredHolder or something to that effect
rocky estuary
And that's it?
gloomy sinew
Without looking at the internals what consequences will this change have for mos...
- continue using deferredregisters to register stuff
- DeferredHolder instead of RegistryObject
- query vanilla registries when actually reading registries (I already do this anyway)
high raptor
using registry keys more when creating deferred registers too, i think
since deferredregister.create or whatever has like three overloads
rocky estuary
So from an outsider prespecrive it's just a name change
lofty citrus
RegistryObject still remains, but it's deprecated and it's only a light wrapper over DeferredHolder
it'll get yeeted come breaking changes time 
rocky estuary
Right so it's not a breaking chsnge for first phase:TM:
deep lance
RegistryObject still remains, but it's deprecated and it's only a light wrapper ...
what if RegistryObject is an abstract class that DeferredHolder extends
past saffron
What would be the point of that? The idea behind DeferredHolder is to keep the lazy init of RegistryObject while being a Holder, in part to make multiplatform stuff simpler
deep lance
for backwards compatibility, i had it in my head that if we do that then DeferredRegister.register can return an actual DeferredHolder rather than a wrapper
actually is there a need for there to be two classes at any time? why couldn't we just make RegistryObject implement Holder, and wait for breaking to rename
past saffron
for backwards compatibility, i had it in my head that if we do that then Deferre...
That would be a signature change and therefore not binary compatible. DeferredRegister.register must return RegistryObject until the next breaking phase. RegistryObject is also not a wrapper
past saffron
actually is there a need for there to be two classes at any time? why couldn't w...
Ease of discovery by virtue of deprecation
hoary umbra
re: https://github.com/neoforged/NeoForge/pull/35#pullrequestreview-1554878082
@winged iris / @quaint pagoda does vanilla do registry sync at all? I thought vanilla did not perform sync of any kind with regard to registry content
quaint pagoda
I am pretty sure the answer is no
but you would have to ask pup how that comment came about
keen thicket
it doesn't (need to)
winged iris
I haven't had a chance to look through the code and realized that maybe that is something we have to worry about
quaint pagoda
why do you think vanilla does? In my eyes we would have already removed the forge feature if vanilla did this
keen thicket
why would vanilla do it in the first place 
quaint pagoda
to be nice :>
keen thicket
vanilla? nice? funny joke
gloomy sinew
yeah I'd be very surprised if vanilla does any registry syncing in the static registries
keen thicket
it doesn't make sense for vanilla to sync registries when they're static init'd smh. useless packets
gloomy sinew
even the numeric ids should be consistent in vanilla
keen thicket
numeric ids are the thing you need to sync/persist
quaint pagoda
https://github.com/neoforged/NeoForge/pull/35/files/bfc02f7771326c0b3d4286252a0a68f8c1e38277#r1279514088 @winged iris I don't see how this replaces the feature but also I don't think anyone was using the feature. @hoary umbra can you explain how it works now for optional registries
gloomy sinew
right but I mean in vanilla they're probably consistent on both sides without needing a sync
keen thicket
yes
hoary umbra
All DH's support optional registries
just don't call .get()
because, obviously, the item is not present since the registry is not present
winged iris
and my comment was just asking to make sure whether or not the binding would cause issues
I thought it wouldn't but wanted to check
hoary umbra
no bc the event doesn't fire for registries that don't exist
winged iris
given the fact that the registry event shouldn't fire if the registry doesn't exist
I know
I just wanted confirmation is why I asked
hoary umbra
I mean technically it could, I guess
there's no sanity checks in the signature to make sure the registry is present
the ctor should check at least one of the regs is present
and the names used are based on this weird patched-in MappedRegistry.getKnownRegistries()
winged iris
that's so that it is in a consistent order and one that follows vanilla's init order
quaint pagoda
All DH's support optional registries
that sounds bad
how are you supposed to ensure your entries registered even if you don't want to query them
hoary umbra
why...?
gloomy sinew
well
hoary umbra
isPresent
quaint pagoda
like by default I think we should error if an entry isn't registered when the user expected it to be registered
which is already what happens
gloomy sinew
if they're my own entries, for vanilla's registries or my own registries, I can reasonably assume they're registered without checking isPresent
if my own entries didn't register then something went horribly wrong and it should crash very fast
quaint pagoda
it's not always going to crash fast though
winged iris
that's so that it is in a consistent order and one that follows vanilla's init o...
I remember figuring out and writing the impl of MappedRegistry.getKnownRegistries with lex at some point because there were various issues (namely with configured and placed feature ordering)
gloomy sinew
those're datapack registries now though 
quaint pagoda
I'm curious if the deferred register not being registered throws an error if the registry object doesn't detect itself as registered (right now without this PR)
quaint pagoda
`isPresent` <:thonk:402571441716854794>
not all entries are needed to be queried with get() but are still necessary to be registered
e.g. command argument providers
winged iris
those're datapack registries now though <:thinkies:932605814995292192>
so? The point still stands that some registries interact with each other and firing based on vanilla ordering makes it so that you can at least consistently reference stuff
quaint pagoda
I'm never gonna need to query that entry but it still has to be registered
gloomy sinew
also the way vanilla holders work is that if a holder was created for a registry but the value was never registered/bound, the registry throws on freeze
winged iris
and those were the two most common ones when we wrote that bit so that it fires in that order
hoary umbra
if the registry exists then the element will be registered, unless you don't register your DR, in which case it just fails anyway
quaint pagoda
the point of the createOptional things was by default things explicitly error if they don't get registered
hoary umbra
all createOptional did was error check if the registry exists, you can do that yourself
quaint pagoda
rip yeah I guess that's all it did
I guess it's not worth keeping lol https://i.imgur.com/ZEAJMCz.png
huh, cofh core is using it https://i.imgur.com/U5PbB3i.png
nvm it's just static wrapper methods that are never actually used
see these people are using it actually correctly https://github.com/TwelveIterationMods/Waystones/blob/d0b9024a8fac9c6236d88c673d305d51bfd38ecd/forge/src/main/java/net/blay09/mods/waystones/compat/RepurposedStructuresIntegration.java#L13
this is a lot easier than declaring an optional dependency on repurposed structures to ensure their mod constructor runs first so that you can do a ModList check from your constructor to optionally register the Deferred Register
actually I guess you don't have to sort your mod constructor to know if a mod is loaded
so meh it's only like a one liner difference to change the DR registration, theoretically
it's a pretty niche case anyways because the custom registry has to not use custom mod class types for it to be useful at all anyways without requiring a ModList#isLoaded check before loading the class and getting massive classloading errors
anyways all that to say I guess it makes sense to get rid of as it's not doing much at all
hoary umbra
Yeah if it was doing something else that was potentially more useful I could see it being helpful
but its just a very obscure "does registry exist" check
marking that comment as resolved
quaint pagoda
maybe we should link back to this convo as evidence
hoary umbra
done™️
winged iris
fairly certain it is fine, not marking it as approved yet (may not until tomorrow or something idk) as I want to test it against say mek to do an extra level of testing/validation that it doesn't cause any compile or runtime breaking changes
hoary umbra
yeah, not 100% certain on the impact of everything having a vanilla registry
pretty sure it's harmless
gloomy sinew
"what's the worst that could happen?"
winged iris
well that part I don't know, especially given my custom registries 4/5 of them have vanilla registries already
quaint pagoda
@hoary umbra <br> is fine in brand new javadocs but you shouldn't really mix it into javadocs that have an existing standard being used until we get an autoformatter or defined standard somewhere.
e.g. https://github.com/neoforged/NeoForge/pull/35/files#diff-4b3668674d37f99def6bc1719fad4898bf978e0dd0037ba59b2d60860ce3dc75R57
that's just causing 2 blank lines for no reason
I have thoughts on this. this is a bad javadoc in my eyes.
Instead, we should be pointing people to ForgeRegistries$Keys class.
Additionally, we should deprecate all keys that point to vanilla registries and link back to the vanilla key, and have people use that, as they will be deleted.
so, most of the keys in ForgeRegistries$Keys should be deprecated and disappear soon
I can also move these to github, probably should
I did that
hoary umbra
I think messing with the key deprecations is out of scope for now, I was going to do another one which shuffles the deprecation fields from the vanilla stuff to the forge stuff
quaint pagoda
I think messing with the key deprecations is out of scope for now, I was going t...
Ok well at least point to ForgeRegistries.Keys instead of suggesting getRegistryKey
hoary umbra
yeah that's fine
hoary umbra
Not sure why the dislike for <br> and <p> in the same thing, they have different purposes
I use br to split sentences to their own lines, and p to split ideas into their own paragraphs
though any cases of <br> with an immediate <p> following are just me being stupid
abstract scarab
^
people have a go at me for doing the same thing lol
br and p format differently, so I use them differently
quaint pagoda
I wasn't aware of that. Different how?
quaint pagoda
Not sure why the dislike for `<br>` and `<p>` in the same thing, they have diffe...
the thing is most of our codebase already isn't using br
hoary umbra
most of our codebase is using whatever the writer decided was good enough, lol
quaint pagoda
mm fair most of the time I find p and br to be equivalent and stick to p for larger separation but that's new info to me
hoary umbra
i.e. this one
the first three are <br> but the space between time. and Example is <p>
if I just drop the <br>'s it reads like this which is just harder to parse imo https://i.imgur.com/v91HhAM.png
quaint pagoda
difference of opinions then
I think adding a line break after every sentence is unnecessary
hoary umbra
the first three sentences are disjoint but the not disjoint enough that they warrant a new paragraph
it is unnecessary if the sentences form a coherent thought
keen thicket
let's poke @lofty citrus for a qualified opinion
quaint pagoda
yes that sounds good
he can do a formal writeup which we can all vote upon if he wants :p
hoary umbra
In absolute terms I use
<br> for disjoint sentences that are part of the same thought
<p> for disjoint thoughts
quaint pagoda
or we can just trust him and shipit because a vote over br/p is stupid
I can trust a person's judgment and still have my own opinions
if we "just trust" people then nothing is actually decided upon democratically smh
hoary umbra
just trust me bro™️
<br><br><br><br><br><br><br><br><br><br><br><br><br><br><br>
keen thicket
you mean <bro>
hoary umbra
we're good with this DR modid -> namespace change right? The ctor is private so nobody could be extending the class
mfw find-replace
Also, anyone else have any thoughts on providing a "super-DR" which is preloaded with helpers for the vanilla registries?
that way you don't need to make multiple DR objects, just one
winged iris
Also, anyone else have any thoughts on providing a "super-DR" which is preloaded...
That sounds like a potential maintenance nightmare. Either way out of out of scope of this PR
hoary umbra
yeah not for now
but it is a little bit cleaner (imo) to have one object that you call .item(() -> x) .block(() -> x) etc on instead of one per registry
ripe storm
Throwing my own opinion in on the whole br/p tag thing: I think it's rare to see individual sentences that are disjoint enough to warrant new lines but not a new paragraph, but br still has uses as a formatting mark when a visual break is necessary
ripe storm
https://i.imgur.com/9hmfZOh.png
But like, this? That looks bad. Imagine the window was half the width. You'd have a lot of partial little lines
As someone who works on a low resolution screen, single line paragraphs are hell and should be avoided like the plague
Because they turn into one-and-an-eigth line paragraphs instead
So if the sentences aren't disjoint enough for a paragraph break, I'd string em together
That way you don't get lots of little short fragments that don't wrap right as you're trying to read on a small screen (or heck, read javadocs on mobile!)
hoary umbra
I don't think that box gets any smaller (I'm pretty sure it's fixed-width by default)
ripe storm
Keep in mind that people read javadocs in lots of different ways
They're just HTML when it comes down to it
If it would be a bad idea on a webpage, it's probably a bad idea in javadocs (to a reasonable degree, of course)
(at least in IntelliJ it scales to your screen width to a degree, if your screen is small. Or maybe that's a setting I turned on. So it's definitely not "fixed width" and should not be treated as such)
hoary umbra
eclipses is fixed width for sure, bc its this little tiny box no matter how I resize the window
ripe storm
Yeah, but eclipse is not what everyone uses
hoary umbra
I mean yeah, but neither is IDEA, so what's the point of this convo lol
as long as the doc is readable this is just nitpicking
ripe storm
I mean yeah, but neither is IDEA, so what's the point of this convo lol
as long ...
My point is that with line breaks like that, it is not very readable in several fairly common places
Such as IntelliJ, or on the web
quaint pagoda
we're good with this DR `modid` -> `namespace` change right? The ctor is privat...
yeah but that's an unnecessary restriction
ripe storm
Or, heck, for anyone who doesn't have a screen wide enough to show that whole line at once
hoary umbra
No i mean for bincompat with the rename
if the class was (currently) extensible I couldn't rename the field
hoary umbra
alright, things updated again
someone should fix the compat check to auto-rebase before checking 
quaint pagoda
meh
It should automatically work
it tries to find the fork point to bin compat check with
lofty citrus
re: javadoc
<br>/hard line breaks should be avoided when possible. <p> for paragraph separation (none of this <br><br> nonsense). don't be afraid to have paragraphs with multiple sentences; they're normal. the first paragraph of a javadoc is special anyway, because (a) it's the first thing the reader readers in logical order, and (b) the first sentence of the first paragraph is taken as its summary (except when overriden by {@summary}, but that rarely if ever happens)
high raptor
out of curiosity is there any reason javadoc can't use something like markdown, outside of the {@things}?
quaint pagoda
cuz this isn't kotlin?
deep lance
because javadoc was invented in 1996
high raptor
I mean, it's just the tool displaying it that needs to support it, right?
quaint pagoda
yes and that includes all IDEs
it's not really feasible for us to go to markdown at all
hoary umbra
all two ide's, lol
but idk how complicated ide plugins are to write
apparently it was attempted https://github.com/openjdk/jdk/pull/11701
quaint pagoda
I'm just gonna say now we are not gonna write custom IDE plugins that we have to maintain just to use markdown...
and that would install all users wanting to read our jdocs to use those plugins
hoary umbra
Yeah there's not been any more noise on the jdk issue for now
heavy zephyr
markdown isn't particularly nice for things like @link
I largely prefer javadoc to whatever python does... you can't really @param with markdown
warped forge
someoen should create a "markdoc" that's like markdown but with additional syntax for tagging params, return types and such
I mean as a tool you can run on a source folder and it outputs documentation
/*#
@Summary
Wheee `code`
@Param arg1
The first argument. See @[](dev.wheee.TheClass) for details.
@Return
Returns `nothing`.
#*/
``` @hoary umbra this was caused by u from upstream merges I think
---
Index: src/main/resources/forge.exc
===================================================================
diff --git a/src/main/resources/forge.exc b/src/main/resources/forge.exc
--- a/src/main/resources/forge.exc (revision d80f945e2b9ebfc07908e7cf573000e514ad795c)
+++ b/src/main/resources/forge.exc (revision 1f323abe4a129bba3640345b9a7690b727834584)
@@ -87,6 +87,8 @@
net/minecraft/server/network/ServerStatusPacketListenerImpl.<init>(Lnet/minecraft/network/protocol/status/ServerStatus;Lnet/minecraft/network/Connection;Ljava/lang/String;)V=|p_272864_,p_273586_,statusCache
net/minecraft/server/packs/metadata/pack/PackMetadataSection.<init>(Lnet/minecraft/network/chat/Component;ILjava/util/Map;)V=|p_10371_,p_10372_,packTypeVersions
net/minecraft/server/packs/repository/Pack$Info.<init>(Lnet/minecraft/network/chat/Component;IILnet/minecraft/world/flag/FeatureFlagSet;Z)V=|f_244592_,dataFormat,resourceFormat,f_244041_,hidden
+net/minecraft/tags/TagFile.<init>(Ljava/util/List;ZLjava/util/List;)V=|f_215959_,f_215960_,remove
+net/minecraft/tags/TagLoader$EntryWithSource.<init>(Lnet/minecraft/tags/TagEntry;Ljava/lang/String;Z)V=|f_216042_,f_216043_,remove
net/minecraft/world/entity/Entity.changeDimension(Lnet/minecraft/server/level/ServerLevel;Lnet/minecraftforge/common/util/ITeleporter;)Lnet/minecraft/world/entity/Entity;=|p_20118_,teleporter
net/minecraft/world/entity/Entity.playCombinationStepSounds(Lnet/minecraft/world/level/block/state/BlockState;Lnet/minecraft/world/level/block/state/BlockState;Lnet/minecraft/core/BlockPos;Lnet/minecraft/core/BlockPos;)V=|p_277472_,p_277630_,primaryPos,secondaryPos
net/minecraft/world/entity/Entity.playMuffledStepSound(Lnet/minecraft/world/level/block/state/BlockState;Lnet/minecraft/core/BlockPos;)V=|p_283110_,pos
it causes patch artifacts which I found while working on the overhaul
hoary umbra
scrim
quaint pagoda
hopefully this will be solved by the new PR check going forward? But I thought that would have already caught this
when did this get merged...
hoary umbra
had to be part of the huge upstream pr, right?
quaint pagoda
yes
anyways a status update about the overhaul (at least, my draft of it):
I got everything compiling but haven't run the game yet
currently all forge registries are now just Registry<xxx>
and Registry now extends INewForgeRegistry with extra methods that either vanilla is missing cuz bad api or like, aliases
hoary umbra
smh, INewForgeRegistry and not INeoForgeRegistry
quaint pagoda
hmmm honestly good point
hoary umbra
I thought we pinned XExtension earlier
quaint pagoda
I was probably gonna move it to just IForgeRegistry after I delete the original
hoary umbra
so it should just be IRegistryExtension™️
quaint pagoda
that can be discussed later 
I still have to actually make syncing and serializing work
and also reverting to frozen when you leave a world
the system is a fucking mess if you couldn't tell
hoary umbra
hm
you know, thinking about it
why does it need to revert to frozen
it has to reconfigure when you load up a world
and it always does that, so... 
quaint pagoda
shrug it can be removed later
get a working product that does what the old system does is my current priority then api design and review can be done
not to mention if your deferredholder shit is merged soon then I have an entire other thing to worry about rebasing on top of...
god you did a lot of changes to that pr
did not notice
quaint pagoda
ah
hoary umbra
only the last two commits on the bottom are "new"
quaint pagoda
oop
hoary umbra
quaint pagoda
I edited message
hoary umbra
Unless we feel like debating javadoc semantics anymore we're probably good :p
hoary umbra
quaint pagoda
The namespace will be the one passed to the constructor.
imo this is still too generic and should specify the namespace is the one used when the DR was constructed
hoary umbra
screm
quaint pagoda
you rang?
the autoformat line adjust all params to start at the same column is a bit annoying but that's probably a eclipseism
hoary umbra
what does constructor indicate, the other constructor?
quaint pagoda
in my eyes this could easily be confusing as you are registering a supplier to a constructor, i.e. factory
hoary umbra
I'm pretty sure anyone with half a braincell does not need that javadoc at all, lol
quaint pagoda
using {@linkplain #namespace the provided namespace}.
honestly this makes a lot more sense to use
hoary umbra
autoformatter probably did that
quaint pagoda
yeah turn it off
quaint pagoda
> using {@linkplain #namespace the provided namespace}.
honestly this makes a lo...
namespace is protected
there should be a public getter
therefore the jdoc is always relevant
I am quoting from jdoc for createTagKey
and that format should likely be copied to #register
linking to the exact namespace used
hoary umbra
* @param path The path of the new entry. The namespace will be {@linkplain #namespace the provided namespace}.
that work?
I hate having "namespace" twice in the sentence, idk brain no like™️
quaint pagoda
up to you if you want to make it a getter so you can query the namespace the defreg was made with without having to subclass it
quaint pagoda
I don't know how useful being able to query the namespace is soo
hoary umbra
Probably not very, tbh
I still want to contribute a "super defreg" which is one to many instead of one to one (in terms of defreg to registry)
quaint pagoda
meh
at that point just use registerevent
that would be a lot of duplication of the same registry key over and over if its once per field
hoary umbra
nah builtin helpers for the vanilla classes
quaint pagoda
eww
quaint pagoda
I do not like the idea of hardcoding a bunch of vanilla classes
hoary umbra
well it has all of the vanilla helpers + public <P, T extends P> RegistryObject<T> custom(String path, ResourceKey<Registry<P>> registry, Supplier<T> factory)
quaint pagoda
that's just more maintenance
it means we have to add or remove every time vanilla does
hoary umbra
its not like vanilla is going to remove their registries, lol
quaint pagoda
I still don't like the maintenance burden and it's just method spaghet/explosion for not much reason
it would basically make defreg or whatever custom class less resilient across mc versions
defreg is so generic that it mostly is consistent
now we would have to consider renaming things as vanilla renames them and things not working as well across mc version boundaries
hoary umbra
eh it should be fine with DH being a thing now
this is only terrible rn due to how I have to reflect into RO
quaint pagoda
why do u have to
hoary umbra
package private spaghetti 🙃
there's no other way to bind an RO and I didn't want to reimplement it
quaint pagoda
are you making update reference public or something?
hoary umbra
I guess the alternative is I make this class delegate to a fuckton of DR's
but that's also bleh
quaint pagoda
btw ping me with the new commit in your pr when it is done and otherwise it's up to pup / any other team mebers
quaint pagoda
are you making update reference public or something?
how do you plan to do it with DH
quaint pagoda
ah yes
hoary umbra
(DH is also non-final so I can just do that too)
quaint pagoda
hopefully that doesn't become a problem
cuz bind eager vs bind on call is semantically different
hoary umbra
its still binds eager for ones from DR
quaint pagoda
is it just the one change in `register()`?
yeah I guess so
hoary umbra
quaint pagoda
I would say you could try to limit the random whitespace changes but I don't personally care that much given I've also done the same before
lofty citrus
is the namespace exposed as a getter?
hoary umbra
I mean once bound its just a != null
quaint pagoda
no it's a protected field
lofty citrus
hmmmm
quaint pagoda
this is my current changes for my draft of the overhaul of registries https://github.com/neoforged/NeoForge/compare/1.20.x...SizableShrimp:NeoForge:1.20.x-overhaul-registries-cherrypicked
101 dalmations files changed
keen thicket
and also reverting to frozen when you leave a world
why? vanilla already reconfigures the tags when you load up a new world, you can just inject the ID save loading hook somewhere near the initial updateTags call instead of adding another layer of complexity
hoary umbra
thats what I said 
quaint pagoda
it depends on if I preserved the dummying system or not
blocks used to get dummied into air
that means you would have extra entries depending on what world you are in
and if you dont revert to frozen or clear then they would never go away
hoary umbra
that thing is worthless, lol
quaint pagoda
I think it is gone in my thing tho
right now only the aliases would pollute the registries if I didn't revert to frozen
hoary umbra
aliases don't get ids
quaint pagoda
that's not what I said
they're polluting the registry
in my eyes it is the safest bet to have it be like normal when not in a world
hoary umbra
its a huge overhead though
quaint pagoda
with how I've designed it not really
in the old system sure
as reverting to frozen and loading from disk were separate
I'm trying to unify those codepaths
and it's I think maybe 2-3 patches to do?
it's actually 2 method calls and neither change patches cuz they are in forge classes and there's a layer of indirection
I have to fix all of these and it's annoying af https://i.imgur.com/Qu9JLCc.png
esp. cuz I have brain fog cuz I like let this sit for a while
hoary umbra
last doc change pushed
keen thicket
time for shrimp's never-ending nitpicks
keen thicket
nit-stabby
deep lance
if we're doing DeferredHolder<MyBlock>, how does that affect e.g. Holder#is(TagKey<T>)?
quaint pagoda
@hoary umbra update: trying to run the registry overhaul has led me to find multiple infinite stackoverflow loops
hooray
past saffron
if we're doing DeferredHolder<MyBlock>, how does that affect e.g. Holder#is(TagK...
It defers to the actual holder that comes from the registry once it exists
keen thicket
screm
keen thicket
at that point why implement Holder
if for most operations you'll want to cast to the base
hoary umbra
the generics are always the problem
deep lance
we could do DeferredHolder<Block, MyBlock> and solve a lot of generic problems (this is an option i suggested before, but worth bringing up again with the method parameter issues), at the cost of longer declarations
hoary umbra
that gets tricky while trying to maintain signature compat with RO
deep lance
I'm pretty sure we can override get [and value] to return the more specific type
hoary umbra
because RO has to then be DH<T, T>
ripe storm
I'm *pretty* sure we can override `get` [and `value`] to return the more specifi...
Yes, you can
past saffron
This generics issue is nothing new, RO#getHolder() already blows up with exactly the same problem
deep lance
not new perhaps but more visible
ripe storm
because RO has to then be DH<T, T>
Ah, yeah, that issue
quaint pagoda
generics are all fake anyways
you should just actually test it
but also I think double the generics is a horrible solution
ripe storm
RO cannot have two generic types in the declaration
Nah, that wouldn't be breaking, probably
deep lance
it's for damn sure be source breaking, unclear if it'd be binary breaking
ripe storm
Hmm. What else do you do besides double generics?
deep lance
but anywy
we can do one thing now and another thing in the next phase when RO is gone gone
ripe storm
it's for damn sure be source breaking, unclear if it'd be binary breaking
Source breaking yes. Binary almost certainly no
keen thicket
horrible idea
hoary umbra
ah yes, that works
ripe storm
Oh god. Type safety would be nice
hoary umbra
good plan™️
type safety on what, its a tag key, lol
like resource key, the generic is entirely made up™️
past saffron
https://i.imgur.com/l854UHf.png
RO spits out exactly the same thing
hoary umbra
yeah I figured
ripe storm
Hmm. I think long term double generics may be the only way to fix this
hoary umbra
if we want to avoid rawtypes, yeah
past saffron
Meanwhile I'm just typing my ROs to the basic type (i.e. Block)
deep lance
type safety on what, its a tag key, lol
type safety from someone trying to check their item for membership in a block tag
ripe storm
That just tossed the ball. The type safety there is made up but still useful to avoid passing your thing intended as an item tag to a block
hoary umbra
BLOCKS.register("test_block", () -> new AirBlock(BlockBehaviour.Properties.of().mapColor(MapColor.STONE)));
this would infer AirBlock twice
deep lance
like, it won't crash, it'll just silently accept the argument and not work
keen thicket
you can yeet type safety but have runtime safety
tag keys contain the associated registry ID
deep lance
yes that's safe
but it doesn't stop you from passing in the wrong thing
safely
this is the string capability ids argument all over again
hoary umbra
its just mojang fault for not making it ? super T
ripe storm
`BLOCKS.register("test_block", () -> new AirBlock(BlockBehaviour.Properties.of()...
Nah, take the second inference from the type of the DeferredRegister
deep lance
i don't blame them, generic wildcards are hell
ripe storm
Then you'll infer AirBlock and Block
deep lance
yeah, <U extends T> DeferredHolder<T, U> register(string, supplier<U>)
hoary umbra
@quaint pagoda what think, dual generic or rawtype overloads
keen thicket
we can go the BDFL route, no overlords
ripe storm
Given that the type info has to be somewhere you'll have to use one of the two
hoary umbra
though rawtype overloads are never going to work right with public Stream<TagKey<T>> tags()
deep lance
the rawtypes are so cursed, we might as well just have register return a Supplier that secretly implements Holder
ripe storm
Keep in mind that if you choose raw type overloads, you'll be unable to pass it to anywhere where vanilla expects a normal Holder<Block> unless you have a cast method or something
quaint pagoda
it's generic hell, we should be able to just fix vanilla's methods manually
deep lance
Keep in mind that if you choose raw type overloads, you'll be unable to pass it ...
no, that's fine oddly
keen thicket
pls no
ripe storm
it's generic hell, we should be able to just fix vanilla's methods manually
We'll screw over multiloader dev if we do that
keen thicket
no more messing with vanilla signatures
quaint pagoda
how would it screw multiloader
ripe storm
no, *that's* fine oddly
You can't pass a Holder<AirBlock> to a Holder<Block> argument
ripe storm
how would it screw multiloader
Cause suddenly I need to think about forge patches anywhere vanilla has holders with tightly bounded arguments
deep lance
ok it is a problem, but it's just a problem the raw overloads don't solve, not one that they cause.
that's where i was confused
deep lance
use ? super T where?
quaint pagoda
in the .is() methods
deep lance
you cannot
quaint pagoda
??
ripe storm
Shrimp means in a patch on vanilla
deep lance
oh, not just in the DH override?
hoary umbra
Nonetheless public Stream<TagKey<T>> tags() is still a problem
quaint pagoda
not really
ripe storm
Which... Please can we not change signatures of vanilla methods? It always causes pain
hoary umbra
that one cannot be ? super T
quaint pagoda
tagkey<myblock> wouldn't be a mega problem
ripe storm
Yes it would
ripe storm
Cause you couldn't compare it to anything
hoary umbra
you cannot ask a Holder<Block> if it is(TagKey<MyBlock>)
gloomy sinew
what if we just... waited until the next breaking period to add DeferredHolder, and toss out RegistryObject when we did
keen thicket
I'm just suggesting we use ? super T in place of T
this is how
quaint pagoda
you can if you patch it to use ? super T
deep lance
what if we just... waited until the next breaking period to add DeferredHolder, ...
i asked this before! i don't get the point of phase 1
hoary umbra
MyBlock is not ? super Block, lol
ripe storm
what if we just... waited until the next breaking period to add DeferredHolder, ...
That's not the issue. The issue is that people would expect to make a DeferredHolder<MyBlock> but vanilla has Holder<Block>'s
deep lance
we could just tell them no
ripe storm
My personal preference is double generics
deep lance
DeferredHolder<Block> is what you get
hoary umbra
yeah honestly RO's signature is a non-issue, that thing is spaghetti™️
gloomy sinew
doubel generics should be fine
deep lance
cast it yourself if you want
ripe storm
It's easy, it's typesafe, and it works
quaint pagoda
it's so much more verbose for so little gain
deep lance
I mean, is it?
gloomy sinew
it's either that or make DeferredHolder not implement Holder :p
deep lance
Item is four letters
ripe storm
it's so much more verbose for so little gain
Anything else requires vanilla patches that don't actually solve the issue
quaint pagoda
just add a unsafe cast method that lets people get whatever type of holder they want unsafely
ripe storm
just add a unsafe cast method that lets people get whatever type of holder they ...
If we do that, let's make the actual holder type (Block) the default and have people unsafe cast to their mod block types
hoary umbra
it is either that or not implement holder, unfortunately
I mean I can just strip the direct holder impl and add .holder() which generic downcasts to whatever
no good solutions, java generic just bad™️
keen thicket
probably a better idea imo. but it also wouldn't really fix .is(TagKey)
quaint pagoda
the java generics aren't bad mojang just didn't design their system right
ripe storm
probably a better idea imo. but it also wouldn't really fix `.is(TagKey)`
It would cause you'd get an actual holder at the end
hoary umbra
well then DH gets a native is<TagKey<? super T>> and holder() for other things
keen thicket
you'd do MY_THING.<Block>holder().is(BlockTags.STONE)
which is ugly
and not intuitive
quaint pagoda
a cursed suggestion would be adding isForge methods thru an interface extension and isForge(TagKey<?>) etc.
ripe storm
the java generics aren't bad mojang just didn't design their system right
They can't have both the is and the TagKey stream stuff working without a strict bound on T. One would be made flexible with super, and the other with extends
ripe storm
a cursed suggestion would be adding isForge methods thru an interface extension ...
Still doesn't fix the tag stream one
winged iris
Question if you add an intermediary class that then rawtypes the generics on the stream and the is tagkey does Java let you then make deferredholder extend that and use whatever generics you please?
ripe storm
you'd do `MY_THING.<Block>holder().is(BlockTags.STONE)`
That Block would be inferred
So you'd be fine
keen thicket
it wouldn't, inferrence is serial
deep lance
They can't have both the `is` and the TagKey stream stuff working without a stri...
i mean, no - the "actual" type on the TagKeys is always the registry type [Block, Item etc]
so still super
hoary umbra
its also very annoying that public <R super T> Holder<R> holder() { is illegal, because it does mean that you have to downcast
maybe we just make .get() downcast, idk
ripe storm
i mean, no - the "actual" type on the TagKeys is always the registry type [Block...
Yeah that's my point there. You'd have to return a super in one and accept a super in another. Returning a super isn't great inference, so generally you accept an extends instead
hoary umbra
but then supplier's signature isn't correct
The only thing that makes everything work™️ is dual generic
ripe storm
it wouldn't, inferrence is serial
Not always? I'm fairly certain Java can solve that type algebra for this example
Personally, dual generics aren't a huge issue. It's, like, one find and replace or whatever to add them
I'd say go for it, since it is the least intrusive solution that still has all the various features we'd like
deep lance
Question if you add an intermediary class that then rawtypes the generics on the...
oh god that's cursed but it might work
quaint pagoda
Personally, dual generics aren't a huge issue. It's, like, one find and replace ...
ha no
hoary umbra
what happens if I implement rawtype Holder
ripe storm
If you've made your tag keys also raw types you're not doing well
ripe storm
Okay, so, besides the slight extra verbosity, what are the actual issues with double generics?
hoary umbra
what
it would be stream<TagKey>
winged iris
Alternatively can we just add extra methods with slightly different names in DH that act as bouncers for the proper type? And if someone is using a DH they use those as needed but if they pass their DH as a holder still (maybe?)
quaint pagoda
no it wouldnt
winged iris
No shadows
winged iris
When you kill class level rawtypes it nukes everything
ripe storm
Alternatively can we just add extra methods with slightly different names in DH ...
They can't pass it as a Holder<Block> any more to any method expecting that
quaint pagoda
Alternatively can we just add extra methods with slightly different names in DH ...
that sounds like what I suggested
hoary umbra
why tf does it do that
ripe storm
When you kill class level rawtypes it nukes everything
Groovy lets you get around this. Just rewrite forge in groovy /s
hoary umbra
screm to generics
quaint pagoda
why tf does it do that
cuz backwards compat
keen thicket
Groovy lets you get around this. Just rewrite forge in groovy /s
no /s, just do it
ripe storm
But seriously:
Okay, so, besides the slight extra verbosity, what are the actual issues with double generics?
hoary umbra
There are none its just the verbosity, its probably the right call
winged iris
Idk I am fine with double generics, some of my registry object wrappers have between two and five generics
deep lance
Question if you add an intermediary class that then rawtypes the generics on the...
no.
ripe storm
I really wish Java let you have var fields with initializers right about now
quaint pagoda
I think they mean IntermediaryClass<T> extends Holder with no T
gloomy sinew
what if you had DeferredHolder deprecate is and add an isTag
deep lance
I think they mean IntermediaryClass<T> extends Holder with no T
i thought the point was to extend Holder<T> but magically make is(TagKey<? super T>) work
quaint pagoda
what if you had DeferredHolder deprecate `is` and add an `isTag`
people are just suggesting the same things now
winged iris
I think they mean IntermediaryClass<T> extends Holder with no T
Well that isn’t what I meant but is maybe worth trying as well, but I think it nukes all generics even higher up
deep lance
well, DH would put back whatever generics we want
alas, no
you cannot override a raw method with a method with a proper generic param, period, regardless of what happened higher up
ripe storm
what if you had DeferredHolder deprecate `is` and add an `isTag`
Doesn't matter. You still have a Holder<MyBlock> that can't be passed to anything expecting a Holder<Block>
past saffron
Turning the question the other way round, what's the problem with making people type their DHs to the registry base type (i.e. Block, Item, etc.)?
gloomy sinew
carpal tunnel
hoary umbra
I frequently have methods on my classes that I don't want to call via ((MyClass) DH.get()).blah()
gloomy sinew
oh you mean like
ripe storm
Turning the question the other way round, what's the problem with making people ...
If they want to get their block back out of it. But yeah, that's also an option
deep lance
hmm
winged iris
Technically we could add a getTyped but that would be an unchecked cast
quaint pagoda
Well that isn’t what I meant but is maybe worth trying as well, but I think it n...
honestly this would work. but you would have to duplicate a lot of methods
deep lance
what if we made it opt-in
hoary umbra
its already an unchecked cast
gloomy sinew
DeferredHolder<Block> block = register("block", () -> new ReallySpecificBlockClass())
hoary umbra
no sugarcoating it, its always an unchecked cast, even now
gloomy sinew
that's no good because you need the actual type of the block a lot
past saffron
Yes
ripe storm
Could just have a <B extends T> B getAndCast() method
deep lance
DeferredHolder has a single type argument, but it implements an interface with two type arguments
keen thicket
<Z extends T> Z getUnchecked() { return (Z) get(); }
deep lance
and has a method that casts to this interface
winged iris
Tbh double generic is likely the way to go (even though then the supplier bit might not be the generic type the person wants but 
)
ripe storm
`<Z extends T> Z getUnchecked() { return (Z) get(); }`
Yeah, I like this
hoary umbra
getUnchecked is going to have inference problems
ripe storm
Unless we do double generic
hoary umbra
I can't just call getUnchecked().myMethod()
ripe storm
Eh, store it on a local variable
gloomy sinew
double generic is fine, you'll only ever have to type the whole type in your static init fields
ripe storm
Or just do double generics
ripe storm
Definitely
deep lance
also honestly
past saffron
that's no good because you need the actual type of the block a lot
I have never run into issues doing it this way and I have all my stuff typed to base type
deep lance
Item, <- 6 chars
quaint pagoda
imo if you go dual generics route then you should not deprecate registryobject for removal and just remove the holder stuff from it
hoary umbra
screms return (T) this.holder.get(); (it did this anyway, but now I have to look at it)
deep lance
and if your supplier returns the wrong type you can cast there to avoid having to write type params on register
quaint pagoda
people who want deferredholder -> DeferredRegister#registerHolder, have to hold 2 generics
people who dont need holder -> DeferredRegister#register, just like normal, everything is the same
ripe storm
imo if you go dual generics route then you should not deprecate registryobject f...
Why wouldn't it be deprecated for removal still?
past saffron
`Item, ` <- 6 chars
BlockEntityType would like to have a word with you
hoary umbra
RO has to extend DH, there is no "remove the holder stuff from it"
gloomy sinew
RO has to extend DH, there is no "remove the holder stuff from it"
why?
hoary umbra
bincompat™️
ripe storm
I say we just merge this with breaking changes and drop RO entirely
gloomy sinew
why do registryobjects and deferredholders both need to exist at the same time
quaint pagoda
^^^^
hoary umbra
bincompat™️
gloomy sinew
we can wait for a breaking change and add deferredholders and remove robs
deep lance
ok but why do we need bincompat
ripe storm
Like, if this waits till the breaking change window we can just drop RO
deep lance
why do we need phase 1
quaint pagoda
oh commoble had a different ide afrom me
my idea is that they can be separate solutions for different people and coexist
hoary umbra
because DR can't make both of them at once
deep lance
it's a fun party trick but sweeping changes like this are what breaking windows are meant for
gloomy sinew
and then I had a mental image of sweeping all the robjects out the door
hoary umbra
DeferredRegistwo
deep lance
if deferred registers are so great why isn't there deferred register 2
ripe storm
my idea is that they can be separate solutions for different people and coexist
I just don't see the reason to keep around RO if DH exists. If the only reason is "less typing cause only one generic" that's hardly a reason
keen thicket
RO is the dumber older sibling of DH, we should just remove RO. if you want one generic, use a supplier
ripe storm
Hey, that's an idea
deep lance
if you don't need DH and don't like the double generics
ripe storm
Why not add a method to DH to get a supplier
deep lance
you can declare your statics as Supplier
gloomy sinew
why not make DH implement Supplier
deep lance
DH DOES IMPLEMENT SUPPLIER
keen thicket
not if you make DH extends the base class in Holder
also the holder-implements-supplier is patched by forge
deep lance
can we make it implement Supplier<MyBlock>?
hoary umbra
public class DeferredHolder<R, T extends R> implements Holder<R>, Supplier<T>
ripe storm
`public class DeferredHolder<R, T extends R> implements Holder<R>, Supplier<T>`
Yeah, exactly
deep lance
like, the method override is allowed but is the implements decl allowed
ripe storm
That way if you want to type less, your field is just a supplier
winged iris
Given holder is an interface would any of this be easier if we made DH extend RO implements Holder instead of RO extends DH
hoary umbra
no
winged iris
Okay
ripe storm
Given holder is an interface would any of this be easier if we made DH extend RO...
Wouldn't really matter. This is all still an issue if RO is gone
hoary umbra
it means i have to keep all the logic in RO or make RO an interface (can't do that bc of the factories)
basically makes it way more work for later to keep RO as the base class
deep lance
goddamnit it can't
hoary umbra
with the deprecation we have everything ready to go do just press delete when bc arrives
ripe storm
But yeah, I think making DH implement Supplier<MyBlock>` is a good solution, and maybe removing the forge patch that shoves Supplier on holders to allow that? Not entirely sure
hoary umbra
don't need to remove the patch
deep lance
wait
hoary umbra
you can implement supplier again with a stronger type
ripe storm
goddamnit it can't
It can if you stop forge from shoving supplier on holders
deep lance
it can, it just can't inherit the default impl of get
gloomy sinew
ripe storm
you can implement supplier again with a stronger type
It can? I... Did not think that worked, as a Supplier<MyBlock> is not a Supplier<Block>, which the superclass implements
deep lance
It can if you stop forge from shoving supplier on holders
is that a forge thing?
gloomy sinew
still doesn't work
ripe storm
is that a forge thing?
To my awareness, yes
quaint pagoda
you can override get() with the resolved type cuz covariant but you wouldn't be able to automatically keep it around as a Supplier<R>
gloomy sinew
yeah if we remove the supplier patch from holder we could add it to deferredholder
deep lance
goddammit you could do this in C#
quaint pagoda
is that a forge thing?
yes that was a me thing
ripe storm
We could also just have a getSupplier() method on DH that gets a supplier of the stricter type
past saffron
I just don't see the reason to keep around RO if DH exists. If the only reason i...
Also, if you want to avoid the second generic in your variables, you could also just extend DH with a custom type-specific implementation since DH won't be final unlike RO
gloomy sinew
public interface Holder<T> extends java.util.function.Supplier<T>, net.minecraftforge.registries.tags.IReverseTag<T> {
if you want a Supplier from a Holder just ::value
deep lance
what would be the patch surface of porting minecraft to C#
gloomy sinew
yeah if we remove the supplier patch from holder we could add it to deferredhold...
'course that idea breaks if monag ever makes holders suppliers but we can burn that bridge when we come to it
ripe storm
Like, why don't we give DH a method to get a specific supplier, and have it extend holder which is the generic supplier?
keen thicket
it'd be bedrock but more MS
hoary umbra
I can do this though™️ https://i.imgur.com/B7vTI34.png
ripe storm
if you want a `Supplier` from a `Holder` just `::value`
Or that yeah, though we need to overload get in DH to make the returned type more specific
Which is easy
So yeah, this works great
People who want to type less get a supplier, otherwise you just specify both
gloomy sinew
oh, yeah, that does work
gloomy sinew
oh right
ripe storm
Yeah, but that's easy
gloomy sinew
okay, let's just take the supplier patch off holder and put it on deferredholder
hoary umbra
for their thingy
gloomy sinew
is anyone actually using holders as suppliers
ripe storm
okay, let's just take the supplier patch off holder and put it on deferredholder
I mean, no need to do that. We can just leave it where it is and let people use ::get. Though honestly, either way works
quaint pagoda
is anyone actually using holders as suppliers
at this point probably
it was done to make it easier to pass in both ROs and Holders to a forge patched constructor that takes a supplier
ripe storm
Cause if get returns MyBlock, ::get will get you a Supplier<MyBlock> even if the DH is a Supplier<Block>
deep lance
is it though
hoary umbra
yeah, if you need to pass vanilla
ripe storm
I mean, they're forge added suppliers
hoary umbra
vanilla doesn't keep holder fields
ripe storm
vanilla doesn't keep holder fields
It doesn't keep suppliers either and we're currently using those
keen thicket
just wrapAsHolder()
hoary umbra
well yes but I can () -> STONE pretty easily
ripe storm
Ah, fair enough
hoary umbra
also most things with supplier have a T constructor and a Supplier<T>
because the supplier one is for deferred /modded st uff
now this, this is an issue
ripe storm
Personally, I am in favor of keeping holders as suppliers, and making get on DH return MyBlock so that the DH will be a Supplier<Block> but that using ::get on it will provide a Supplier<MyBlock>
ripe storm
https://i.imgur.com/5Ssh6Qm.png
Eh, I'd say let's screw bincompat and throw it all in a breaking changes window
keen thicket
https://i.imgur.com/5Ssh6Qm.png
yeah that's easy. screw source compat
ripe storm
Or that, keep bin compat and break source compat
Generics don't exist at runtime anyways really
deep lance
really we get two short ways to declare statics, as a bonus
DeferredHolder<Block, MyBlock> = register()
Holder<Block> = register()
Supplier<MyBlock> = register()::get```ItemLike x = register()::get
hoary umbra
actually it has to be even more cursed and I have to shadow R to let it infer RO<T>
screm
public <R, T extends R, D extends DeferredHolder<R, T>> D register(String path, Supplier<T> sup)
however
even this huge pile of generic garbage has an issue
if someone is, for whatever reason, manually typing register, it does not have source compat
like we do (for some reason) in this test mod https://i.imgur.com/xKONQaY.png
granted idk why it does that
it just works if removed
gloomy sinew
like we do (for some reason) in this test mod https://i.imgur.com/xKONQaY.png
doesn't like entitytype registration need to be manually typed
or I might be thinking of Builder.<>of
past saffron
The builder has to be typed in a lot of cases, not the RO
hoary umbra
yeah that's the builder
I can't think of an instance where the RO has to be typed since the inference is very loose
gloomy sinew
hoary umbra
but it's still technically source breaking
hoary umbra
I think due to erasure this is bin breaking as well
gloomy sinew
yeah register does already have a type param
hoary umbra
since the signature becomes (...)DeferredHolder instead of (...)RegistryObject
could just hold off on the dual generic
gloomy sinew
could just hold off on adding deferredholders until we yeet robjects :p
hoary umbra
I could but I'm waiting for them to exist so I can stop doing this https://github.com/Shadows-of-Fire/Placebo/blob/1.20/src/main/java/dev/shadowsoffire/placebo/registry/DeferredHelper.java#L169
keen thicket
screm shadows that MH is slower than reflection...
hoary umbra
ultimately a gradual transition will be smoother even if it involves a little refactoring here and there™️
for now people get to swap to DH, and then later they can implement the second generic on the DH
ripe storm
like we do (for some reason) in this test mod https://i.imgur.com/xKONQaY.png
Do we care that much about source compat? And/or can it wait till 1.20.2?
hoary umbra
the generic hack is bin breaking anyway so its irrelevant
ripe storm
Ah, yeah
hoary umbra
personally I think that source compat example in particular is silly (nobody should be doing that)
but yeah, D is breaking since it erases to DH not RO
I hate that I can't do this as a temp workaround
public <X super T> Holder<X> weakHolder()
I can just do <X> but typesafety 
keen thicket
public <X super T> Holder<X> weakHolder(Class<X>) just to make it nicer for chaining
winged iris
X super T is illegal Maty, that’s the problem
hoary umbra
honestly idk why its illegal
keen thicket
I was fixing his idea
hoary umbra
bit of a weird restriction