Fix inverted logic in TagEmptyCondition

[daedalus4096]

trafficstars

The tag-empty condition was previously evaluating as true for populated tags instead of empty ones, causing incorrect loading of dependent recipes. This change implements a fix and adds a pair of tests to prevent regressions.

[CLAassistant]

All committers have signed the CLA.

[LexManos]

Actually the correct fix is to remove the whole regops block of code. Tags are not populated into the passed in registry until after the entire process is loaded. Which is annoying. Which is why the whole context object needs to be passed around.

Also running through your tests, the tag_empty_condition_with_empty_tag doesn't fail before this change which makes me confused. But I figured it out. During the reload process, the registry access is toggled to create any tag it's asked for.

        this.registryLookup.missingTagAccessPolicy(ReloadableServerResources.MissingTagAccessPolicy.CREATE_NEW);
        this.tagManager = new TagManager(p_206857_);```
        this.commands = new Commands(p_206858_, CommandBuildContext.simple(this.registryLookup, p_250695_));
        // Forge: Create context object and pass it to the recipe manager.
        this.context = new net.minecraftforge.common.crafting.conditions.ConditionContext(this.tagManager);
        this.recipes = new RecipeManager(this.registryLookup, this.context);
        this.advancements = new ServerAdvancementManager(this.registryLookup, this.context);
        this.functionLibrary = new ServerFunctionLibrary(p_206859_, this.commands.getDispatcher());

So the var optional = items.get().get(this.tag); will always return a filled value. Thus the old code is wrong. The tags are only populated into the registry at the very end after everything is loaded.

So ya, the only valid way to check is to use the Context. So deleting the whole

        // Check the actual deserialization context if possible
        if (ops instanceof RegistryOps<?> reg) {
            Optional<HolderGetter<Item>> items = reg.getter(Registries.ITEM);
            if (items.isPresent()) {
                var optional = items.get().get(this.tag);
                return optional.isPresent();
            }
        }

Is the way to go, this code was copied from the ItemExistsCondition, which since Items are not data driven functions correctly.

Besides that, looks fine, would be nice to have more robust tests Specifically need to test that the TagEmpty checks the CURRENTLY loading tags. Perhaps could use one of the obscure Forge tags, and have the test override it to empty? Perhaps tag(Tags.Items.EGGS)

[daedalus4096]

I corrected my original fix as you suggested. Tests still pass.

While I attempted to make the tests more robust as you requested, I was unsuccessful in doing so. Specifically, I was unable to empty out an existing tag, eggs in this case. Despite adding an item tag provider to the test file and updating the tag, attempts to remove or replace the original tag entry were unsuccessful. I could add items to that tag, which I was able to confirm via debug logging, but I couldn't remove anything. My suspicion, which I can't prove, is that the test's egg tag file was being applied before Forge's egg tag file, so that no matter what I did in the test provider, Forge always added back in the original entry when parsing the tag definitions.

If you want me to investigate further, I can, but I'm not sure how to overcome that hurdle.

[LexManos]

Fixing the laod order should be simple as editing the generated toml file for the test mods to have AFTER forge.

[daedalus4096]

Ah, today I learned. Thanks for the pointer. Unfortunately, following it seems to have disproven my theory about mod loading order being a culprit. Forcing the test mods to load after Forge yielded no change in behavior. On a lark, I tried having them load before Forge, and that just caused the mod loader to crash, citing a cycle in mod loading order. So it seems the test mods are being loaded after Forge by default after all.

I tried a few other tags, both Forge and Vanilla, on the off chance there was something specific with eggs that was interfering, but no dice. Adding an item to a previously undefined tag and then removing it in the same tag definition file seems to work as expected: you end up with an empty tag. But attempting within Forge tests to replace or remove items from tags defined by Forge or Vanilla simply seems busted, and I don't know why. I'm going to keep digging as to why the tag loader is misbehaving this way, but this is uncharted territory for me.

[LexManos]

Push up what you have and I can take a look tomorrow. It should be fine as the test mods are specifically designed to behave exactly like if they were packaged in their own individual normal jar.

[daedalus4096]

Here's my current code, after cleaning up all the extraneous log messages. And I think I've found the culprit, but I'm not sure how to fix it. The TagLoader#load method is processing and applying the TagFile objects in alphabetical order of their source mod ID, rather than the order in which the mods were loaded. The means that even though "conditional_recipe" is set to be loaded after "forge", it's still having its tags applied first. Prepending a "z" to the test's MODID and regenerating the data will cause the currently failing test to pass.

[LexManos]

Not sure what you're running into but adding a mods.toml for this specific test works fine:

--- /dev/null
+++ b/src/test/resources/conditional_recipe/META-INF/mods.toml
@@ -0,0 +1,6 @@
+[[dependencies.conditional_recipe]]
+    modId="forge"
+    mandatory=true
+    versionRange="[0,]"
+    ordering="AFTER"
+    side="BOTH"

Be sure that you create a new world when you do this. As the datapack sorting is stored in the world. And will override this ordering. Or you can use the /datapack command to change the order and /reload to reload.

[daedalus4096]

Interesting. I was creating the dependencies section in code as part of ForgeDevLocator, but I dropped that modification from the change once I found the alphabetical sorting thing. I'm surprised to hear that it works properly when run the way you describe, but then again you're the expert here.

[LexManos]

Ya, I forgot that I added support for per-test mods.toml. Which is why I pointed you at the locator. Sorry. The world storing the order threw me off for a bit. So be sure you're not running into that. And let me know how it goes on your end. Definitely don't like tests that only work for one person.

The reason you're running into alphabetical sorted list is that, save for any other order being declared, mod files are loaded from directory in alphabetical order to make the load order consistent across operating systems.

[daedalus4096]

Well this is interesting/annoying. Apparently whether the test succeeds or fails depends at least partially upon how you execute it. I've been doing most of my testing by running the forge_server_gametest_test run configuration. When I do that, with the code currently uploaded, then tag_empty_condition_with_empty_tag, and only that test, fails. But if I instead use the forge_client_test run configuration and execute that test using the /test command, then it succeeds. How have you been running it?

This is after I blew away the world directory in all of the forge project's run folders, by the way.

[LexManos]

Interesting indeed, forge_server_gametest_test fails for me as well. Good to know its reproduceable. I'll poke around in the morning {its after 11pm here} let me know if you figure anything out.

[daedalus4096]

I'm going to be largely unavailable for the next few days, but I expect to be back on the case by Monday.

[LexManos]

So, found the issue. (I should be sleeping but brain is doing what it feels like) net.minecraft.gametest.framework.GameTestServer:82 It uses p_206609_.getAvailableIds() when it should be p_206609_.getSelectedIds() Specifically the function net.minecraft.server.packs.repository.PackRepository.discoverAvailable() uses a TreeMap which sorts the packs alphabetically by id. Changing the getAvalibleIds to getSelectedIds should work without issue. I just havent dug deep enough to say that for completely certian.

[autoforge[bot]]

@daedalus4096, this pull request has conflicts, please resolve them for this PR to move forward.

[daedalus4096]

Okay, so I'm back from vacation and looking at this issue again. I took a look at what you posted last, and unfortunately the fix you suggested didn't work, at least not in forge_server_gametest_test. The getSelectedIds method also returns the pack information in alphabetical order. Specifically, the toString'd contents of its return value are:

[bundle, mod:biome_test_mod, mod:conditional_recipe, mod:creative_mode_tab_test, mod:criterion_test, mod:datapack_builtin_entries_provider_test, mod:forge, mod:gather_components_test_event, mod:global_loot_test, mod:gui_layer_test, mod:mdk_datagen, mod:network_data_registry, mod:test_helper_mod, trade_rebalance, vanilla]

If you have any more suggestions, I'd love to hear them. Otherwise, given that it's vanilla code that seems to be misbehaving and I don't know the first thing about creating patches, I'm at the point where I just want to change ConditionalRecipeTest.MODID to be test_conditional_recipe and call it a day. Modifying vanilla pack contents in game tests seems to be a pretty uncommon edge case, since I seem to have been the first to stumble across this issue.

[LexManos]

Well that's annoying, but that's why I like having a second person test.

Did you make sure to try on a new world?

As for making patches, it's easy. You edit the code in the 'Forge' project. And then run gradlew forge:genPatches

[LexManos]

Poked around a bit more with some ideas. I could change getAvalible to use a Linked Map instead of a Tree Map so it maintains order but that could cause issues in other sections of code that expect it alphabetical {if they exist} I could add a secondary function for GameTestServer itself that gets it in the sorted listed. But that would break the ability to manually test ordering in the world.

So what I have decided to propose is adding the ability for game test server to ignore old worlds. Specifically the changes below (test them out and see if they work for you) that adds a new --uniqueWorld parameter for the gametest server runs. Which sets the world name to the current time. Now the issue is because this creates a new world every run, it could cause your world folder to grow quite large. So you may have to prune it every now and again. But its ~7mb/test in my runs. Tho a benefit of it not deleting worlds is that if you DO have failed tests you could go back and look at the exact world in single player.

A mitigation for the potentially large world folder {after many runs} would just be to have it as another run config. Or add a run config that just 'cleans' the workspace.

diff --git a/build_forge.gradle b/build_forge.gradle
index 00e2eace0..4971e711a 100644
--- a/build_forge.gradle
+++ b/build_forge.gradle
@@ -194,6 +194,7 @@ patcher {

         forge_server_gametest {
             args '--launchTarget', 'forge_dev_server_gametest'
+            args '--uniqueWorld' // Unique world is used so that the world regenerates, as well as the world config isn't influenced by other runs
         }

         forge_server_gametest_test {
diff --git a/patches/minecraft/net/minecraft/server/Main.java.patch b/patches/minecraft/net/minecraft/server/Main.java.patch
index 9b953ae54..be15eb654 100644
--- a/patches/minecraft/net/minecraft/server/Main.java.patch
+++ b/patches/minecraft/net/minecraft/server/Main.java.patch
@@ -1,15 +1,17 @@
 --- a/net/minecraft/server/Main.java
 +++ b/net/minecraft/server/Main.java
-@@ -83,6 +_,15 @@
+@@ -83,6 +_,17 @@
          OptionSpec<Void> optionspec13 = optionparser.accepts("jfrProfile");
          OptionSpec<Path> optionspec14 = optionparser.accepts("pidFile").withRequiredArg().withValuesConvertedBy(new PathConverter());
          OptionSpec<String> optionspec15 = optionparser.nonOptions();
 +        optionparser.accepts("allowUpdates").withRequiredArg().ofType(Boolean.class).defaultsTo(Boolean.TRUE); // Forge: allow mod updates to proceed
 +        optionparser.accepts("gameDir").withRequiredArg().ofType(File.class).defaultsTo(new File(".")); //Forge: Consume this argument, we use it in the launcher, and the client side.
 +        final OptionSpec<net.minecraft.core.BlockPos> spawnPosOpt;
++        OptionSpec<Void> uniqueWorld = null;
 +        boolean gametestEnabled = Boolean.getBoolean("forge.gameTestServer");
 +        if (gametestEnabled) {
 +            spawnPosOpt = optionparser.accepts("spawnPos").withRequiredArg().withValuesConvertedBy(new net.minecraftforge.gametest.BlockPosValueConverter()).defaultsTo(new net.minecraft.core.BlockPos(0, 60, 0));
++            uniqueWorld = optionparser.accepts("uniqueWorld");
 +        } else {
 +            spawnPosOpt = null;
 +        }
@@ -31,7 +33,7 @@
              Path path = optionset.valueOf(optionspec14);
              if (path != null) {
                  writePidFile(path);
-@@ -105,24 +_,26 @@
+@@ -105,24 +_,29 @@
              Bootstrap.validate();
              Util.startTimerHackThread();
              Path path1 = Paths.get("server.properties");
@@ -58,6 +60,9 @@
              File file1 = new File(optionset.valueOf(optionspec9));
              Services services = Services.create(new YggdrasilAuthenticationService(Proxy.NO_PROXY), file1);
              String s = Optional.ofNullable(optionset.valueOf(optionspec10)).orElse(dedicatedserversettings.getProperties().levelName);
++            // Forge: make each gametest use a timestamped world name
++            if (uniqueWorld != null && optionset.has(uniqueWorld))
++                s = "gametest_world\\" + java.time.format.DateTimeFormatter.ofPattern("yyyy-MM-dd_HH.mm.ss.SSS").format(java.time.LocalDateTime.now());
 +            if (s == null || s.isEmpty() || new File(file1, s).getAbsolutePath().equals(new File(s).getAbsolutePath())) {
 +                LOGGER.error("Invalid world directory specified, must not be null, empty or the same directory as your universe! " + s);
 +                return;

Z:\Projects\Forge_1211>git diff --cached
diff --git a/patches/minecraft/net/minecraft/gametest/framework/GameTestServer.java.patch b/patches/minecraft/net/minecraft/gametest/framework/GameTestServer.java.patch
new file mode 100644
index 000000000..30ec2bcce
--- /dev/null
+++ b/patches/minecraft/net/minecraft/gametest/framework/GameTestServer.java.patch
@@ -0,0 +1,11 @@
+--- a/net/minecraft/gametest/framework/GameTestServer.java
++++ b/net/minecraft/gametest/framework/GameTestServer.java
+@@ -79,7 +_,7 @@
+         } else {
+             p_206609_.reload();
+             WorldDataConfiguration worlddataconfiguration = new WorldDataConfiguration(
+-                new DataPackConfig(new ArrayList<>(p_206609_.getAvailableIds()), List.of()), FeatureFlags.REGISTRY.allFlags()
++                new DataPackConfig(new ArrayList<>(p_206609_.getSelectedIds()), List.of()), FeatureFlags.REGISTRY.allFlags()
+             );
+             LevelSettings levelsettings = new LevelSettings("Test Level", GameType.CREATIVE, false, Difficulty.NORMAL, true, TEST_GAME_RULES, worlddataconfiguration);
+             WorldLoader.PackConfig worldloader$packconfig = new WorldLoader.PackConfig(p_206609_, worlddataconfiguration, false, true);

[daedalus4096]

That seemed to work, thank you! The previously failing tag-empty test now passes reliably in the forge_server_gametest_test run configuration, even when the conditional recipe test mod ID comes before Forge alphabetically. I'll get the code you suggested pushed up to the PR momentarily. Thank you for your extensive help in resolving this issue.