Simplify FlagCondition #1713
Simplify FlagCondition #1713
Conversation
Last commit published: fc5a7a94a4bc06f7e9af4104a62d4fc81fad5a1c. PR PublishingThe artifacts published by this PR:
Repository DeclarationIn order to use the artifacts published by the PR, add the following repository to your buildscript: repositories {
maven {
name 'Maven for PR #1713' // https://github.com/neoforged/NeoForge/pull/1713
url 'https://prmaven.neoforged.net/NeoForge/pr1713'
content {
includeModule('net.neoforged', 'neoforge')
includeModule('net.neoforged', 'testframework')
}
}
}MDK installationIn order to setup a MDK using the latest PR version, run the following commands in a terminal. mkdir NeoForge-pr1713
cd NeoForge-pr1713
curl -L https://prmaven.neoforged.net/NeoForge/pr1713/net/neoforged/neoforge/21.3.52-beta-pr-1713-cleanup-1712/mdk-pr1713.zip -o mdk.zip
jar xf mdk.zip
rm mdk.zip || del mdk.zipTo test a production environment, you can download the installer from here. |
|
@Technici4n, this PR introduces breaking changes. Compatibility checks
|
src/main/java/net/neoforged/neoforge/common/conditions/IConditionBuilder.java
Outdated
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/common/conditions/IConditionBuilder.java
Outdated
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/common/conditions/FlagCondition.java
Outdated
Show resolved
Hide resolved
tests/src/main/java/net/neoforged/neoforge/debug/data/CustomFeatureFlagsTests.java
Outdated
Show resolved
Hide resolved
ApexModder
added
cleanup
| public static ICondition isEnabled(FeatureFlag requiredFlag) { | ||
| return isEnabled(FeatureFlagSet.of(requiredFlag)); | ||
| } | ||
|
|
||
| public static ICondition isEnabled(FeatureFlag requiredFlag, FeatureFlag... requiredFlags) { | ||
| return isEnabled(FeatureFlagSet.of(requiredFlag, requiredFlags)); | ||
| } | ||
|
|
||
| public static ICondition isDisabled(FeatureFlagSet requiredFeatures) { | ||
| return new FlagCondition(requiredFeatures, false); | ||
| } | ||
|
|
||
| public static ICondition isDisabled(FeatureFlag requiredFlag) { | ||
| return isDisabled(FeatureFlagSet.of(requiredFlag)); | ||
| } | ||
|
|
||
| public static ICondition isDisabled(FeatureFlag requiredFlag, FeatureFlag... requiredFlags) { | ||
| return isDisabled(FeatureFlagSet.of(requiredFlag, requiredFlags)); |
There was a problem hiding this comment.
Please retain these methods, I find them much better to read and understand then new NotCondition(FlagCondition.of(...)) and given that they return ICondition, the actual implementation does not matter
There was a problem hiding this comment.
Creating the conditions manually is not the recommended way of creating them, using the builder is. The builder is simply not(...)
There was a problem hiding this comment.
even going that route not(featureFlags()) doesnt really explain what it does by simply reading the methods names, where as not(featureFlagsEnabled()) or not(isFeatureEnabled()) does.
and having nice simple utilities like isFeatureDisabled while sure can be fully implemented with not is still nice to have and makes overall code easier to read and understand.
There was a problem hiding this comment.
I do agree with renaming the methods. However we don't have modIsntLoaded or itemDoesntExist or tagIsntEmpty or andNot or orNot utilities. Conditions are boolean composition and we would be bloating the api surface by introducing combinations instead of having people compose the condition exactly as it would be output in the json.
| default ICondition isFeatureEnabled(FeatureFlag requiredFlag) { | ||
| return FlagCondition.isEnabled(requiredFlag); | ||
| } | ||
|
|
||
| default ICondition isFeatureEnabled(FeatureFlag requiredFlag, FeatureFlag... requiredFlags) { | ||
| return FlagCondition.isEnabled(requiredFlag, requiredFlags); | ||
| } | ||
|
|
||
| default ICondition isFeatureDisabled(FeatureFlagSet requiredFeatures) { | ||
| return FlagCondition.isDisabled(requiredFeatures); | ||
| } | ||
|
|
||
| default ICondition isFeatureDisabled(FeatureFlag requiredFlag) { | ||
| return FlagCondition.isDisabled(requiredFlag); | ||
| } | ||
|
|
||
| default ICondition isFeatureDisabled(FeatureFlag requiredFlag, FeatureFlag... requiredFlags) { | ||
| return FlagCondition.isDisabled(requiredFlag, requiredFlags); |
| @Override | ||
| protected void buildRecipes() { | ||
| // recipe available when above flag is enabled | ||
| shapeless(RecipeCategory.MISC, Items.DIAMOND) | ||
| .requires(ItemTags.DIRT) | ||
| .unlockedBy("has_dirt", has(ItemTags.DIRT)) | ||
| .save(output.withConditions(FlagCondition.isEnabled(flag)), enabledRecipeName); | ||
| .save(output.withConditions(featureFlagsEnabled(flag)), enabledRecipeName); | ||
|
|
||
| // recipe available when above flag is disabled | ||
| shapeless(RecipeCategory.MISC, Items.DIRT) |
There was a problem hiding this comment.
I would just remove this now.
There was a problem hiding this comment.
just to clarify i would remove the disabled flag test yeah, test linked is the enabled flag test
cant see why would want to remove both tests, good to keep a test to ensure it continues working
Simplification of #1712, coming after the fact since it was merged so quickly. 😉