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

[BC] Change how resource pack grouping works and group mod data packs #138

Closed
wants to merge 10 commits into from

Conversation

lukebemish
Copy link
Contributor

@lukebemish lukebemish commented Sep 13, 2023

Currently, NeoForge groups resource packs from various mods into a single resource pack via DelegatingPackResources, a PackResources which groups others together. Doing so requires adding the getChildren extension to PackResources, and changes the nature of PackResources from "a single member of a resource stack" to "one or more layers of a resource stack". This is, in most situations, not an issue.

However, vanilla has added more and more features that are controllable by the pack.mcmeta file - and accessed through the getMetadataSection of PackResources, including overlays and filtering. Data and resource packs make use of these features; mods may as well. The use of DelegatingPackResources to group mod resources means that this information is lost, and mods (as well as resource packs packaged as low-code mods) cannot make use of these features. Additionally, the changed behaviour of PackResources can easily lead to confusion while using them, as I have experienced personally - though this particular issue could be mitigated by some javadoc changes.

While for resource packs these limits are rather benign ("could cause issues" not "has caused issues"), as the features in question aren't used by many resource packs and low-code resource packs are rare, they become rather more problematic if, in the future, we wished to group mod data, not just assets - which is probably a good idea to do at some point. Low-code datapacks are much more common, and filters and overlays are much more useful in datapacks.

My proposal here is to switch the grouping from PackResources to Pack; each Pack may now optionally hold a number of "child" packs (which are required to be marked as hidden). When the selected pack list is being rebuilt in PackRepository, these child packs are injected, in order, directly beneath the parent pack; this means that they will stay with the parent pack, even as packs are re-ordered, shuffled around, whatever! From the user perspective, only the parent pack is visible (if it isn't hidden). From a code perspective, listing selected packs from a PackRepository and onwards, all the packs in question are visible, with the child packs always directly below the parent pack, in order.


This PR currently switches NeoForge's mod asset loading to use grouped packs instead of grouped pack resources, and replaces DelegatingPackResources with EmptyPackResources, a PackResources that has metadata and nothing else. I would welcome feedback on the approach in general, but am also looking specifically for good test cases to make sure I didn't frick anything up.

Beyond changing resource pack grouping, this PR uses the new grouping system to consolidate logic for mod data and resource pack setup - which includes grouping mod datapacks together as well. This should ensure that mod datapacks are always placed in mod load order, not "order I installed the mods in" order as they currently are, which should help solve some of the funky issues that can pop up there.

@CLAassistant
Copy link

CLAassistant commented Sep 13, 2023

CLA assistant check
All committers have signed the CLA.

@lukebemish lukebemish changed the title Change how resource pack grouping works [BC] Change how resource pack grouping works Sep 13, 2023
Copy link
Member

@XFactHD XFactHD left a comment

Choose a reason for hiding this comment

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

Overall this looks significantly cleaner in practice than in my mind. I quickly threw a build of this at AtlasViewer together with JEI to have another mod with resources in the environment and my "source pack" collection worked flawlessly with this and became simpler as well due to not needing a special case for the delegating pack (which I completely forgot existed when this was discussed on Discord).

I've noted a few issues and a few minor changes.

@lukebemish lukebemish changed the title [BC] Change how resource pack grouping works [BC] Change how resource pack grouping works and group mod data packs Sep 14, 2023
@lukebemish
Copy link
Contributor Author

lukebemish commented Sep 14, 2023

Marking this PR as no longer a draft as I think code wise it's in a place where more review is welcome, but note that I've still only tested this in a couple of cases and haven't figured out a good list of weird edge cases to throw at it - and the datapack side of stuff in particular could do with more testing! scratch that, I'll wait to mark it ready for review until I've got a PR to forgespi open to remove those remaining "TODO" blocks - basically, give mods separate ways to mark themselves as wanting a separately visible data/resource pack

@lukebemish
Copy link
Contributor Author

PRs to NeoForgeSPI and FancyModLoader opened for that needed flag for datapack grouping; see neoforged/FancyModLoader#21 and neoforged/NeoForgeSPI#1

@lukebemish lukebemish marked this pull request as ready for review September 14, 2023 17:05
@KnightMiner
Copy link
Contributor

Would it be feasible to make this change without it being a breaking change by just deprecating the old method and keeping the delegating resource pack around but not used? No one should be relying on the fact that mod resources are delegating, rather they should just iterate though the list of all packs and use the forge added method as needed.

@lukebemish
Copy link
Contributor Author

lukebemish commented Sep 14, 2023

The resource grouping part? Possibly, though I would argue it's better to roll it into one big change - a big part of the point of this after all is to change back the assumption of PackResources to the vanilla "one resource per PackResource", which then simplifies a ton of other stuff. The grouping mod datapack part? Definitely not, that requires breaking changes to NeoForgeSPI

@lukebemish
Copy link
Contributor Author

The resource grouping part? Possibly, though I would argue it's better to roll it into one big change - a big part of the point of this after all is to change back the assumption of PackResources to the vanilla "one resource per PackResource", which then simplifies a ton of other stuff. The grouping mod datapack part? Definitely not, that requires breaking changes to NeoForgeSPI

To go off of this - machermans pointed out that the spi changes could be made non breaking, so yes, it is technically possible. However, I would argue that it is still not worth it, given that one of the major goals of this PR is removing the concept of nested pack resources entirely

@lukebemish
Copy link
Contributor Author

lukebemish commented Dec 10, 2023

Superseded by #367

@lukebemish lukebemish closed this Dec 10, 2023
@lukebemish lukebemish deleted the no-more-grouping branch January 10, 2024 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.20 Targeted at Minecraft 1.20 breaking change Breaks binary compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants