Add Dynamic ToolStatType to datapack by Qikahome · Pull Request #5573 · SlimeKnights/TinkersConstruct

Qikahome · 2026-01-06T11:29:08Z

It seems work well;
TEST THINGPACK:
testpack.zip

Qikahome · 2026-01-06T11:29:52Z

I know nothing about the test, so it cant build now, but it can run client

KnightMiner

This has the start of something that could work, but it does seem you made some design decisions that will make this very hard to maintain. I left some comments proposing a cleaner design. Feel free to ping me on discord and we can discuss it more in depth.

KnightMiner · 2026-01-08T23:40:50Z

src/main/java/slimeknights/tconstruct/library/materials/IMaterialRegistry.java

+  /**
+   * Clears all dynamic stat types..
+   */
+  void clearDynamicStatTypes();


This should not be publicly exposed. We only need to clear then before stats load.

KnightMiner · 2026-01-08T23:41:17Z

src/main/java/slimeknights/tconstruct/library/materials/IMaterialRegistry.java

+   * Fallback is not supported for dynamic stat types.
+   * @param type  Stat type
+   */
+  void registerDynamicStatType(FlexMaterialStatType type);


This does not seem necessary. Its loaded in JSON, why must it be registered?

If it has to be registered, a JSON only addon can't use it.

KnightMiner · 2026-01-08T23:42:03Z

src/main/java/slimeknights/tconstruct/library/materials/MaterialRegistry.java

+    materialStatTypesLoader = new MaterialStatTypesLoader(() -> {
+      statsLoaded = true;
+      checkAllLoaded();
+    });


Why do we need this? Just put the loading at the start of the material stats loader. You may need to override a method for that. Also ensures stat types are guaranteed to load before we need them instead of being non-deterministic.

Also, setting the same boolean as another loader is just wrong. But since this is the wrong approach don't worry about that part of the comment.

KnightMiner · 2026-01-08T23:43:34Z

src/main/java/slimeknights/tconstruct/library/materials/stats/IMaterialRegister.java

@@ -0,0 +1,5 @@
+package slimeknights.tconstruct.library.materials.stats;
+
+public record IMaterialRegister() {


Why is there an empty record named like an interface here? This seems like an accidental commit.

New classes and public methods should have javadocs explaining how they are used.

KnightMiner · 2026-01-08T23:45:49Z

src/main/java/slimeknights/tconstruct/library/materials/stats/MaterialStatsManager.java

+   * Another registry, cleared on reload.
+   */
+  @Getter
+  private IdAwareComponentRegistry<FlexMaterialStatType> dynamicStatTypes = null;


This probably does not need to be publicly exposed as the other is only exposed so you can register to it.

Part of me thinks it would be better to make this a Map instead of the registry as I am not sure its saving much using that class. Feel free to justify though where its making the code easier.

KnightMiner · 2026-01-09T00:13:27Z

src/main/java/slimeknights/tconstruct/library/materials/stats/types/FlexMaterialStatType.java

+public class FlexMaterialStatType extends MaterialStatType<FlexMaterialStat> {
+
+    public FlexMaterialStatType(MaterialStatsId id, boolean canRepair,
+            LinkedHashMap<String, Supplier<Stat<?, ?>>> stats) {


This should take a List<DynamicStatField> in the constructor.

Also, canRepair should be computed from that list, not passed in separately.

KnightMiner · 2026-01-09T00:14:30Z

src/main/java/slimeknights/tconstruct/library/materials/stats/types/FlexMaterialStatType.java

+
+    public FlexMaterialStatType(MaterialStatsId id, boolean canRepair,
+            LinkedHashMap<String, Supplier<Stat<?, ?>>> stats) {
+        super(id, (FlexMaterialStat) null, null);


I really don't like passing in null to super here, we should be able to create the default from the List<DynamicStatField>, by giving each field a default value.

KnightMiner · 2026-01-09T00:14:59Z

src/test/java/slimeknights/tconstruct/fixture/MaterialRegistryFixture.java

+  @Override
+  public void registerDynamicStatType(FlexMaterialStatType type) {
+    throw new UnsupportedOperationException("No registration possible in test mock");
+  }


These need not even exist in our tests. Entirely internal to the stats manager

KnightMiner · 2026-01-09T00:15:24Z

...ava/slimeknights/tconstruct/library/materials/stats/types/ToolStatTypeNotMatchException.java

+
+import slimeknights.tconstruct.library.tools.stat.ToolStatId;
+
+class ToolStatTypeNotMatchException extends RuntimeException {


Again, why the custom exception? Looks to me like another json parse exception.

KnightMiner · 2026-01-09T00:17:14Z

...test/java/slimeknights/tconstruct/library/materials/stats/UpdateMaterialStatsPacketTest.java

    );

-    UpdateMaterialStatsPacket packetToDecode = sendAndReceivePacket(materialToStats);
+    Collection<FlexMaterialStatType> dynamic = new HashSet<>();// I dont know how to test so just an empty set


The way you test is you write JSON for custom stat types, and then use them to load stats with those types.

From the packet, make sure:

The stat type you send in comes back out the same

stats using the stat type you send in come back out the same

But it also needs tests for JSON parsing:

make sure you can parse a stat type as expected.

make sure stats using the custom stat type parse as expected.

Qikahome · 2026-01-09T14:24:02Z

I wrote the code for the test part, but it seems that ToolStats wasn't registered in the test, which caused my code to throw a JsonParseException.

Qikahome · 2026-01-09T15:00:37Z

newtestpack.zip

Qikahome · 2026-01-09T18:27:04Z

Tests are completed

KnightMiner · 2026-01-10T07:01:05Z

src/test/java/slimeknights/tconstruct/fixture/MaterialStatTypesFixture.java

  public static final List<DynamicStatField<?>> statFields = List.of(
-    new FloatDynamicStatField("test1",ToolStats.DURABILITY,1f,FloatDynamicStatField.Operation.PERCENT, "desc1", "info1"),
-    new FloatDynamicStatField("test2",ToolStats.MINING_SPEED,2f,FloatDynamicStatField.Operation.UPDATE, "desc2", "info2"),
-    new TierDynamicStatField("test3",ToolStats.HARVEST_TIER,Tiers.STONE,"desc3")


Worth noting, we do use these stats in other tests. You might just need to ensure static init is run before the deserialize method runs.

But yeah, fake stats are fine for the tests. I sometimes use "test" as a namespace for them to distinguish them.

src/test/java/slimeknights/tconstruct/library/materials/stats/MaterialStatsManagerTest.java

Qikahome · 2026-01-29T15:33:24Z

May I ask how this PR is going?

KnightMiner · 2026-02-01T04:23:54Z

This is not the only thing I have to work on nor the only PR I have to review right now. You don't have to keep commenting on it requesting an update.

KnightMiner

My main comment on this is you seem to be reinventing the wheel in how DynamicStatField works. You can simplify that a lot if you take advantage of the Loadable API we already use elsewhere.

Feel free to talk to me on #coding on our discord if you have questions about how to do that.

KnightMiner · 2026-02-01T04:24:46Z

src/main/java/slimeknights/tconstruct/library/materials/stats/types/DynamicMaterialStat.java

+	 */
+	@Nullable
+	public DynamicStat getStat(String name)
+	{


Minor, but be consistent on formatting. In this case, brackets go on same line, not newline.

KnightMiner · 2026-02-01T04:25:29Z

src/main/java/slimeknights/tconstruct/library/materials/stats/types/DynamicMaterialStat.java

+	 * @return  Stat, or null if not found
+	 */
+	@Nullable
+	public DynamicStat getStat(String name)


I understand what this method does from the javadoc, but not why it is needed. Does any other class use it or is it just used internally?

In network transmission or serialization, we need specific property values, but it's not feasible to create a getter for every DynamicStat.
So I get the value from the name.

KnightMiner · 2026-02-01T04:26:40Z

src/main/java/slimeknights/tconstruct/library/materials/stats/types/DynamicMaterialStat.java

+
+	@Override
+	public List<Component> getLocalizedInfo() {
+		return stats.values().stream().map(stat->stat.getLocalizedInfo()).collect(Collectors.toList());


Probably worth caching these two lists as part of the constructor since they are not changing.

Also, you can use DynamicStat::getLocalizedInfo for the map call, IDEA has a warning in place if you don't do that so I tend to do it.

KnightMiner · 2026-02-01T04:27:17Z

src/main/java/slimeknights/tconstruct/library/materials/stats/types/DynamicMaterialStat.java

+
+	@Override
+	public void apply(@Nonnull ModifierStatsBuilder builder, float scale) {
+		stats.values().forEach(stat -> {


Similarly to above, IDEA is configured to warn of single line lambda statements; can make this compact as stat -> stat.apply(builder, scale)

KnightMiner · 2026-02-01T04:27:46Z

...in/java/slimeknights/tconstruct/library/materials/stats/types/DynamicMaterialStatRecord.java

+ * A record class that represents a dynamic material stat.
+ */
+public record DynamicMaterialStatRecord(MaterialStatType<?> type, List<DynamicStatField<?>> statFields)
+        implements RecordLoadable<DynamicMaterialStat> {


There is not a line length limit for this repository; keep implements on the same like as the class declaration unless its seriously long (in which case I tend to prefer splitting the record constructor instead).

KnightMiner · 2026-02-01T05:05:40Z

...main/java/slimeknights/tconstruct/library/materials/stats/types/MaterialStatTypeBuilder.java

+    private boolean canRepair = false;
+    @Getter
+    @Setter
+    @Accessors(fluent = true)


Builders should chain, use chain=true.

KnightMiner · 2026-02-01T05:06:32Z

...main/java/slimeknights/tconstruct/library/materials/stats/types/MaterialStatTypeBuilder.java

+     * @return The built material stat type.
+     */
+    public DynamicMaterialStatType build() {
+        return new DynamicMaterialStatType(new MaterialStatsId(registryName), this.canRepair, fields);


Might as well make the field be MaterialStatsId instead of waiting to do so. Add name overload for ResourceLocation if you wish.

KnightMiner · 2026-02-01T05:08:34Z

...main/java/slimeknights/tconstruct/library/materials/stats/types/MaterialStatTypesLoader.java

+ * stat types is "data/foobar/materials/stat_types".
+ */
+@Log4j2
+public class MaterialStatTypesLoader extends MergingJsonDataLoader<MaterialStatTypeBuilder> {


Loaders should not be merging, we do not need multiple sources of what a stat type looks like.

Take a look at MaterialManager for reference.

KnightMiner · 2026-02-01T05:09:49Z

src/main/java/slimeknights/tconstruct/library/materials/stats/MaterialStatsManager.java

    onLoaded.run();
  }

+  public void setDynamicStatTypes(Map<MaterialStatsId, DynamicMaterialStatType> dynamicStatTypes) {


Needs a javadoc. I assume this is set from the packet? If so, an @Internal is a good idea too.

KnightMiner · 2026-02-01T05:11:08Z

...main/java/slimeknights/tconstruct/library/materials/stats/types/MaterialStatTypesLoader.java

+  /** Map of material stat types, keyed by material ID */
+  @Getter
+  @Setter
+  private Map<MaterialStatsId, DynamicMaterialStatType> statTypes;


This being nullable is just leading to a bunch of redundant checks. Just make it default to Map.of() (empty map).

Dynamic ToolStatType

f1f2808

Merge branch 'SlimeKnights:1.20.1' into 1.20.1

618b0c1

KnightMiner requested changes Jan 9, 2026

View reviewed changes

KnightMiner added Feature For pull requests adding in new content, and occasionally larger enhancements Technical Pull requests making changes to workspace or targeted versions 1.20 Issue affects 1.20 labels Jan 9, 2026

Qikahome added 4 commits January 9, 2026 20:50

Refactored the core code

556392b

Correct the format

31780a9

add java docs and correct format

c4c456b

add particial test

f855201

Qikahome requested a review from KnightMiner January 9, 2026 14:20

I can't complete the test.

39f7af6

All tests passed

cf6ba47

KnightMiner reviewed Jan 10, 2026

View reviewed changes

src/test/java/slimeknights/tconstruct/library/materials/stats/MaterialStatsManagerTest.java Outdated Show resolved Hide resolved

Revert Unnecessary changes

01a5a20

Qikahome force-pushed the 1.20.1 branch from 54d649b to 01a5a20 Compare January 10, 2026 08:22

Qikahome requested a review from KnightMiner January 11, 2026 07:05

Merge branch 'SlimeKnights:1.20.1' into 1.20.1

9eff2d9

KnightMiner requested changes Feb 1, 2026

View reviewed changes

Qikahome and others added 3 commits February 9, 2026 15:27

Merge branch 'SlimeKnights:1.20.1' into 1.20.1

57ec770

Partial modification

da6ffdb

Modify the test to match

62f532b

		@@ -0,0 +1,5 @@
		package slimeknights.tconstruct.library.materials.stats;

		public record IMaterialRegister() {


		import slimeknights.tconstruct.library.tools.stat.ToolStatId;

		class ToolStatTypeNotMatchException extends RuntimeException {

Comments

Conversation

Qikahome commented Jan 6, 2026

Uh oh!

Qikahome commented Jan 6, 2026

Uh oh!

KnightMiner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Qikahome commented Jan 9, 2026

Uh oh!

Qikahome commented Jan 9, 2026

Uh oh!

Qikahome commented Jan 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Qikahome commented Jan 29, 2026

Uh oh!

KnightMiner commented Feb 1, 2026

Uh oh!

KnightMiner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants