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] Resource Conditions in Pack Overlays #3872

Merged
merged 7 commits into from
Aug 4, 2024

Conversation

Apollounknowndev
Copy link
Contributor

Supercedes #3702, rebased to target 1.21.

1.20.2 added pack overlays (explained in the 23w31a changelog), which conditionally overlay resource/data pack files from sub-directories onto the base pack. This PR extends this system to allow for overlays based on resource conditions, which allow overlays to be applied based on conditions other than just the current pack format.

@modmuss50
Copy link
Member

It looks like the conditionalrecipes game tests are failing. 🤔

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

Fixed the failing gametest by moving the conditional overlays test into a proper gametest.

Copy link
Member

@modmuss50 modmuss50 left a comment

Choose a reason for hiding this comment

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

Looks good, just a few minor changes needed.

@modmuss50 modmuss50 added the last call If you care, make yourself heard right away! label Jul 27, 2024
* limitations under the License.
*/

package net.fabricmc.fabric.impl.resource.conditions;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why isn't this a public API class? Without this class pack.mcmeta datagen is impossible

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, I see this was asked to be changed a month ago — in my defence, the PR kept getting closed and it was a bit hard to keep track of.

In any case I still think this class should be an API anyway

Copy link
Member

Choose a reason for hiding this comment

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

We should definally be able to datagenerate this, however I worry just moving this to API isnt the ideal solution.

  • Generally we dont make records/data classes part of the API, we would have an interface + static factory function, it provides a better API surface.
  • How easy is it to datagenerate the pack.mcmeta file right now? I dont think we do anything special for it, this does sound like a useful thing to have. Maybe its best leaving this as-is for now and making a nice API for generating the file?

Copy link
Contributor

@maityyy maityyy Jul 28, 2024

Choose a reason for hiding this comment

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

Generally we dont make records/data classes part of the API

However, this class is very similar to (even copies) the vanilla PackOverlaysMetadata, which makes it easy to understand how to use it

How easy is it to datagenerate the pack.mcmeta file right now?

The only thing is that ResourceFilter requires AW for BlockEntry::<init>, but that's the only such case. I datagen pack.mcmeta to filter recipes & advancements
(yes, another TAW candidate)

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, actually, the static factory function sounds ok, the main thing is to give a name similar to the vanilla class (yarn)

Copy link
Member

Choose a reason for hiding this comment

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

Going to merge this PR as-is, we can have a follow up PR making this better for datagen.

@modmuss50 modmuss50 added the merge me please Pull requests that are ready to merge label Aug 4, 2024
…/overlay-conditions-1.21

# Conflicts:
#	fabric-resource-loader-v0/src/main/java/net/fabricmc/fabric/impl/resource/loader/ResourceManagerHelperImpl.java
@modmuss50 modmuss50 merged commit 8dc279b into FabricMC:1.21 Aug 4, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request last call If you care, make yourself heard right away! merge me please Pull requests that are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants