Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exporting array elements to JSON misses out indexes and loses data #158

Closed
coldino opened this issue Aug 22, 2024 · 9 comments
Closed

Exporting array elements to JSON misses out indexes and loses data #158

coldino opened this issue Aug 22, 2024 · 9 comments

Comments

@coldino
Copy link

coldino commented Aug 22, 2024

It seems to be currently impossible to correctly/fully export assets that override individually indexed array elements.

An example:
image
All of these similar fields (with different indexes) would overwrite each other so only a single field remains in the JSON:

"MaxStatusValues": 550,

Fixing this is a little problematic because most "good" solutions break with the existing format.

  • Can't use an array because you can override just individual elements
  • Switching to a map format would break anything expecting the old format

The compromise solution I'm testing in a fork (0367620) is to continue to export anything with index 0 as-is, but add the index for all others - for example:

"MaxStatusValues": 650,
"MaxStatusValues[1]": 500,
"MaxStatusValues[2]": 500,
"MaxStatusValues[4]": 6000,
"MaxStatusValues[7]": 550,

Thoughts?

@4sval
Copy link
Collaborator

4sval commented Aug 23, 2024

Im sorry I don't understand how similar fields would overwrite each other if Properties is a List
writing the json with similar fields name works fine, are you talking about parsing the json back to an object?

@4sval
Copy link
Collaborator

4sval commented Aug 23, 2024

indexed-props.patch

what about this?
PropertyTagFlags might not be set for your case and that might be the biggest problem here
but the handling of these props should now properly reflect their type

@coldino
Copy link
Author

coldino commented Aug 24, 2024

I'll have to try out the patch to figure out what the effect is.

But let me try to explain the issue a little better:

  • There's no index in the export for array entry overrides so it's impossible to use the value
  • Multiple array entry overrides for different indexes all have exactly the same name (as they don't include their index) so the duplicates are lost

The two JSON examples in the original post show what it currently looks like (missing a lot of data) and what my prototype looks like.

@4sval
Copy link
Collaborator

4sval commented Aug 24, 2024

the underlying json writer is well capable of writing the same property multiple times, I don't really get the issue
image

if you're talking about parsing the json back, that's what the patch is trying to fix, by grouping HasArrayIndex flagged properties by name, and writing an array for each group, instead of writing individual objects for all flagged properties.
so you'd get

{
    "MaxStatusValues": [
        650,
        500,
        500,
        6000,
        550
    ]
}

but it will only work if it's a UE5 game or if you're using a mapping file for it

@coldino
Copy link
Author

coldino commented Aug 24, 2024

Right. JSON readers typically don't like this, but I understand it's quite a grey area in the specs. The bigger problem is that these overrides are usually sparse, so we still need to know the indexes for each entry. If you look at my example it overrides the elements at indexes 0, 1, 2, 4, 7 but leaves the others to be inherited from the parent. Your current array solution doesn't contain that information.

Of course the array could have nulls or something, but it would be neater as a map with the key being the index. In my original post I mentioned this, but was worried about breaking existing readers. If this isn't an issue, a map would be the perfect result.

Thanks for taking time to look at this!

@hallipr
Copy link
Contributor

hallipr commented Aug 26, 2024

I believe coldino is saying that a more accurate representation of the property would be:

{
    "MaxStatusValues": [
        650,
        500,
        500,
        null,
        6000,
        null,
        null,
        550
    ]
}

and a more consise, but possibly breaking form would be:

{
    "MaxStatusValues": {
        "0": 650,
        "1": 500,
        "2": 500,
        "4": 6000,
        "7": 550
    }
}

another verbose but backward compatible solution could add a non-standard json property to hold the array indices

{
  "Properties": {
    "MaxStatusValues": [
      650,
      500,
      500,
      6000,
      550
    ]
  },
  "__PropertyArrayIndex": {
    "MaxStatusValues": [
      0,
      1,
      2,
      4,
      7
    ]
  }
}

@coldino
Copy link
Author

coldino commented Aug 26, 2024

Well, the core issue is that the indexes are not exported, yes, but actually both forms are format-breaking. As that is the case, the map is by far the preferred.

@4sval
Copy link
Collaborator

4sval commented Aug 26, 2024

the main problem for implementing a format breaking solution is knowing the difference between an indexed prop and a non-indexed prop on UE4 games
only ArrayIndex > 0 could be used, but that is the least reliable solution because it being 0 doesn't mean the prop is not part of an array

so the viable option as of now would be the forked one ig?

@coldino
Copy link
Author

coldino commented Aug 26, 2024

Yup it's a dilemma for sure - that's why I leave it up to you really as you know much more about the project and wider ecosystem than me.

If you want to go with the fork option I can raise a PR but as it's only one line feel free to just do it if takes less of your time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants