Minecraftforge: Ambiguous variant/property references in the Forge block state deserializer/format

[sfPlayer1]

https://github.com/MinecraftForge/MinecraftForge/blob/752be6b526e162665ac16e20c161b38de4e745ea/src/main/java/net/minecraftforge/client/model/ForgeBlockStateV1.java#L103 doesn't always identify the correct variant structure. Forge supports both full variant ("name=value,name2=value2,...": {..}) and single-property ("name": { "value": {..} }, "name2": { "value2": {..} }, ...) declarations and tries to distinguish them by their shape, which is currently quite unreliable. See https://gist.github.com/RainWarrior/0618131f51b8d37b80a6 for a format reference.

A full variant may also use a JSON object at the tested location, e.g. for the textures, transform, submodel and custom properties:
{ "variants" : { "asd=xy": { "textures": { ... } } } }

If such a JSON object property happens to be first, the parser will misidentify the full variant reference as a single property reference.

The following checks can reduce the ambiguity:

• full variant reference if the key contains '=' (property names only allow a-z0-9_) - variants like "normal" are still ambiguous
• full variant reference if the value is an array (e.g. "normal": [{ "textures": { ... } }]) - already implemented
• full variant reference if any of the values values doesn't contain a JSON object, not just the first as currently implemented
• single property reference if the values contain an invalid key (not model/textures/x/y/...)
• otherwise assume single property reference - status quo, wrapping the value in an array is an existing workaround, breaks seamless vanilla compatibility

Conflicting identification results are an error.

Changing the format to use a root key separate from "variants" for single property references and making "variants" use vanilla semantics is AFAIK the only complete solution to the ambiguity problem while keeping vanilla compatibility.

The problem may seem less important, but it's one of the contributors to the JSON based model system being rather opaque, unreliable and hard to fully grasp ("why wrap this block in []"). Static validation of the files/better error messages may be easier with something more precise.

[stale[bot]]

This issue has been automatically marked as stale because it has not had activity in a long time. If this issue is still relevant and should remain open, please reply with a short explanation (e.g. "I have checked the code and this issue is still relevant because ___." or "Here's a screenshot of this issue on the latest version"). Thank you for your contributions!

[stale[bot]]

This issue has been automatically closed because it has not had activity in a long time. Please feel free to reopen it or create a new issue.

[catb0t]

I don't think this should be forgotten about but it's unfortunate that nobody is discussing it

[sfPlayer1]

1.13 may be a reasonable opportunity to change the key from "variants" to "properties" for forge-style combinatorial property matches.