Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[1.21.3] Implement builtin mod resource/data pack sorting #4070

Draft
wants to merge 7 commits into
base: 1.21.3
Choose a base branch
from

Conversation

Apollounknowndev
Copy link
Contributor

@Apollounknowndev Apollounknowndev commented Sep 5, 2024

Supersedes #3913, targets 1.21.2 and changed how sorting works.

Overriding data/resource pack files from other mods is currently quite difficult as mod loading order isn't consistent. This can be especially annoying for compatibility mods that exist to override data/resource files from other mods.

This PR addresses the issue by implementing a system for mods to define in their fabric.mod.json what their priority is in the internal Fabric Mods data/resource pack relative to other mods. For example, a pack that overrides trees from a biome mod can set itself to load above the biome mod's datapack internally to make sure files are consistently overriden.

Mods that wish to change their priority relative to other in the load order can add this to their fabric.mod.json file:

"custom": {
    "fabric:resource_load_order": {
        "before": [
          "mod_a",
          "mod_c",
        ],
        "after": [
          "mod_d"
        ]
    }
}

Mods in the before list will load their resources before/under the mod, mods in the after list will load their resources after/above the mod.

@modmuss50
Copy link
Member

I think this is fine, I havent looked at it in depth, but the idea seems solid. I think we discussed this before, but are we happy to have this in the fabric.mod.json as opposed to the pack.mcmeta file? I think im happy either way.

This clearly needs some tests, if you want some help setting up multiple testmods for one module let me know, it should be tottaly possible to do.

@modmuss50 modmuss50 added the enhancement New feature or request label Sep 12, 2024
@Apollounknowndev
Copy link
Contributor Author

It would be possible to move this to the pack.mcmeta file, but it'd require refactoring how mod packs are registered in an awkward way (Mod resource packs would need to have their profiles built, then sorted, then registered) and requires an additional accessor to get the resource pack metadata so I didn't opt for it. If there's an advantage to moving it to pack.mcmeta that makes it worth the hassle lmk.
I've tried to setup test mods but I'm not sure how to do the gradle wizardry, some help on that would be much appreciated.

@modmuss50
Copy link
Member

I have added 3 blank test mods, hopefully this will help you make some tests out of them. I also made some small code cleanup/changes with unit testing in mind, but ran out of time. Feel free to take a look at this, if not I might come back to it.

@Apollounknowndev
Copy link
Contributor Author

I've tried getting the testmod to load but it crashes with NullPointerException: Cannot invoke "java.util.jar.Manifest.getMainAttributes()" because "manifest" is null :P

@modmuss50
Copy link
Member

I've tried getting the testmod to load but it crashes with NullPointerException: Cannot invoke "java.util.jar.Manifest.getMainAttributes()" because "manifest" is null :P

Sorry I missed this. Where are you seeing this problem?


if (pack != null) {
packs.add(pack);
if (array != null && array.getType() == CustomValue.CvType.ARRAY) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should reject or at least warn about unsupported types. It might be nice to support a direct string value since that's what would happen for most mods using this feature ("after": "mod_xy")

@Apollounknowndev
Copy link
Contributor Author

Apollounknowndev commented Oct 24, 2024

Sorry I missed this. Where are you seeing this problem?

This occurs when trying to run runTestmodClient or runTestmodServer. The mod list shows testmod-a and testmod-b but doesn't get to testmod-c without crashing with a Failed to remap mods! error.

@modmuss50 modmuss50 changed the base branch from 1.21.2 to 1.21.3 November 14, 2024 18:19
@modmuss50 modmuss50 changed the title [1.21.2] Implement builtin mod resource/data pack sorting [1.21.3] Implement builtin mod resource/data pack sorting Nov 14, 2024
@Apollounknowndev
Copy link
Contributor Author

Finally have a few tests in using the three test mods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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