Yellow-Dog-Man / Resonite-Issues

Issue repository for Resonite.
https://resonite.com
138 stars 2 forks source link

AnimJ track fields are order dependent #2447

Open mpmxyz opened 3 months ago

mpmxyz commented 3 months ago

Describe the bug?

ECMA-404 does not specify if the order of properties within JSON objects is significant. RFC 4627 on the other hand calls objects an "unordered collection of zero or more name/value pairs". The general expectation among many developers and libraries is that it does not matter how you order the fields of objects. Unfortunately loading AnimJ fails if you don't specify the track properties exactly in the following order:

  1. trackType
  2. valueType
  3. data

To Reproduce

Try to import one of these files after you remove the .json extension: failed.animj.json failed2.animj.json

Expected behavior

The previous files should import like this: passed.animj.json

Screenshots

No response

Resonite Version Number

2024.6.25.1149

What Platforms does this occur on?

Windows

What headset if any do you use?

No response

Log Files

Anonymous - 2024.6.25.1149 - 2024-07-01 23_16_27.log I tried loading "failed.animj" between two "DEBUG_TEST" lines.

Additional Context

No response

Reporters

No response

Frooxius commented 3 months ago

While the standard might specify that the order is not important, the specific applications might depend on it.

In this case, we are using polymorphism - the track type determines how is the rest of the JSON de-serialized, which creates an order dependency.

This means that addressing it would add a fair bit of complications to the code, as we'd have to skip over a chunk of JSON to determine the type of data and then backtrack, which I don't think there's a "nice" way of doing that with System.Text.Json

Does this become a problem in practice? E.g. do you use solutions which keep re-ordering the fields and don't let you control how they're setup?

If it's not practical problem, we might not invest time into this and instead just specify that for this particular application, the order is important.

stiefeljackal commented 3 months ago

Does this become a problem in practice? E.g. do you use solutions which keep re-ordering the fields and don't let you control how they're setup?

Truthfully, for the 12 years that I have been in the IT industry, the AnimJ deserialization process is the first and only one that I encountered where a specific JSON property order is required. In past projects that I have worked on, the system did not care about the property ordering; the only thing it cared was the property name and type mapping. One current example that most use today is the Headless config file. The property ordering in the JSON file does not matter when being deserialized.

I understand that the deserialization process for the track is currently procedural via a method (I know because I have worked on a mod for AnimJ in the past). As much as I am for keeping the code elegant, I do believe that specifications should be followed especially if they are followed in the industry. There might be some attributes present in System.Text.Json that might help deserialize a given child object based on one or more properties within the parent object. Unfortunately, this is just me speculating since I haven't used System.Text.Json that much and mostly used the Newtonsoft implementation in past C# projects.

Frooxius commented 3 months ago

Ideally we would have it be order independent, yes, but we do have limited amount of engineering time and we need to prioritize this accordingly against other issues.

If it's causing practical issues, then it can get higher priority. If it's not, then we're likely to prioritize lower, because working on this wouldn't bring much benefit to the community at the time.

From cursory search, polymorphism in System.Text.Json had (has?) the same issue: https://github.com/dotnet/runtime/issues/72604, so it's not a problem unique to this. Headless config doesn't use polymorphism, so it doesn't exhibit the same issue.

stiefeljackal commented 3 months ago

Ideally we would have it be order independent, yes, but we do have limited amount of engineering time and we need to prioritize this accordingly against other issues.

Of course. I was not suggesting this needs to be fixed ASAP since it can be done at a later time. I do not believe too many people are using AnimJ at this time.

If it's causing practical issues, then it can get higher priority. If it's not, then we're likely to prioritize lower, because working on this wouldn't bring much benefit to the community at the time.

If anything, disregard the order dependency for trackType and valueType and keep data as the last property. I believe doing that will help alleviate most of the problem since trackType and valueType are probably the two that are commonly flipped around; they do not depend on each other. However....

From cursory search, polymorphism in System.Text.Json had (has?) the same issue: https://github.com/dotnet/runtime/issues/72604, so it's not a problem unique to this.

It seems like this issue will be resolved in the .NET 9 version, so it might be worth it to wait.

Headless config doesn't use polymorphism, so it doesn't exhibit the same issue.

I understand that. I was just naming an example of something that did not require a strict property ordering. For something specific to polymorphism, I believe I have done something in the past with Newtonsoft, but I cannot remember.

mpmxyz commented 3 months ago

While the standard might specify that the order is not important, the specific applications might depend on it.

In this case, we are using polymorphism - the track type determines how is the rest of the JSON de-serialized, which creates an order dependency.

This means that addressing it would add a fair bit of complications to the code, as we'd have to skip over a chunk of JSON to determine the type of data and then backtrack, which I don't think there's a "nice" way of doing that with System.Text.Json

Does this become a problem in practice? E.g. do you use solutions which keep re-ordering the fields and don't let you control how they're setup?

If it's not practical problem, we might not invest time into this and instead just specify that for this particular application, the order is important.

It is a thing I discovered when researching on how to generate AnimJ files but since I'm manually composing the string in ProtoFlux anyway I don't have an urgent need to have it changed. I've documented the behavior on the Wiki for now so people doing similar things are made aware.

PS: Interpretation really depends on the "standard" one is following - the linked RFC makes objects unordered.

Banane9 commented 3 months ago

On Json.NET, I'd probably approach this using a JsonConstructor (Attribute) to feed the raw parsed data into a constructor to handle it. Could also use the JsonExtensionData Attribute to only evaluate the data after the other properties are set.

I'm not sure what the System.Text.Json parser offers in that regard.