Quilt Tags API #35

LambdAurora · 2021-10-04T15:09:53Z

Big PR, but very useful features, including:

TagType! A way to assign some behavior to tags like making them load on the client only, or have a tag that have a default on client in-case the server doesn't have a tag, or prevent the loading of a world/server if a tag isn't present to avoid breakage, etc.
TagRegistry! The equivalent of TagFactory from FAPI, includes a new method to accept a TagType parameter, all tags are defaulted to TagType.NEUTRAL otherwise. Also include biome tags.
Do not include an equivalent to FabricDataGeneratorTagBuilder since the class it hooks into is not public, can be ported once global access wideners are a thing perhaps.

Reviews are welcome, testing is encouraged!

LambdAurora · 2021-10-05T09:26:31Z

Latest commit includes the latest tweaks I wanted to include, which tl;dr is preventing log spam of missing entries from tags due to dynamic registries shenanigans.

library/data/tags/src/main/java/org/quiltmc/qsl/tag/api/TagRegistry.java

library/data/tags/src/main/java/org/quiltmc/qsl/tag/api/package-info.java

Co-authored-by: BasiqueEvangelist <basiqueevangelist@yandex.ru>

…aries into tags_api

library/data/tags/src/main/java/org/quiltmc/qsl/tag/api/package-info.java

build-logic/src/main/java/qsl/internal/extension/QslModuleExtension.java

library/data/tags/build.gradle

i509VCB · 2021-10-24T00:30:36Z

library/data/tags/src/main/java/org/quiltmc/qsl/tag/api/QuiltTag.java

+
+/**
+ * Interface implemented by {@link net.minecraft.tag.Tag} instances when QSL is present.
+ *


This semantic has caused issues in the past before. Such an interface should exist for other tag impls but access to functions should be done through static helper functions that may return a value if this data is not present.

Not entirely sure, my big issue with that is static helper functions are way too disconnected on an usage standpoint.
What I could do is force this interface to be implemented with default methods on the Tag interface so no cast exceptions.

Okay a compromise:

Note in the docs that casting to QuiltTag is a usage error. Encourage using the cast function as it is currently named to get the functionality provided by this interface.

Implementations of Tag may implement this interface. A default implementation is provided.

library/data/tags/src/main/java/org/quiltmc/qsl/tag/api/QuiltTagBuilder.java

library/data/tags/src/main/java/org/quiltmc/qsl/tag/api/TagRegistry.java

library/data/tags/src/main/java/org/quiltmc/qsl/tag/impl/TagDelegate.java

library/data/tags/src/main/java/org/quiltmc/qsl/tag/mixin/RequiredTagListRegistryMixin.java

i509VCB · 2021-10-24T00:58:48Z

.../data/tags/src/main/java/org/quiltmc/qsl/tag/mixin/client/ClientPlayNetworkHandlerMixin.java

+
+	@Inject(method = "onDisconnected", at = @At("TAIL"))
+	private void onDisconnected(Text reason, CallbackInfo ci) {
+		// @TODO Replace with networking API?


Make an issue on this todo after this api is merged.

library/data/tags/src/testmod/java/org/quiltmc/qsl/tag/test/client/ClientTagsTestMod.java

library/data/tags/src/main/java/org/quiltmc/qsl/tag/api/package-info.java

Juuxel

I really like the client tags and different tag types (including required tags especially - even though they're a vanilla feature, some APIs like FAPI and Architectury API only expose optionals).

Juuxel · 2021-11-10T22:32:25Z

library/data/tags/src/main/java/org/quiltmc/qsl/tag/api/QuiltTag.java

+	/**
+	 * {@return {@code true} if the given tag has been "replaced" by a data pack at least once}
+	 */
+	boolean hasBeenReplaced();


What's the use case for this? Although I recall Fabric also has this weird API...

I guess it can be for debugging?

I don't know all the details honestly, would need to find which PR added this to FAPI.

Actually found why: FabricMC/fabric#63, original reason sounds sensible.

Edit: asie did explained the entire reason why and the use case: FabricMC/fabric#579 (comment)

I think we should keep it then.

I think it would be useful to mention an example of why this would be used similar to the fabric api comment.

library/data/tags/src/main/java/org/quiltmc/qsl/tag/api/QuiltTagBuilder.java

…aries into tags_api

library/data/tags/src/main/java/org/quiltmc/qsl/tag/api/package-info.java

library/data/tags/src/main/java/org/quiltmc/qsl/tag/impl/TagDelegate.java

BasiqueEvangelist · 2021-11-13T20:52:03Z

library/data/tags/src/main/java/org/quiltmc/qsl/tag/impl/TagRegistryImpl.java

+	}
+
+	public static void forceInit() {
+		// noop. this only forces to run the static initializer.


Why do you need to force running clinit? Isn't only the ThreadLocal and Logger being initialized?

If this isn't ran when it forced, it will cause recursion and very bad crashes.

library/data/tags/src/main/java/org/quiltmc/qsl/tag/impl/TagRegistryImpl.java

library/data/tags/src/main/java/org/quiltmc/qsl/tag/impl/client/ClientTagRegistryManager.java

library/data/tags/src/main/java/org/quiltmc/qsl/tag/impl/QuiltTagHooks.java

library/data/tags/src/main/java/org/quiltmc/qsl/tag/mixin/RequiredTagListMixin.java

library/data/tags/src/main/java/org/quiltmc/qsl/tag/mixin/TagBuilderMixin.java

library/data/tags/src/testmod/java/org/quiltmc/qsl/tag/test/client/ClientTagsTestMod.java

…t/ClientTagRegistryManager.java Co-authored-by: BasiqueEvangelist <basiqueevangelist@yandex.ru>

…legate.java Co-authored-by: BasiqueEvangelist <basiqueevangelist@yandex.ru>

…e-info.java Co-authored-by: BasiqueEvangelist <basiqueevangelist@yandex.ru>

library/data/tags/src/main/java/org/quiltmc/qsl/tag/api/package-info.java

…aries into tags_api

library/data/tags/src/main/java/org/quiltmc/qsl/tag/api/TagRegistry.java

…istry.java Co-authored-by: Juuxel <6596629+Juuxel@users.noreply.github.com>

library/data/tags/src/main/java/org/quiltmc/qsl/tag/api/QuiltTag.java

i509VCB · 2021-11-20T02:27:27Z

library/data/tags/src/main/java/org/quiltmc/qsl/tag/api/QuiltTag.java

+
+/**
+ * Interface implemented by {@link net.minecraft.tag.Tag} instances when QSL is present.
+ *


Okay a compromise:

Note in the docs that casting to QuiltTag is a usage error. Encourage using the cast function as it is currently named to get the functionality provided by this interface.

Implementations of Tag may implement this interface. A default implementation is provided.

library/data/tags/src/main/java/org/quiltmc/qsl/tag/api/TagRegistry.java

i509VCB · 2021-11-20T02:42:26Z

library/data/tags/src/main/java/org/quiltmc/qsl/tag/impl/TagRegistryImpl.java

+@ApiStatus.Internal
+public final class TagRegistryImpl<T> implements TagRegistry<T> {
+	private static final Logger LOGGER = LogManager.getLogger();
+	private static final ThreadLocal<Boolean> MISSING_TAGS_CLIENT_FETCH = new ThreadLocal<>();


Is a thread local an error here? My interpretation of the uses is that the integrated server would never see the client thread signal it that it is fetching tags because the signal would never signal the server's thread.

If it is an error, would AtomicBoolean suffice.

This boolean is used exclusively on the client to check whether there is missing tags that should prevent the connection to the server, the entire goal of this ThreadLocal is to change the behavior of getMissingTags to exclude any tags not required to connect in onSynchronizeTags(SynchronizeTagsS2CPacket).

AtomicBoolean could introduce unwanted behavior on the integrated server.

library/data/tags/src/main/java/org/quiltmc/qsl/tag/impl/TagRegistryImpl.java

i509VCB · 2021-11-20T02:49:41Z

library/data/tags/src/main/java/org/quiltmc/qsl/tag/mixin/TagMixin.java

+
+	@Override
+	default boolean hasBeenReplaced() {
+		return false; // The goal is to prevent hard-fails with custom Tag implementations.


I'm not sure we can prove beyond a reasonable doubt that any implementation of Tag we or anyone else does not explicitly handle may replace it's contents or has had it's contents replaced.

We can't, but not providing a default implementation could lead to bad crashes quickly.

So either we force the implementation of QuiltTag, or we encourage it without forcing.

library/data/tags/src/testmod/java/org/quiltmc/qsl/tag/test/client/ClientTagsTestMod.java

LambdAurora · 2021-11-21T20:53:19Z

Since we're a bit short on time, and all reviews should have been addressed, merging it.

If any issue occurs they always can be corrected in a later PR.

Add Quilt Tags API.

7d718db

LambdAurora added documentation enhancement New feature or request new: module A pull request which adds a new module new: library A pull request which adds a new library. library: data Related to the data library. labels Oct 4, 2021

TheGlitch76 added the hacktoberfest-accepted label Oct 4, 2021

Leo40Git approved these changes Oct 4, 2021

View reviewed changes

Leo40Git mentioned this pull request Oct 4, 2021

Registry Entry Attachment API #29

Merged

6 tasks

Make client tag loading more lenient and fix server crash.

bd4d9f3

BasiqueEvangelist reviewed Oct 5, 2021

View reviewed changes

LambdAurora and others added 6 commits October 5, 2021 22:49

Apply suggestions from code review

c940dce

Co-authored-by: BasiqueEvangelist <basiqueevangelist@yandex.ru>

Fix doc/impl mismatch.

9274c47

qsl$ -> quilt$

abdb0d4

Merge upstream.

97f0427

Add missing remap false.

a486484

Merge branch 'tags_api' of github.com:LambdAurora/quilt-standard-libr…

2d7a145

…aries into tags_api

LambdAurora force-pushed the tags_api branch from d4478b9 to 2d7a145 Compare October 11, 2021 21:11

Use invoke prefix instead of quilt$ for invokers.

6a0ab9d

LemmaEOF approved these changes Oct 17, 2021

View reviewed changes

LambdAurora requested a review from a team October 17, 2021 11:21

williambl reviewed Oct 18, 2021

View reviewed changes

library/data/tags/src/main/java/org/quiltmc/qsl/tag/api/package-info.java Show resolved Hide resolved

Add see alsos to the package info in API class javadocs.

c3ae694

i509VCB reviewed Oct 24, 2021

View reviewed changes

LambdAurora added 3 commits October 30, 2021 20:07

Apply some suggestions.

8f77511

Merge from upstream.

0f685d6

Revert lifecycle events buildscript change.

dcd4d4b

Juuxel reviewed Nov 10, 2021

View reviewed changes

library/data/tags/src/main/java/org/quiltmc/qsl/tag/api/package-info.java Outdated Show resolved Hide resolved

Juuxel approved these changes Nov 10, 2021

View reviewed changes

LambdAurora added 2 commits November 11, 2021 14:02

Add class documentation on TagDelegate.

5d265c7

Merge branch 'tags_api' of github.com:LambdAurora/quilt-standard-libr…

2e32a38

…aries into tags_api

LambdAurora added this to In Progress in Launch Roadmap Nov 13, 2021

LambdAurora added this to the Initial release milestone Nov 13, 2021

BasiqueEvangelist reviewed Nov 13, 2021

View reviewed changes

LambdAurora and others added 3 commits November 13, 2021 23:19

Update library/data/tags/src/main/java/org/quiltmc/qsl/tag/impl/clien…

8f6aabc

…t/ClientTagRegistryManager.java Co-authored-by: BasiqueEvangelist <basiqueevangelist@yandex.ru>

Update library/data/tags/src/main/java/org/quiltmc/qsl/tag/impl/TagDe…

246f2bf

…legate.java Co-authored-by: BasiqueEvangelist <basiqueevangelist@yandex.ru>

Update library/data/tags/src/main/java/org/quiltmc/qsl/tag/api/packag…

a07beb7

…e-info.java Co-authored-by: BasiqueEvangelist <basiqueevangelist@yandex.ru>

IotaBread approved these changes Nov 15, 2021

View reviewed changes

library/data/tags/src/main/java/org/quiltmc/qsl/tag/api/package-info.java Outdated Show resolved Hide resolved

LambdAurora added 3 commits November 18, 2021 00:32

Merge branch '1.18' into tags_api

1346651

Update to 1.18.

b1be504

Merge branch 'tags_api' of github.com:LambdAurora/quilt-standard-libr…

b97db4c

…aries into tags_api

LambdAurora changed the base branch from 1.17.1 to 1.18 November 18, 2021 00:04

LambdAurora added 3 commits November 18, 2021 01:14

Fix some issues.

887ac5e

Improve documentation and fix some issues.

035f805

Improve TagRegistry implementation.

59b9c2d

Juuxel approved these changes Nov 18, 2021

View reviewed changes

library/data/tags/src/main/java/org/quiltmc/qsl/tag/api/TagRegistry.java Outdated Show resolved Hide resolved

LambdAurora and others added 2 commits November 18, 2021 20:25

Update library/data/tags/src/main/java/org/quiltmc/qsl/tag/api/TagReg…

314bb65

…istry.java Co-authored-by: Juuxel <6596629+Juuxel@users.noreply.github.com>

Make QuiltTagBuilder proxy methods for TagBuilder to ease its use.

5224aa7

LambdAurora requested a review from i509VCB November 20, 2021 00:17

LambdAurora changed the title ~~Add Quilt Tags API.~~ Quilt Tags API Nov 20, 2021

i509VCB suggested changes Nov 20, 2021

View reviewed changes

LambdAurora added 2 commits November 20, 2021 11:25

Merge derived header format changes.

53dc088

Add more documentation, and apply suggestions from code review.

21436fc

LambdAurora merged commit 3348c0d into QuiltMC:1.18 Nov 21, 2021

LambdAurora mentioned this pull request Nov 21, 2021

Tags API #11

Closed

TheGlitch76 mentioned this pull request Dec 4, 2021

Data Library TODO #46

Open

4 tasks

OroArmor moved this from In Progress to Done in Launch Roadmap Jan 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Quilt Tags API #35

Quilt Tags API #35

LambdAurora commented Oct 4, 2021 •

edited

Loading

LambdAurora commented Oct 5, 2021

i509VCB Oct 24, 2021

LambdAurora Oct 30, 2021

i509VCB Nov 20, 2021 •

edited

Loading

i509VCB Oct 24, 2021

Juuxel left a comment

Juuxel Nov 10, 2021

LambdAurora Nov 10, 2021

LambdAurora Nov 18, 2021 •

edited

Loading

i509VCB Nov 20, 2021

BasiqueEvangelist Nov 13, 2021

LambdAurora Nov 13, 2021

i509VCB Nov 20, 2021 •

edited

Loading

i509VCB Nov 20, 2021

LambdAurora Nov 20, 2021

i509VCB Nov 20, 2021

LambdAurora Nov 20, 2021

LambdAurora commented Nov 21, 2021

Quilt Tags API #35

Quilt Tags API #35

Conversation

LambdAurora commented Oct 4, 2021 • edited Loading

LambdAurora commented Oct 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i509VCB Nov 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Juuxel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LambdAurora Nov 18, 2021 • edited Loading

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

i509VCB Nov 20, 2021 • edited Loading

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

LambdAurora commented Nov 21, 2021

LambdAurora commented Oct 4, 2021 •

edited

Loading

i509VCB Nov 20, 2021 •

edited

Loading

LambdAurora Nov 18, 2021 •

edited

Loading

i509VCB Nov 20, 2021 •

edited

Loading