-
Notifications
You must be signed in to change notification settings - Fork 399
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
[1.21] Resource Conditions in Pack Overlays #3872
Conversation
It looks like the conditionalrecipes game tests are failing. 🤔 |
Fixed the failing gametest by moving the conditional overlays test into a proper gametest. |
There was a problem hiding this 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.
.../src/main/java/net/fabricmc/fabric/api/resource/conditions/v1/OverlayConditionsMetadata.java
Outdated
Show resolved
Hide resolved
.../src/main/java/net/fabricmc/fabric/api/resource/conditions/v1/OverlayConditionsMetadata.java
Outdated
Show resolved
Hide resolved
.../src/main/java/net/fabricmc/fabric/api/resource/conditions/v1/OverlayConditionsMetadata.java
Outdated
Show resolved
Hide resolved
...v1/src/main/java/net/fabricmc/fabric/mixin/resource/conditions/ResourcePackProfileMixin.java
Outdated
Show resolved
Hide resolved
* limitations under the License. | ||
*/ | ||
|
||
package net.fabricmc.fabric.impl.resource.conditions; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
…/overlay-conditions-1.21 # Conflicts: # fabric-resource-loader-v0/src/main/java/net/fabricmc/fabric/impl/resource/loader/ResourceManagerHelperImpl.java
Supercedes #3702, rebased to target 1.21.