Static Resources API #321

cocona20xx · 2023-07-09T04:56:33Z

This post is inaccurate to what now exists in the API, please see the comment made before marking this PR as ready for review!

This is an API for Static Resources: user-controlled data that is accessible early, scope-independent and does not change after it is loaded.

Motivations

Due to the properties of Static Resources, they have uses in creating data-driven APIs that affect the game's state in a way that does not change, and/or in a way that must be done far before vanilla's Resource/Data Packs become available. This makes a Static Resources API essential for a future Content Pack API, as the registries said API would need to modify are static in of themselves and are frozen early on in the game's loading process, making regular resources (in the form of Data Packs) highly unsuited for such an API.

The `Resource` class and mutability of on-disk files

While the first two properties of Static Resources (early access and scope-independence) are already implemented in the current draft, the third property (immutability) is a far more complicated affair, as the needs of each consumer of Static Resources will likely vary heavily in terms of how many times said consumer accesses said resources. Since Resource objects are just pointers to a file and this API primarily returns Resource objects, it is possible for the contents of the file to be modified at runtime, which may cause unexpected behavior for the consumer. To be honest, I (rin) am not sure whether or not ensuring strict immutability of the data provided by Resource instances is something this API should even be concerned with doing—this will need discussion in the Quilt discord and/or in this PR's comments.

The API itself: modder & QSL-side

The API is found in the core/resource_loader package, and is primarily accessed through two StaticResourceLoaderImpl objects. Said objects are accessible outside of the API via the static interface method StaticResourceLoader#get, to which a vanilla ResourceType object is provided—while this currently (subject to change if requested—this could easily be enforced) only dictates where the API gets Resources from, it should be treated the same as in vanilla, where ResourceType.CLIENT_RESOURCES are only available on the client, and ResourceType.SERVER_DATA in all scopes.

The implementation of StaticResourceLoader (and thus the two objects provided, depending on whether server data or client resources are requested) serves as a fairly thin wrapper around another API-internal class, StaticResourceManager, which is currently little more than a re-implementation of the vanilla MultiPackResourceManager class. This was done as MultiPackResourceManager is an essential part of the very much not static vanilla resource loading process, and is targeted by mixins changing things about said process—using an object of that class (or of a child of that class) for this API would lead to it being affected by those mixins.

Currently, consumers of Static Resources are responsible for loading the files provided Resource objects point to themselves.

The API itself: the stuff exposed to end-users

Static Resources are stored within the static/resources and static/data folders: the former being for client-side resources/data and the latter for server-side resources/data. Basically, these folders have a same purpose to vanilla's resources folder and the datapacks folder in each world save, respectively. Like vanilla resource packs and data packs, the files placed in these folders must be contained in packs as well—though the distinction between resource packs and data packs is less concrete with static packs than it is with vanilla's packs.

TODO Checklist before this API is ready for review and merger

Implement the Static Resource Loader (StaticResourceLoader/StaticResourceManagerImpl) and the manager backing it (StaticResourceManager)
Write any additional API infrastructure needed for the Static Resource Loader. Input is needed here!
Write test mod stuff for the Static Resource Loader
Write Javadocs

If data immutability should be ensured by the API:

Create some form of CachedResource class or something along those lines, some form of hashing of the loaded data, etc
Figure out how to even ensure said disk-loaded data immutability and implement it

falkreon

I would have designed this a lot differently (see: StaticData mod). There are some core issues I'd like to address here before I'm happy with the API.

Dividing things into Static Resources versus Data is an interesting idea that I'm going to need to put some thought into, I'll definitely have more to say later on that.

falkreon · 2023-07-13T13:00:28Z

.../resource_loader/src/main/java/org/quiltmc/qsl/resource/loader/api/StaticResourceLoader.java

+	Resource getResourceOrThrow(Identifier id) throws FileNotFoundException;
+	InputStream open(Identifier id) throws IOException;
+	BufferedReader openAsReader(Identifier id) throws IOException;
+	Map<Identifier, JsonElement> findJsonObjects(String namespace, String startingPath);


We've tightly coupled search and decoding here. Even as a helper method, this is awkward. Consider something like

findObjects(Identifier idWithStartingPath).getAsJson();

and while we're at it, consider using the quilt json5 parser instead of the Gson one.

falkreon · 2023-07-13T13:02:20Z

.../resource_loader/src/main/java/org/quiltmc/qsl/resource/loader/api/StaticResourceLoader.java

+	static StaticResourceLoader get(ResourceType type){
+		return StaticResourceLoaderImpl.get(type);
+	}
+	List<Resource> getAllResources(Identifier id);


Seems like copying vanilla respacks and datapacks for no benefit. I'd much rather have a good search system and let people search recursively/non-recursively at root than have this method exclusively for a case basically no one needs.

falkreon · 2023-07-13T13:03:10Z

...esource_loader/src/main/java/org/quiltmc/qsl/resource/loader/impl/StaticResourceManager.java

+import java.util.stream.Stream;
+
+/**
+ * Basically a glorified re-implementation of {@link MultiPackResourceManager} to avoid mixins to said class effecting static data loading.


falkreon · 2023-07-13T13:04:10Z

...esource_loader/src/main/java/org/quiltmc/qsl/resource/loader/impl/StaticResourceManager.java

+	/**
+	 * Based heavily on {@link MultiPackResourceManagerMixin#quilt$recomputeNamespaces()}
+	 */
+	private void computeNamespaces(){


again, seems like a lot of moving parts for what's essentially a copy of a little-used feature from vanilla.

falkreon · 2023-07-13T13:05:33Z

.../resource_loader/src/main/java/org/quiltmc/qsl/resource/loader/api/StaticResourceLoader.java

+	List<Resource> getAllResources(Identifier id);
+	Set<String> getNamespaces();
+	List<ResourcePack> getPacks();
+	Map<Identifier, List<Resource>> findAllResources(String startingPath, Predicate<Identifier> pathFilter);


Is there a reason the static resource doesn't contain provenance information? Is it just to reuse the Resource class? This could bake down into a List or a Stream if we wanted it to, and users could mapreduce it if they actually cared about the properties that make it a map here.

falkreon · 2023-07-13T13:06:54Z

.../resource_loader/src/main/java/org/quiltmc/qsl/resource/loader/api/StaticResourceLoader.java

+	Stream<ResourcePack> streamPacks();
+	Optional<Resource> getResource(Identifier id);
+	Resource getResourceOrThrow(Identifier id) throws FileNotFoundException;
+	InputStream open(Identifier id) throws IOException;


Probably tight coupling. Consider

findFirst(id).open();

...y/core/resource_loader/src/main/java/org/quiltmc/qsl/resource/loader/api/ResourceLoader.java

cocona20xx · 2023-07-15T23:10:04Z

After quite a bit of refactoring work, I'd say this is finally ready for review. Let's go over what's changed since the first post:

StaticResourceLoader no longer exists, and is simply part of the main ResourceLoader class and impl.
StaticResourceManager is now a child class of MultiPackResourceManager that implements a wrapper interface—the wrapper interface is also implemented by MultiPackResourceManagerMixin to prevent cast-related exceptions. Said wrapper interface's helper method is used in said mixin to check if the MultiPackResourceManager object mixin methods are currently running in are StaticResourceManager objects, at which point they immediately return (if a void method) or pass the original value (if a @ModifyVariable mixin method). Future mixins added to MultiPackResourceMangerMixin should use this wrapper to prevent them from being ran within StaticResourceManagers.
No helper methods for working with JSON exist anymore—these will probably be a future PR, and work with all Resource objects, not just static ones.
Pack loading actually works now, and tests exist to prove that it works.

Thank you to everyone who gave feedback and advice in the discord and on here during the process of developing this PR, it wouldn't have been possible to write it otherwise.

LambdAurora

If you are using Intellij, please import the checkstyle file as the code style for this project as several part of the PR are not compliant with the code style of QSL.

LambdAurora · 2023-07-15T23:15:33Z

...y/core/resource_loader/src/main/java/org/quiltmc/qsl/resource/loader/api/ResourceLoader.java

+	 * @param type the given resource type
+	 * @return the static resource manager instance
+	 */
+	static @NotNull MultiPackResourceManager getStaticResourceManager(@NotNull ResourceType type){


Any reason to return MultiPackResourceManager and not just ResourceManager?
So far ResourceManager is usually used for APIs that provide a resource manager.

Also, given the API structure, this should not be static but be something to implement directly on the ResourceLoaderImpl class like every other method present in this interface.

The former I agree with and will be changed accordingly in the next commit, but the latter I have to disagree with—the creation and storage of the StaticResourceManagers that back the Static Resources API are both entirely static-scoped, so it's both less verbose and more aligned with the data structure of the API itself to get the static resource managers in a static method.

If this is a sticking point I can change it, but I feel like it makes more sense this way.

I'm inclined to agree that this should be static, given that a static resource manager is not tied to a ResourceLoader in any way

LambdAurora · 2023-07-15T23:16:05Z

...e/resource_loader/src/main/java/org/quiltmc/qsl/resource/loader/impl/ResourceLoaderImpl.java

@@ -98,13 +105,11 @@ public final class ResourceLoaderImpl implements ResourceLoader {
 			.toBooleanOrElse(QuiltLoader.isDevelopmentEnvironment());
 	private static final boolean DEBUG_RELOADERS_ORDER = TriState.fromProperty("quilt.resource_loader.debug.reloaders_order")
 			.toBooleanOrElse(false);
-


I would highly appreciate to not remove blank lines that are present for readability.

Fixed in next commit, hopefully?

LambdAurora · 2023-07-15T23:19:08Z

...e/resource_loader/src/main/java/org/quiltmc/qsl/resource/loader/impl/ResourceLoaderImpl.java

+	private static List<ResourcePack> findUserStaticPacks() {
+		List<ResourcePack> returnList = new ArrayList<>();
+		File directoryFile = QuiltLoader.getGameDir().resolve(STATIC_PACK_ROOT).toFile();
+		File[] potentialPackFiles = directoryFile.listFiles(filterFile -> {


Given that most things use the NIO Path API, it might make more sense to use the Files NIO API to search packs.

Tested implementing it that way, and it both works and is slightly cleaner overall so it'll be swapped to using Files.walk in the next commit where listFiles is currently.

LambdAurora · 2023-07-15T23:20:25Z

..._loader/src/main/java/org/quiltmc/qsl/resource/loader/impl/StaticResourceManagerWrapper.java

+ * @see StaticResourceManager
+ */
+@ApiStatus.Internal
+public interface StaticResourceManagerWrapper {


This is useless, a simple instanceof StaticResourceManager is already sufficient for checking.

Would the mixin need to implements ResoruceManager for that to work? I'm presuming yes, and am going to test it while working on the upcoming commit—if it doesn't work I'll post proof as screenshot or video.

Couldn't get it to work with both implements AutoCloseableResourceManager & implements ResourceManager. I have exactly zero clue why this is the case—logic says it should work, but it just doesn't.

You need to cast to object before you instanceof. Java is smart and finds impossible conditions

Yup, turns out it was an 'IDEA and the compiler don't understand mixins' moment and not an actual error—this has been fixed and thus said wrapper interface will be gone in next commit.

LambdAurora · 2023-07-15T23:21:50Z

...rce_loader/src/testmod/java/org/quiltmc/qsl/resource/loader/test/StaticResourcesTestMod.java

+			MultiPackResourceManager clientManager = ResourceLoader.getStaticResourceManager(ResourceType.CLIENT_RESOURCES);
+			Resource cronch = clientManager.getResource(new Identifier("cronch", "test_client")).get();
+			BufferedReader readerCronch = cronch.openBufferedReader();
+			LOGGER.error(readerCronch.readLine() + "(Reading this line should be impossible!)");


Please use the var-args system of the logger to avoid passing unsafe strings.
Given this is a test mod it's not as bad, but they're also references on how to use an API, so they should not accidentally give bad practices.

Fixed in next commit.

cocona20xx · 2023-07-16T01:55:33Z

...y/core/resource_loader/src/main/java/org/quiltmc/qsl/resource/loader/api/ResourceLoader.java

+	 * @param type the given resource type
+	 * @return the static resource manager instance
+	 */
+	static @NotNull ResourceManager getStaticResourceManager(@NotNull ResourceType type) {


With the most recent commit, everything should be in order—bar the de-staticing of this method, which I'm opposed to, though if it's a sticking point for @LambdAurora I can change it. See #321 (comment) for the reasoning on why that hasn't been done.

Given that static resource managers are unrelated to the normal resource loaders, really - which handle non static data - I am also opposed to de-staticing this

falkreon · 2023-07-16T17:06:05Z

...e/resource_loader/src/main/java/org/quiltmc/qsl/resource/loader/impl/ResourceLoaderImpl.java

+				if (pathAsFile.isFile()) {
+					if (pathAsFile.toPath().toString().endsWith(".zip")) {
+						returnList.add(new ZipResourcePack(n, pathAsFile, false));
+					} else if (!pathAsFile.getName().equals(".DS_Store")) {


if we're going to silently ignore filesystem helper files "like .DS_Store" instead of ignoring specifically .DS_Store, can we do something like

private static final Set<String> IGNORED_FILES = Set.of(".DS_Store"); ... } else if (!IGNORED_FILES.contains(pathAsFile.getName())) { ... }

Otherwise, if there will definitely never be any other fs helper files, we could just delete the comment

are thumbs.db files still around?

What does vanilla do for detecting packs in the resource packs folder? I feel like we should probably copy that

Vanilla doesn't actually ignore .DS_Store files and similar (it throws an error if they're inside packs!), but it silently doesn't load loose files as a pack, same as is done here. Telling users that loose files aren't supported is a thing since other static data implementations do allow for them, so it felt pertinent to say 'hey this API doesn't do that'.

It's worth noting that both the macOS and Windows filesystems are case-insensitive by default, so a plain equals won't suffice, either. And... there are many more files like this, including but not limited to ._* on macos, .directory on some Linux DEs, desktop.ini on windows, etc etc

Anyways, the fs helper file thing has been reimplemented with thumbs.db and .DS_Store in the set, and has a message to make an issue on the main github if an OS filesystem helper is warned of as a loose file

It's worth noting that both the macOS and Windows filesystems are case-insensitive by default, so a plain equals won't suffice, either. And... there are many more files like this, including but not limited to ._* on macos, .directory on some Linux DEs, desktop.ini on windows, etc etc

Yaaay... eating some food rn but will go back and do another pass on this afterwards—thankfully Set has a case-insensitive contains method built-in, so that part won't be too big of an issue...

cocona20xx · 2023-07-17T18:19:54Z

Unless anyone else has suggestions for additions again, this should be ready for re-review by @LambdAurora

...e/resource_loader/src/main/java/org/quiltmc/qsl/resource/loader/impl/ResourceLoaderImpl.java

LambdAurora

Please space out your code for readability.

...e/resource_loader/src/main/java/org/quiltmc/qsl/resource/loader/impl/ResourceLoaderImpl.java

...esource_loader/src/main/java/org/quiltmc/qsl/resource/loader/impl/StaticResourceManager.java

...oader/src/main/java/org/quiltmc/qsl/resource/loader/mixin/MultiPackResourceManagerMixin.java

...y/core/resource_loader/src/main/java/org/quiltmc/qsl/resource/loader/api/ResourceLoader.java

...e/resource_loader/src/main/java/org/quiltmc/qsl/resource/loader/impl/ResourceLoaderImpl.java

cocona20xx · 2023-08-16T19:04:16Z

Could we potentially get some more movement on this soonish, now that QFAPI sync is back in order?

...y/core/resource_loader/src/main/java/org/quiltmc/qsl/resource/loader/api/ResourceLoader.java

cocona20xx · 2023-10-26T21:50:24Z

Should be ready for 1.20.2 now—I have absolutely no clue why there's conflicts in ResourceLoaderImpl, however. Both supposed conflicts seem to just be adding stuff? Not sure why its marking them as conflicts, but it took quite a few attempts to get this to merge at all locally, as it tended to get stuck in an infinite loop of trying to resolve the same conflicts over and over again.

cocona20xx · 2024-01-31T02:00:44Z

Ready for 1.20.2 again if all checks pass—ResourceLoaderImpl's conflicts are git being weird just like last time

OroArmor · 2024-01-31T02:01:54Z

this might be hard but i would try rebasing your PR off of 1.20.2. That should hopefully fix the conflict

cocona20xx · 2024-01-31T02:03:20Z

Have tried doing that and it gets stuck in the infinite loop usually. Might try doing it again later

Basically a manual rebase/merge onto upstream 1.20.2—actually doing either resulted in things breaking horribly because git moment Also fixed the quilt.mod.json for the resource_loader package—it still had the old name for the ClientLoaderEventsTestMod test (formerly ClientResourceLoaderEventsTestMod)

ix0rai added new: module A pull request which adds a new module library: core Related to the core library. t: new api This adds a new API. s: wip This pull request is being worked on. labels Jul 9, 2023

falkreon reviewed Jul 13, 2023

View reviewed changes

lukebemish reviewed Jul 15, 2023

View reviewed changes

...y/core/resource_loader/src/main/java/org/quiltmc/qsl/resource/loader/api/ResourceLoader.java Outdated Show resolved Hide resolved

cocona20xx marked this pull request as ready for review July 15, 2023 23:10

LambdAurora suggested changes Jul 15, 2023

View reviewed changes

cocona20xx commented Jul 16, 2023

View reviewed changes

falkreon reviewed Jul 16, 2023

View reviewed changes

cocona20xx requested a review from LambdAurora July 17, 2023 21:08

embeddedt reviewed Jul 17, 2023

View reviewed changes

...e/resource_loader/src/main/java/org/quiltmc/qsl/resource/loader/impl/ResourceLoaderImpl.java Outdated Show resolved Hide resolved

LambdAurora suggested changes Jul 19, 2023

View reviewed changes

LambdAurora reviewed Jul 19, 2023

View reviewed changes

...y/core/resource_loader/src/main/java/org/quiltmc/qsl/resource/loader/api/ResourceLoader.java Outdated Show resolved Hide resolved

cocona20xx requested a review from LambdAurora July 19, 2023 23:48

falkreon reviewed Jul 22, 2023

View reviewed changes

...e/resource_loader/src/main/java/org/quiltmc/qsl/resource/loader/impl/ResourceLoaderImpl.java Outdated Show resolved Hide resolved

OroArmor approved these changes Aug 28, 2023

View reviewed changes

TheGlitch76 requested changes Sep 15, 2023

View reviewed changes

...y/core/resource_loader/src/main/java/org/quiltmc/qsl/resource/loader/api/ResourceLoader.java Outdated Show resolved Hide resolved

cocona20xx requested a review from TheGlitch76 September 16, 2023 20:24

cocona20xx changed the base branch from 1.20 to 1.20.2 October 26, 2023 21:46

OroArmor approved these changes Oct 28, 2023

View reviewed changes

cocona20xx closed this Feb 3, 2024

cocona20xx force-pushed the feature/static-resources branch from efb892f to 5da1018 Compare February 3, 2024 01:03

cocona20xx reopened this Feb 3, 2024

Static Resources API #321

Are you sure you want to change the base?

Static Resources API #321

Conversation

cocona20xx commented Jul 9, 2023 • edited Loading

This post is inaccurate to what now exists in the API, please see the comment made before marking this PR as ready for review!

Motivations

The Resource class and mutability of on-disk files

The API itself: modder & QSL-side

The API itself: the stuff exposed to end-users

TODO Checklist before this API is ready for review and merger

If data immutability should be ensured by the API:

falkreon 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

cocona20xx commented Jul 15, 2023

LambdAurora 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

cocona20xx Jul 16, 2023 • edited Loading

Choose a reason for hiding this comment

cocona20xx Jul 16, 2023 • 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

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

kb-1000 Jul 16, 2023 • edited Loading

Choose a reason for hiding this comment

cocona20xx Jul 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cocona20xx commented Jul 17, 2023

LambdAurora left a comment

Choose a reason for hiding this comment

cocona20xx commented Aug 16, 2023

cocona20xx commented Oct 26, 2023

cocona20xx commented Jan 31, 2024

OroArmor commented Jan 31, 2024

cocona20xx commented Jan 31, 2024

cocona20xx commented Jul 9, 2023 •

edited

Loading

The `Resource` class and mutability of on-disk files

cocona20xx Jul 16, 2023 •

edited

Loading

cocona20xx Jul 16, 2023 •

edited

Loading

kb-1000 Jul 16, 2023 •

edited

Loading

cocona20xx Jul 16, 2023 •

edited

Loading