Replace ConditionalOps with InterceptingOps that supports conditions on any level of a data structure

[ChiefArug]

This implements a condition system utilising the existing ICondition and IContext interfaces but replacing ConditionalOps and it's downisdes. This new system allows conditions on any level of a json structure, without having to modify the codec parsing the structure (like what the old system required).

Example JSON

Expand

{
  "type": "minecraft:crafting_shaped",
  "category": "misc",
  "key": {"#":{"tag": "minecraft:planks"}},
  "pattern": [
    "##",
    {
      "neoforge:conditional_type": "neoforge:alternative",
      "conditions": [{
        "type": "neoforge:not",
        "value": {
          "type": "neoforge:mod_loaded",
          "modid": "fabric"
        }
      }],
      "value": " #",
      "or_else": "##"
    }
  ],
  "result": {
    "neoforge:conditional_type": "neoforge:alternative",
    "conditions": [{"type": "neoforge:false"}],
    "value": {
      "count": 1,
      "id": "minecraft:crafting_table"
    },
    "or_else": {
      "count": 64,
      "id": "crafting_table"
    }
  },
  "show_notification": false
}

How it works

This PR achieves conditions on any level of a structure via a new DynamicOps<T> called InterceptingOps<T, C> that intercepts all calls to get a value from a T. Once intercepted it attempts to parse what was trying to be got using a Codec<Wrapper<T, C>>. If the codec does not successfully parse then the original result is returned, otherwise the Wrapper then unwraps with some context C. (note that this means that any errors in the format of the conditional operation and condition are silently discarded. see TODO) You would expect that double parsing everything nearly everything passing through codecs would cause performance issues, but from some rough testing with ~100 1.21 mods this does not have a noticeable impact on performance. I do want to do more thorough testing but need help with setting up the tooling to do that (for my roughs I just used log message times).

In this situation the Wrapper is ConditionalOperation and is used to modify T based on whether some conditions specified in the json are true or not. However, InterceptingOps<T, C> is very versatile and could also be used to do wacky things like invert all booleans, or even something more complicated like being able to define areas of datapack json that can be injected into from another datapack registry, allowing opt-in modifying of specific areas of json, like an api for datapacks, even if it is an area where you don't control the main codec (ie loot tables)!

Non-breaking Changes

One of the aims is for this to try not to be a breaking change and therefore the existing format of using neoforge:conditions inline with the conditional data will still work, once I figure out how to do that. Unused classes from the old system are also not removed, just Deprecated with javadoc tags added to most of them to point to the new system.

Tests

This PR also comes with some JUnit tests for the ConditionalOperaions and it's use of InterceptingOps, including a test for the old system mentioned above still working. The existing non JUnit test ConditionalCodecTest has not been modified yet, but I plan on moving it to JUnit tests and removing anything there that uses the old system's java-side stuff, whilst keeping all json formats currently there to prevent this breaking anything. I am interested in anyone who currently uses the java-side parts of the old system to see what they use it for and if this pr needs changes to continue supporting those usecases.

Datagen

When I brought this up in the Discord a major concern was datagen support.

Unfortunately due to the nature of datagen (black box codecs inside black box builders) this is near impossible to achieve well for any condition system more complicated than 'wrap the outer object in something'. Even rewriting datagen wouldn't help much as you still have to deal with the black boxes that are codecs. The best solution I have come up with for this is a system that takes two builders finished outputs and a list of conditions, and works out the difference between the builders to generate a json using ConditionalOperations. However, that would get bulky when trying to use multiple sets of conditions to change different parts, and I personally would have trouble implementing this nicely. Also at that point you can just have a SimpleAlternative that swaps between the two jsons depending on conditions, therefore leaving out all the complicated diffing logic.

Meta

Note this PR has not been opened with the explicit expectation that it will be merged, but rather to open discussion as its easier to do that when you have the code in front of you (especially for a complex to explain system like this). I would like to see this merged as it provides more flexibility at a low maintainability cost, but I also recognise that not many people need the flexibility that this offers, and that technically everything here can already be done using the existing system, this just makes it a ton less verbose.

TODO

• [ ] Implement backwards compat support for top level neoforge:conditions.
• [ ] Implement basic top level datagen support.
• [ ] Finish removing usage of old ConditionalOps.
• [ ] Move old condition tests to JUnit tests
• [ ] Add a Codec to be used as a initial parse test predicate, to allow returning the error if the full codec fails rather than suppressing it

[neoforged-pr-publishing[bot]]

• [ ] Publish PR to GitHub Packages

[Technici4n]

IMO it is better to explicitly define where conditions can be.

[marchermans]

How do you expect this to work with models that require a field to be provided but it is made conditionally.

Is the entire object then not parsed? Or is a null returned, or some default value?

[ChiefArug]

Unlike the old system these conditions don't exist inline, have a look at the example json at the top. You cant just null out an object by having conditions in it, you need to wrap it in another object that specifies a conditional operation. (except for special casing the top level to preserve backwards compat).

How the conditions being false are handled depends on the type of ConditionalOperation used (and modders/neo can register more, just like IConditions) The current three types are as follows

• neoforge:alternative has a specific or_else return value specified in its format.
• neoforge:or_null returns DynamicOps#empty() (so in the case of JsonOps, JsonNull.INSTANCE which just converts to null) if the conditions are false. Note that due to how Mojang implements optional fields, a field being null is the same as it being not present for the purposes of optionals (even if it is not a lenient optional).
• neoforge:remove_fields returns a MapLike with specified fields removed if the condition is true, or the same MapLike unmodified if it is false. If its not a MapLike then it just returns the same thing regardless.

[marchermans]

All three of these seem extremely risky to me when the object you are deserializing is not expecting them.

Null and field removal specifically is extremely risky in my eyes

[ChiefArug]

Its expected that people who know enough to use this know the json format of the object they are making conditional and that erraneously yeeting required stuff will cause issues.

Besides, with what is currently here there is nothing new you can do with only json compared to the old system (at least for recipes where the neoforge:alternatives recipe type existed to pick between alternatives), this is just a much shorter and more understandable (at least wrt what is conditionally changing) format. (this system is also lot more exandable than the old one though, a mod could add a ConditionalOperation to have a value in json conditionally depend on a config value, which wasn't possible before if the mod didn't control the Codec (ie loot tables.))

[marchermans]

Its expected that people who know enough to use this know the json format of the object they are making conditional and that erraneously yeeting required stuff will cause issues.

Besides, with what is currently here there is nothing new you can do with only json compared to the old system (at least for recipes where the neoforge:alternatives recipe type existed to pick between alternatives), this is just a much shorter and more understandable (at least wrt what is conditionally changing) format. (this system is also lot more exandable than the old one though, a mod could add a ConditionalOperation to have a value in json conditionally depend on a config value, which wasn't possible before if the mod didn't control the Codec (ie loot tables.))

I don't think that is how it should work. The point of ConditionalOps is that it works on the "root" level of the object, and that both the datapack maker and the modder is made properly aware of the fact that the object is potentially not available.

With this system, yes the datapack maker can introduce complex structures with alternatives etc, but he can also return null, or other values that the modder might not really expect.

Which I don't think that is a great idea.

[ChiefArug]

but he can also return null, or other values that the modder might not really expect.

They can already do that...? Nothing stops me from making a recipe thats { "type": null }, or { "type": { "foo": "bar"} } except that I know that won't work because that isn't what that field takes. With this system the only difference is that those would be wrapped in a conditional with type or_null or alternative, which seem pretty clear as to what that does. I.E if there is an error about a null value it seems pretty clear that a conditional with type or_null would be the cause, same as just straight up putting null.

both the datapack maker and the modder is made properly aware of the fact that the object is potentially not available.

In my opinion it isn't very clear to the modder that the object potentially isn't available with the current system, but that is not important because of the way its implemented. The modder doesn't need to care because an object who's conditions fail act like it was never there in the first place, in a similar way to if it was filtered out with a block filter in the pack.mcmeta. This system is similar in design, the modder's Codec only recieves each piece of the json when that piece's conditional operations have been resolved. (I think this is known as a transparent system, the consumer of the json doesnt have to interact with this system because it is all done automagically)

[ChiefArug]

After further thinking about this and attempting to implement a codec that supports the old system and the new system I've decided to close this.

Its a nice system, but its usecases are few and far between, and those can all already be served by the existing system, albeit not as well.

If someone does want to continue this or a similar system feel free to reach out to me.