[1.20.x] Deprecate RegistryObject for removal #35

Shadows-of-Fire · 2023-07-19T10:18:09Z

This PR performs the following goals as an initial transition to the vanilla registry system, in accordance with #37:

Forces all forge registries to be backed by a vanilla registry.
Makes RegistryObject implement Holder.
Deprecates RegistryObject for removal, as all uses can be replaced with Holder.
Introduces DeferredHolder to replaces uses of RegistryObject not covered by Holder$Reference.

Due to method signatures, DeferredRegister#register must still return a RegistryObject, but callers can switch the return type of the stored value to Holder or DeferredHolder.

src/main/java/net/minecraftforge/registries/RegistryObject.java

src/main/java/net/minecraftforge/registries/ForgeRegistry.java

src/main/java/net/minecraftforge/registries/DeferredRegister.java

cpw · 2023-07-20T14:42:09Z

Suggestion: while this is still being discussed, can we mark this as a draft PR, please? Thanks.

src/main/java/net/minecraftforge/registries/DeferredHolder.java

Shadows-of-Fire · 2023-07-20T17:07:52Z

Suggestion: while this is still being discussed, can we mark this as a draft PR, please? Thanks.

I could but I think we're past that point now :p

src/main/java/net/minecraftforge/registries/RegistryBuilder.java

src/main/java/net/minecraftforge/registries/DeferredRegister.java

src/main/java/net/minecraftforge/registries/DeferredHolder.java

…ge changes, and revolve more around vanilla Holder types. Mentioned NeoForge Registry changes: neoforged/NeoForge#35 neoforged/NeoForge#37

Shadows-of-Fire · 2023-07-30T08:37:35Z

Changes made, except for the one about the cast in DeferredHolder#create - that one has to stay there, as moving it is a compile -time error (sometimes, the eclipse compiler works fine but gradle does not)

C:\Development\Github\NeoForgeFork\src\main\java\net\minecraftforge\registries\DeferredHolder.java:53: error: no suitable method found for create(ResourceKey<CAP#1>,ResourceLocation)
        return new DeferredHolder<T>((ResourceKey) ResourceKey.create(registryKey, valueName));
                                                              ^
    method ResourceKey.<T#2>create(ResourceKey<? extends Registry<T#2>>,ResourceLocation) is not applicable
      (cannot infer type-variable(s) T#2
        (argument mismatch; ResourceKey<CAP#1> cannot be converted to ResourceKey<? extends Registry<T#2>>))
    method ResourceKey.<T#3>create(ResourceLocation,ResourceLocation) is not applicable
      (cannot infer type-variable(s) T#3
        (argument mismatch; ResourceKey<CAP#1> cannot be converted to ResourceLocation))
  where T#1,T#2,T#3 are type-variables:
    T#1 extends Object declared in method <T#1>create(ResourceKey<? extends Registry<? super T#1>>,ResourceLocation)
    T#2 extends Object declared in method <T#2>create(ResourceKey<? extends Registry<T#2>>,ResourceLocation)
    T#3 extends Object declared in method <T#3>create(ResourceLocation,ResourceLocation)
  where CAP#1 is a fresh type-variable:
    CAP#1 extends Registry<? super T#1> from capture of ? extends Registry<? super T#1>

pupnewfster

Couple more changes. I also still need to try and figure out/validate whether making all forge registries have a backing vanilla one causes compatibility issues when it comes to syncing, as I am not sure if vanilla will then try to sync all the things in BuiltInRegistries.REGISTRY... And if it does:
a. There may be issues when using a client that doesn't have this PR on a server that does (or vice versa)
b. It would be syncing things that RegistryBuilder#disableSync was used to try and disable

src/main/java/net/minecraftforge/registries/RegistryBuilder.java

src/main/java/net/minecraftforge/registries/DeferredHolder.java

src/main/java/net/minecraftforge/registries/DeferredRegister.java

SizableShrimp

Here are my thoughts.

src/main/java/net/minecraftforge/registries/DeferredRegister.java

Shadows-of-Fire · 2023-08-02T00:41:37Z

DefReg docs updated, rebased to fix compat check that was failing due to #72

FiniteReality · 2023-09-21T08:30:45Z

src/main/java/net/minecraftforge/registries/RegistryObject.java

-     * @param name the name of the object to look up in the forge registry
-     * @param registry the forge registry
-     * @return a {@link RegistryObject} that stores the value of an object from the provided forge registry once it is ready
-     */
    public static <T, U extends T> RegistryObject<U> create(final ResourceLocation name, IForgeRegistry<T> registry)


I'm assuming that most users of this API would be third parties, should they be directed to IForgeRegistry#getHolder?

On a related note, should IForgeRegistry#getHolder be updated to return a DeferredHolder/similar itself?

should they be directed to IForgeRegistry#getHolder

No, probably not, given that getHolder defers to the vanilla registry, which is implemented as Optional.ofNullable(this.byKey.get(pKey)). This operation requires that the object be registered before the Holder can be accessed (which is usually not the case at the time of creation of DH/RO's)

IForgeRegistry#getHolder be updated to return a DeferredHolder/similar itself

Since those methods are mostly bouncers to vanilla they will be gone after the upcoming refactor. There might be room to add an additional getDeferredHolder method to IForgeRegistry but it would just be a util that wraps create.

Technici4n · 2023-09-25T08:57:06Z

src/main/java/net/minecraftforge/registries/DeferredHolder.java

+ * @param <T> The type of object being held by this DeferredHolder.
+ */
+// TODO: (Breaking Change) - Add a second generic type for the base registry type
+public class DeferredHolder<T> implements Holder<T>


There should be one type for the registry and one type for the actual type. The Holder should have the registry type.

Bikeshedding here, but I'd prefer DeferredHolder<TRegistry, TInstance extends TRegistry> implements Holder<TRegistry>

Technici4n · 2023-09-25T08:58:24Z

src/main/java/net/minecraftforge/registries/DeferredHolder.java

+    public static <R, T extends R> DeferredHolder<T> create(ResourceKey<? extends Registry<R>> registryKey, ResourceLocation valueName)
+    {
+        return create(ResourceKey.create(registryKey, valueName));
+    }
+
+    /**
+     * Creates a new DeferredHolder targeting the value with the specified name in the specified registry.
+     * 
+     * @param <T>          The registry type.
+     * @param registryName The name of the registry the target value is a member of.
+     * @param valueName    The name of the target value.
+     */
+    public static <T> DeferredHolder<T> create(ResourceLocation registryName, ResourceLocation valueName)
+    {
+        return create(ResourceKey.createRegistryKey(registryName), valueName);
+    }
+
+    /**
+     * Creates a new DeferredHolder targeting the specified value.
+     * 
+     * @param <T> The type of the target value.
+     * @param key The resource key of the target value.
+     */
+    public static <T> DeferredHolder<T> create(ResourceKey<? super T> key)
+    {
+        return new DeferredHolder<>(key);
+    }


These constructors will take an unbounded actual type, and I'm not a fan - it's easy to make a mistake. Maybe we make these methods protected. Otherwise we can also split DeferredHolder<R> and SomeSubclass<R, T> (R referring to the registry type here).

Technici4n · 2023-09-25T08:58:38Z

src/main/java/net/minecraftforge/registries/DeferredHolder.java

+     * The currently cached value.
+     */
+    @Nullable
+    protected Holder<T> holder = null;


Is thread safety a potential concern here?

No threading issue is possible here, since bind will get the same value if called multiple times (setting this to null between), and there is no window in which holder is no longer null but not the correct holder.

In other words, threading issues would happen in the middle of a call to bind. If one thread finishes bind before the other starts, you just fetch the holder multiple times and get the same value each time. If one thread makes it past line 141 then the other calls bind, it just gets bound again. Worst case scenario is two threads both throw on missing registry.

sciwhiz12 · 2023-11-19T13:43:24Z

Superseded by #257, the registry rework PR.

Shadows-of-Fire requested a review from a team July 19, 2023 10:18

pupnewfster reviewed Jul 19, 2023

View reviewed changes

Technici4n reviewed Jul 19, 2023

View reviewed changes

src/main/java/net/minecraftforge/registries/ForgeRegistry.java Outdated Show resolved Hide resolved

Matyrobbrt reviewed Jul 19, 2023

View reviewed changes

src/main/java/net/minecraftforge/registries/DeferredRegister.java Outdated Show resolved Hide resolved

Shadows-of-Fire requested a review from a team July 20, 2023 04:56

Shadows-of-Fire force-pushed the holder-ro branch from e61eb00 to 138d527 Compare July 20, 2023 05:44

KingLemming reviewed Jul 20, 2023

View reviewed changes

src/main/java/net/minecraftforge/registries/DeferredHolder.java Outdated Show resolved Hide resolved

Shadows-of-Fire mentioned this pull request Jul 20, 2023

[1.20.2+] The Registry Overhaul #37

Closed

pupnewfster requested changes Jul 23, 2023

View reviewed changes

sciwhiz12 added enhancement New (or improvement to existing) feature or request 1.20 Targeted at Minecraft 1.20 labels Jul 28, 2023

Shadows-of-Fire requested review from pupnewfster and a team July 30, 2023 08:37

Shadows-of-Fire force-pushed the holder-ro branch from 178f3f8 to c21566b Compare July 30, 2023 20:53

ApexModder mentioned this pull request Jul 31, 2023

Registry System overhaul ApexStudios-Dev/ApexCore-Archive#9

Merged

16 tasks

pupnewfster requested changes Jul 31, 2023

View reviewed changes

Shadows-of-Fire assigned pupnewfster and SizableShrimp Aug 1, 2023

Shadows-of-Fire requested a review from pupnewfster August 1, 2023 21:00

SizableShrimp requested changes Aug 1, 2023

View reviewed changes

SizableShrimp reviewed Aug 1, 2023

View reviewed changes

src/main/java/net/minecraftforge/registries/DeferredRegister.java Outdated Show resolved Hide resolved

Shadows-of-Fire added 5 commits August 1, 2023 17:41

Force all ForgeRegistries to have a vanilla registry

05091e4

Make RegistryObject a wrapper around Holder$Reference

1fc8759

review changes

f20ee08

misc cleanup

3ba2239

introduce DeferredHolder

c4ca7c4

Shadows-of-Fire added 8 commits August 1, 2023 17:41

minor doc change

f651a7b

minor updates

4bb0ad5

review changes

cb81a8a

Move registry sanity check to bind()

2ed3df9

resource key shuffling

d341040

misc cleanup

1abb0e7

Update defreg docs and rename modid to namespace

90ce4e0

more docs

1e2facb

Shadows-of-Fire force-pushed the holder-ro branch from 8f3361e to 1e2facb Compare August 2, 2023 00:41

sciwhiz12 requested a review from SizableShrimp August 2, 2023 04:00

Shadows-of-Fire added 2 commits August 2, 2023 23:44

Update DeferredRegister.java

f2a7f92

add workaround method to DH

7935b90

FiniteReality reviewed Sep 21, 2023

View reviewed changes

Technici4n requested changes Sep 25, 2023

View reviewed changes

Matyrobbrt mentioned this pull request Nov 6, 2023

[20.2] Registry rework #257

Merged

sciwhiz12 closed this Nov 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1.20.x] Deprecate RegistryObject for removal #35

[1.20.x] Deprecate RegistryObject for removal #35

Shadows-of-Fire commented Jul 19, 2023 •

edited by sciwhiz12

Loading

cpw commented Jul 20, 2023

Shadows-of-Fire commented Jul 20, 2023 •

edited

Loading

Shadows-of-Fire commented Jul 30, 2023

pupnewfster left a comment

SizableShrimp left a comment

Shadows-of-Fire commented Aug 2, 2023

FiniteReality Sep 21, 2023

FiniteReality Sep 21, 2023

Shadows-of-Fire Sep 21, 2023

Technici4n Sep 25, 2023

FiniteReality Sep 25, 2023

Technici4n Sep 25, 2023

Technici4n Sep 25, 2023

KnightMiner Sep 25, 2023 •

edited

Loading

sciwhiz12 commented Nov 19, 2023

[1.20.x] Deprecate RegistryObject for removal #35

[1.20.x] Deprecate RegistryObject for removal #35

Conversation

Shadows-of-Fire commented Jul 19, 2023 • edited by sciwhiz12 Loading

cpw commented Jul 20, 2023

Shadows-of-Fire commented Jul 20, 2023 • edited Loading

Shadows-of-Fire commented Jul 30, 2023

pupnewfster left a comment

Choose a reason for hiding this comment

SizableShrimp left a comment

Choose a reason for hiding this comment

Shadows-of-Fire commented Aug 2, 2023

FiniteReality Sep 21, 2023

Choose a reason for hiding this comment

FiniteReality Sep 21, 2023

Choose a reason for hiding this comment

Shadows-of-Fire Sep 21, 2023

Choose a reason for hiding this comment

Technici4n Sep 25, 2023

Choose a reason for hiding this comment

FiniteReality Sep 25, 2023

Choose a reason for hiding this comment

Technici4n Sep 25, 2023

Choose a reason for hiding this comment

Technici4n Sep 25, 2023

Choose a reason for hiding this comment

KnightMiner Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

sciwhiz12 commented Nov 19, 2023

Shadows-of-Fire commented Jul 19, 2023 •

edited by sciwhiz12

Loading

Shadows-of-Fire commented Jul 20, 2023 •

edited

Loading

KnightMiner Sep 25, 2023 •

edited

Loading