Refactor resource loader internals #3473

apple502j · 2023-12-18T12:59:32Z

Resolves #3302
Supersedes #3312

Likely the biggest refactor to Resource Loader since its inception??? Work ongoing.

Note, if you've been using FAPI resource pack internals, this might break it (as expected).

Split each mod's resource packs.
Make sure not to break programmer art/high contrast.
Add pack dependency system.
Add a placeholder pack, to be used only for UI purposes.
Hide internal/per-mod packs.
Improve documentation.
Improve internal names.
Test.

…s used as special chars...

...der-v0/src/client/java/net/fabricmc/fabric/mixin/resource/loader/client/PackScreenMixin.java

NebelNidas

Not an in-depth review yet, but I've noticed that the misleading ResourcePack#getName() Yarn name still hasn't been fixed (partly my fault, I forgot to create an issue).
I'd rename the corresponding fields and translation keys accordingly to avoid confusion later down the line. Also pinging @PepperCode1 to check if Continuity has any special requirements regarding the format

...ce-loader-v0/src/main/java/net/fabricmc/fabric/impl/resource/loader/ModResourcePackUtil.java

apple502j · 2023-12-27T14:12:54Z

Marked ready for review. Will need some testing.

apple502j · 2023-12-28T03:54:00Z

Test successful, but will need more checks on mod compatibility, ResourceManagerHelper pack interactions, etc.

apple502j · 2023-12-28T09:12:36Z

TODO:

Fix reordering of required packs being broken

apple502j · 2023-12-28T11:37:21Z

...ient/java/net/fabricmc/fabric/mixin/resource/loader/client/GameOptionsWriteVisitorMixin.java

+
+	@SuppressWarnings("unchecked")
+	@ModifyArg(method = "visitObject", at = @At(value = "INVOKE", target = "Ljava/util/function/Function;apply(Ljava/lang/Object;)Ljava/lang/Object;"))
+	private <T> T skipHiddenPacks(T value, @Local String key) {


Injecting here is much easier than maintaining the ordinals among the visitObject calls in accept.

fabric-resource-loader-v0/src/main/resources/fabric.mod.json

modmuss50

Looks good, nothing major is standing out to me. The main thing with this is going to be testing, to A) make sure it works as expected, and B) make sure its not slower anywhere.

Ensuing that the test mod fully utilises the features, and documents how to test it works correctly would go a long way. Im very happy to help with this if needed.

...der-v0/src/main/java/net/fabricmc/fabric/mixin/resource/loader/ResourcePackManagerMixin.java

...oader-v0/src/main/java/net/fabricmc/fabric/impl/resource/loader/PlaceholderResourcePack.java

...ce-loader-v0/src/main/java/net/fabricmc/fabric/impl/resource/loader/ModResourcePackUtil.java

modmuss50

Looks good, what kind of testing has been done on this? There is quite a bit of room for things to go wrong so want to be 100% sure that we are confident with it.

Im happy to move it to last call.

...der-v0/src/main/java/net/fabricmc/fabric/impl/resource/loader/FabricResourcePackProfile.java

...ce-loader-v0/src/main/java/net/fabricmc/fabric/impl/resource/loader/ModResourcePackUtil.java

modmuss50 · 2024-01-15T14:33:00Z

...v0/src/test/java/net/fabricmc/fabric/test/resource/loader/unit/ModResourcePackUtilTests.java

+		allProfiles.putAll(modAProfiles);
+		allProfiles.putAll(modBProfiles);
+
+		testRefreshAutoEnabledPacks(


Nit: You would usually have a test method for each of these, not the end of the world but would make it more clear if just one of these tests broke.

Happy to leave this be.

* First step toward fixing resource pack grouping * Placeholder pack and pack dependency * Various fixes * Fix wrong variable in serialization code * Hide packs in PackScreen and DatapackCommand * Apparently Japanese people aren't alone in having their currency signs used as special chars... * Inject directly to Pack * Add temporary logging, fix bug * Add proper sorting * Improve logging * Fix duplicate name registration * Fix client pack handling * Fix FMJ * Stop using interface injection for internal interface * Delete unused GroupResourcePack * Move refreshAutoEnabledPacks to util * Improve logging * Make a few things private * Use vanilla metadata serialization logic * Improve javadoc * Add junit test * Some final refactors * Update ja_jp.json --------- Co-authored-by: modmuss <[email protected]> (cherry picked from commit 707e4d1)

catter1 · 2024-01-28T18:34:57Z

Thank you so much for working on this! I am excited to test it out, and be able to easily create updates for Fabric again.

jellysquid3 · 2024-01-31T04:39:48Z

This has caused breakages in Sodium, because the high contrast/programmer art packs for mods are given the same resource pack name, which it expects to not be possible.

apple502j · 2024-01-31T05:03:37Z

@jellysquid3 Uhh, my test environment did have Sodium 0.5.5, and I did not encounter any errors while booting up or using High Contrast. It could be due to changes in 0.5.6+, I am honestly unsure.

because the high contrast/programmer art packs for mods are given the same resource pack name, which it expects to not be possible.

Each ResourcePackProfile should have a different name, but it seems like I forgot to override ResourcePack#getName. This is my oversight, yep.

Additionally, this causes massive regressions

The change should have removed any "internal" packs from PackResourceOrganizer, making them invisible to the client. The screenshot obviously says that was not the case, though. Could you provide more info on how to reproduce the issue?

jellysquid3 · 2024-01-31T05:08:39Z

It could be due to changes in 0.5.6+, I am honestly unsure.

We made a change very shortly before Fabric API 0.95.3 was published (a few days ago) that put the resource scan results into a hash table, which doesn't tolerate duplicate keys. It was just really bad luck, and we have since fixed the code to be more tolerant of that case.

The screenshot obviously says that was not the case, though. Could you provide more info on how to reproduce the issue?

This may have since been fixed. Another user reported it to us with Fabric API 0.95.3, but will see if I can reproduce it myself.

dicedpixels · 2024-02-01T17:06:01Z

This happens when "Resource Pack Overrides" is present.

apple502j added 2 commits December 17, 2023 20:00

First step toward fixing resource pack grouping

d871109

Placeholder pack and pack dependency

dcffe91

modmuss50 added the enhancement New feature or request label Dec 21, 2023

apple502j added 4 commits December 22, 2023 00:39

Various fixes

aa5e0a4

Fix wrong variable in serialization code

50665fc

Hide packs in PackScreen and DatapackCommand

689413f

Apparently Japanese people aren't alone in having their currency sign…

9cb6b31

…s used as special chars...

maityyy reviewed Dec 22, 2023

View reviewed changes

...der-v0/src/client/java/net/fabricmc/fabric/mixin/resource/loader/client/PackScreenMixin.java Outdated Show resolved Hide resolved

apple502j added 2 commits December 22, 2023 21:17

Inject directly to Pack

efe6941

Add temporary logging, fix bug

df32795

NebelNidas suggested changes Dec 23, 2023

View reviewed changes

apple502j commented Dec 23, 2023

View reviewed changes

...ce-loader-v0/src/main/java/net/fabricmc/fabric/impl/resource/loader/ModResourcePackUtil.java Show resolved Hide resolved

Add proper sorting

ea2b529

apple502j marked this pull request as ready for review December 27, 2023 14:12

apple502j added 2 commits December 28, 2023 02:08

Improve logging

0bf7686

Fix duplicate name registration

2cac02c

Fix client pack handling

76ae3b0

apple502j commented Dec 28, 2023

View reviewed changes

Fix FMJ

02e45d7

Juuxel reviewed Dec 29, 2023

View reviewed changes

fabric-resource-loader-v0/src/main/resources/fabric.mod.json Outdated Show resolved Hide resolved

modmuss50 reviewed Dec 29, 2023

View reviewed changes

apple502j added 6 commits January 2, 2024 21:31

Stop using interface injection for internal interface

94ad868

Delete unused GroupResourcePack

eb74aba

Move refreshAutoEnabledPacks to util

683d6b7

Improve logging

f45deb3

Make a few things private

fc1a71e

Use vanilla metadata serialization logic

c0af7c9

apple502j added 2 commits January 2, 2024 22:47

Improve javadoc

0d4d8ab

Add junit test

2c92a17

misode mentioned this pull request Jan 7, 2024

1.20.4 errors for data packs Gamemode4Dev/GM4_Datapacks#936

Closed

modmuss50 approved these changes Jan 15, 2024

View reviewed changes

apple502j added 2 commits January 17, 2024 22:21

Some final refactors

a5e12d9

Update ja_jp.json

150d210

modmuss50 mentioned this pull request Jan 19, 2024

Support resource pack filters, overlays and features from pack.mcmeta files in mods. #3312

Closed

modmuss50 added the last call If you care, make yourself heard right away! label Jan 19, 2024

apple502j mentioned this pull request Jan 23, 2024

[Suggestion]: Allow no-code mods to specify that their resources must apply after another mod #3543

Open

Merge branch '1.20.4' into resource-loader/grouping-redo

c577cf0

modmuss50 added the merge me please Pull requests that are ready to merge label Jan 28, 2024

modmuss50 merged commit 707e4d1 into FabricMC:1.20.4 Jan 28, 2024
5 checks passed

This comment was marked as off-topic.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor resource loader internals #3473

Refactor resource loader internals #3473

apple502j commented Dec 18, 2023 •

edited

Loading

NebelNidas left a comment

apple502j commented Dec 27, 2023

apple502j commented Dec 28, 2023

apple502j commented Dec 28, 2023 •

edited

Loading

apple502j Dec 28, 2023 •

edited

Loading

modmuss50 left a comment

modmuss50 left a comment

modmuss50 Jan 15, 2024

catter1 commented Jan 28, 2024

jellysquid3 commented Jan 31, 2024

This comment was marked as off-topic.

apple502j commented Jan 31, 2024

jellysquid3 commented Jan 31, 2024

dicedpixels commented Feb 1, 2024 •

edited

Loading

Refactor resource loader internals #3473

Refactor resource loader internals #3473

Conversation

apple502j commented Dec 18, 2023 • edited Loading

NebelNidas left a comment

Choose a reason for hiding this comment

apple502j commented Dec 27, 2023

apple502j commented Dec 28, 2023

apple502j commented Dec 28, 2023 • edited Loading

apple502j Dec 28, 2023 • edited Loading

Choose a reason for hiding this comment

modmuss50 left a comment

Choose a reason for hiding this comment

modmuss50 left a comment

Choose a reason for hiding this comment

modmuss50 Jan 15, 2024

Choose a reason for hiding this comment

catter1 commented Jan 28, 2024

jellysquid3 commented Jan 31, 2024

This comment was marked as off-topic.

apple502j commented Jan 31, 2024

jellysquid3 commented Jan 31, 2024

dicedpixels commented Feb 1, 2024 • edited Loading

apple502j commented Dec 18, 2023 •

edited

Loading

apple502j commented Dec 28, 2023 •

edited

Loading

apple502j Dec 28, 2023 •

edited

Loading

dicedpixels commented Feb 1, 2024 •

edited

Loading