1.21.x Capability Factory System PR #10118

RealMangorage · 2024-09-21T10:25:03Z

The goal of this PR is to provide a way for devs to register Capability Providers Factories which handle creating their providers, vs having em create it inside of an event every tick.

…lityFactoryHolder's

Jonathing

Good work so far. Before I move on with proper testing, please note the comments I've left, especially the ones in IForgeResourceKey.

Jonathing · 2024-09-29T01:10:08Z

src/main/java/net/minecraftforge/common/extensions/IForgeResourceKey.java

+    default Holder<T> getOrThrow(BlockEntity blockEntity) {
+        return getOrThrow(blockEntity.getLevel());
+    }


I don't think this method is a good idea. You should really be directly calling getOrThrow(Level) if you want to do it like this.

Jonathing · 2024-09-29T01:12:26Z

src/main/java/net/minecraftforge/common/extensions/IForgeResourceKey.java

+    default Holder<T> getOrThrow(Entity entity) {
+        return getOrThrow(entity.registryAccess());
+    }


Same for this. The entity's registry access method is simply calling its level reference's. Just using the single getOrThrow(Level) method is probably for the best, so as to not cause confusion.

He and I talked about this, these are meant to be helper functions that yes are strictly not necessary but people have complained that they want easier access.

Jonathing · 2024-09-29T01:13:35Z

src/main/java/net/minecraftforge/common/extensions/IForgeResourceKey.java

+    default Holder<T> getOrThrow(Level level) {
+        return getOrThrow(level.registryAccess());
+    }


Please add documentation/comment explanation to this method, similar to getOrThrow(RegistryAccess).

Jonathing · 2024-09-29T01:18:31Z

src/main/java/net/minecraftforge/common/capabilities/CapabilityFactoryHolder.java

Missing license header. See existing files in project for examples.

/* * Copyright (c) Forge Development LLC and contributors * SPDX-License-Identifier: LGPL-2.1-only */

Jonathing · 2024-09-29T01:18:37Z

src/main/java/net/minecraftforge/common/capabilities/CapabilityFactoryManager.java

Missing license header. See existing files in project for examples.

/* * Copyright (c) Forge Development LLC and contributors * SPDX-License-Identifier: LGPL-2.1-only */

Jonathing · 2024-09-29T01:18:52Z

src/main/java/net/minecraftforge/common/capabilities/ICapabilityFactory.java

Missing license header. See existing files in project for examples.

/* * Copyright (c) Forge Development LLC and contributors * SPDX-License-Identifier: LGPL-2.1-only */

Jonathing · 2024-09-29T01:20:45Z

src/test/java/net/minecraftforge/debug/gameplay/capabillities/CapabilityTest.java

I think this test should clearly indicate the changes that this PR brings. So, I'd like for you to note with comments where the capability factory system is being used.

Jonathing · 2024-09-29T01:22:11Z

src/main/java/net/minecraftforge/common/ForgeMod.java

@@ -40,6 +40,7 @@
 import net.minecraft.world.level.storage.loot.predicates.LootItemConditionType;
 import net.minecraft.world.phys.Vec3;
 import net.minecraftforge.client.extensions.common.IClientFluidTypeExtensions;
+import net.minecraftforge.common.capabilities.CapabilityFactoryManager;


Unused import, please remove.

Jonathing · 2024-09-29T01:23:05Z

src/main/java/net/minecraftforge/common/extensions/IForgeResourceKey.java

Interface extensions are not patches, so we don't need to be as careful when implementing them as we do with patches. That being said, you should still watch out for the redundancies I've noted in this file.

Jonathing · 2024-09-29T01:26:19Z

src/main/java/net/minecraftforge/common/capabilities/ICapabilityFactory.java

+
+import net.minecraft.resources.ResourceLocation;
+
+public interface ICapabilityFactory<T> {


You should mark this interface with @FunctionalInterface since these factories are effectively lambdas that can be used with the system individually.

Jonathing · 2024-09-29T01:28:11Z

src/main/java/net/minecraftforge/common/capabilities/CapabilityFactoryHolder.java

This is more of my personal opinion, but a lot of the code in this file is hard to read. I would appreciate it if you could either add relevant JavaDoc or explanation comments to some of the methods in this. Thanks.

Jonathing · 2024-09-29T01:28:17Z

src/main/java/net/minecraftforge/common/capabilities/CapabilityFactoryManager.java

Like with CapabilityFactoryHolder, additional comments would be appreciated.

PaintNinja

A good start, but some comments.

Additionally:

Needs documentation written in the PR description, ideally a migration guide as well
Internals marking with @ApiStatus.Internal

PaintNinja · 2024-09-29T11:26:54Z

src/main/java/net/minecraftforge/common/capabilities/CapabilityFactoryManager.java

+import java.util.Map;
+
+public final class CapabilityFactoryManager {
+    private static final CapabilityFactoryManager MANAGER = new CapabilityFactoryManager();


This whole class can be simplified by making the methods and fields static rather than requiring a level of indirection through a singleton

Ya this INSTANCE field pattern is a thing from cpw's days its better to not use this indirect.
On top of that, this class needs to be annotated as internal, as the only thing needing to be public is the init function, which could potentially just be an event handler. Will need to look into that more. Point is this isn't mean to be modder facing in any way.

PaintNinja · 2024-09-29T11:27:54Z

src/main/java/net/minecraftforge/common/capabilities/CapabilityFactoryManager.java

+        return MANAGER;
+    }
+
+    private final Map<Class<?>, Map<ResourceLocation, ICapabilityFactory<?, ?>>> factory = new HashMap<>();


Maps with keys of type Class<?> should use IdentityHashMap for better performance

If you didn't mind, can you explain why that is so I know for future reference?

HashMap uses hashCode() and equals().

IdentityHashMap uses identityHashCode() and ==.

By explicitly operating on reference identity rather than object equality, the internal algorithms used for the map can be tailored to take advantage of that.

In my testing with the two map types for Class<?> keys in my own EventBus, the IdentityHashMap was consistently faster than a plain HashMap.

PaintNinja · 2024-09-29T11:28:21Z

src/main/java/net/minecraftforge/common/capabilities/CapabilityFactoryRegisterEvent.java

+import java.util.Map;
+
+public class CapabilityFactoryRegisterEvent extends Event {
+    final Map<Class<?>, Map<ResourceLocation, ICapabilityFactory<?, ?>>> factory = new HashMap<>();


IdentityHashMap here as well

PaintNinja · 2024-09-29T11:31:07Z

src/main/java/net/minecraftforge/common/capabilities/CapabilityProviderWithFactory.java

+
+public class CapabilityProviderWithFactory<B extends ICapabilityProviderImpl<B>> implements ICapabilityProviderImpl<B> {
+    @VisibleForTesting
+    static boolean SUPPORTS_LAZY_CAPABILITIES = true;


Fields that are not static final should not be SCREAMING_SNAKE_CASE. Either add final here or change to camelCase

Was copied from CapabilityProvider

Your point being?

To keep existing behavior?

src/main/java/net/minecraftforge/common/capabilities/CapabilityProviderWithFactory.java

LexManos · 2024-09-30T09:41:18Z

src/main/java/net/minecraftforge/common/capabilities/CapabilityFactoryRegisterEvent.java

+
+    public CapabilityFactoryRegisterEvent() {}
+
+    public <G, H extends ICapabilityProvider> void register(Class<G> gClass, ResourceLocation resourceLocation, ICapabilityFactory<G, H> factory) {


Can I get a better explanation/docs for the differences between these two functions?
I am not quite understanding why you have CapabilityProviderHolder as public API.
Your example code has noop listeners and singleton providers
I'll look into it a little more but we can talk about this more on discord.

autoforge · 2024-10-26T01:14:40Z

@RealMangorage, this pull request has conflicts, please resolve them for this PR to move forward.

RealMangorage added 4 commits August 17, 2024 19:38

Moved EntityRenderersEvent into a Forge Bus Event.

0f83b82

Did some stuff

6b95e42

Did some stuff

3debd16

Minor fixes

d69665b

RealMangorage marked this pull request as draft September 21, 2024 10:25

autoforge bot added Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. LTS This issue/PR is related to the current LTS version. 1.21 labels Sep 21, 2024

autoforge bot requested a review from a team September 21, 2024 10:25

RealMangorage added 4 commits September 23, 2024 01:12

Minor fixes

09beadc

Minor fixes

c2d550e

Minor fixes

6fb0e52

Minor fixes

a21145a

cech12 mentioned this pull request Sep 26, 2024

re-add Forge support cech12/BucketLib#39

Open

RealMangorage added 4 commits September 26, 2024 08:51

Minor fixes

fd40daf

Minor fixes

89bd041

Added the ability to register CapabilityProviders directly for Capabi…

edbd346

…lityFactoryHolder's

More work

86a6094

RealMangorage changed the title ~~1.21.x Capability Factories~~ 1.21.x Capability Factory System PR Sep 27, 2024

RealMangorage marked this pull request as ready for review September 27, 2024 19:18

Jonathing suggested changes Sep 29, 2024

View reviewed changes

Jonathing reviewed Sep 29, 2024

View reviewed changes

More work

5561794

PaintNinja requested changes Sep 29, 2024

View reviewed changes

More work

4bef7b6

LexManos reviewed Sep 30, 2024

View reviewed changes

Added Docs

b32d368

PaintNinja added Needs Update This request requires an update from the author to confirm whether the request is relevant. and removed Needs Update This request requires an update from the author to confirm whether the request is relevant. labels Oct 2, 2024

PaintNinja marked this pull request as draft October 2, 2024 20:22

This was referenced Oct 7, 2024

[1.20.6] Capability Factories PR #9981

Closed

Rework of AttachCapabilitiesEvent #9718

Closed

More work...

0916bd9

autoforge bot added the Needs Rebase This PR requires a rebase to implement upstream changes (usually to fix merge conflicts). label Oct 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1.21.x Capability Factory System PR #10118

1.21.x Capability Factory System PR #10118

RealMangorage commented Sep 21, 2024 •

edited

Loading

Jonathing left a comment

Jonathing Sep 29, 2024

Jonathing Sep 29, 2024

LexManos Sep 30, 2024

Jonathing Sep 29, 2024

Jonathing Sep 29, 2024

Jonathing Sep 29, 2024

Jonathing Sep 29, 2024

Jonathing Sep 29, 2024

Jonathing Sep 29, 2024

Jonathing Sep 29, 2024

Jonathing Sep 29, 2024

Jonathing Sep 29, 2024

Jonathing Sep 29, 2024

PaintNinja left a comment

PaintNinja Sep 29, 2024

LexManos Sep 30, 2024

PaintNinja Sep 29, 2024

Jonathing Sep 29, 2024

PaintNinja Sep 29, 2024

PaintNinja Sep 29, 2024

PaintNinja Sep 29, 2024

RealMangorage Sep 29, 2024

PaintNinja Sep 29, 2024

RealMangorage Sep 29, 2024

LexManos Sep 30, 2024

autoforge bot commented Oct 26, 2024


		import net.minecraft.resources.ResourceLocation;

		public interface ICapabilityFactory<T> {


		public CapabilityFactoryRegisterEvent() {}

		public <G, H extends ICapabilityProvider> void register(Class<G> gClass, ResourceLocation resourceLocation, ICapabilityFactory<G, H> factory) {

1.21.x Capability Factory System PR #10118

Are you sure you want to change the base?

1.21.x Capability Factory System PR #10118

Conversation

RealMangorage commented Sep 21, 2024 • edited Loading

Jonathing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaintNinja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

autoforge bot commented Oct 26, 2024

RealMangorage commented Sep 21, 2024 •

edited

Loading