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

Treat JSON as resource files. #65295

Merged
merged 1 commit into from
Sep 16, 2022
Merged

Conversation

reduz
Copy link
Member

@reduz reduz commented Sep 3, 2022

This makes the files ended in ".json" be treated as Godot resources.
This solves two problems:

  • Avoid extensions to implement their own handling, which results in conflicts (all must use this one).
  • Allow code to still work opening it as a file (since it will not be imported).

Some questions:

  • Should "data" be a serializable property?
  • Does this still work for editing JSON in the code editor?

Implements godotengine/godot-proposals#3608

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my example, I like the idea that jsons are serialized as data in a resource file.

https://github.com/godot-extended-libraries/json

@derammo
Copy link
Contributor

derammo commented Sep 3, 2022

Just my opinion based on many projects using JSON as a serialization format for various types of data:

I don't think that Godot should claim to own ".json" or that this extension should even indicate a file type. In my experience, I always end up defining application formats like ".myappdata.json" or ".myotherstuff.json" which are different sorts of files that are parsed into different classes. The fact that they are both encoded in JSON is indicated in the second extension so that external editors will immediately know how to show/edit these files without the user having to set the language mode to JSON.

I even store schema files for the JSON that I use as ".schema.json" just so the editors will do the right thing. And these clearly aren't even data.

I think a Godot resource encoded as JSON should own ".resource.json" or whatever else you want, but not the entire ".json" extension, because any non-trivial application will also have its own JSON file formats if it uses JSON at all.

@reduz
Copy link
Member Author

reduz commented Sep 3, 2022

@derammo I understand this, but due to how Godot works, its not possible to do this because it works entirely by extension, which makes the code very simple and easy to maintain. Supporting sub extensions would be an enormous amount of work t and be extremely hacky just for working around this specific corner case (its not needed anywhere else and its still very hacky).

Claiming JSON IMO is the simplest solution so add-ons don't have to fight for it.

@derammo
Copy link
Contributor

derammo commented Sep 3, 2022

[edit: oops cross posting, didn't see your response.]

I guess I am suggesting a version of JSONResourceLoader/Saver that can be extended. Various plugins and modules register something like ".mymodule.json" for example, by subclassing/extending the JSON resource loader / saver to do whatever parsing they want. The core godot resource JSON format could use the same facility.

You could even provide a derived JSONDictionaryResourceLoader/Saver that puts the contents in a Variant structure, if that's what people want. Other modules might need to do their own data structures though, so those would use the raw base JSONResourceLoader/Saver

@reduz
Copy link
Member Author

reduz commented Sep 3, 2022

@derammo Its still very corner-casey and serves no real purpose IMO. Nobody will die if you can put any JSON anywhere. If you really want this, you can export the property yourself as a file path with the sub-extension and handle it on your own.

@Zireael07
Copy link
Contributor

@derammo your comment reminds me of another proposal where it was suggested to use .X.(t)res - also double extensions - for various types of resources

@derammo
Copy link
Contributor

derammo commented Sep 4, 2022

@Zireael07 just to be clear, I wouldn't have suggested implementing double extensions either. There is no reason to restrict the number of "dots" in an extension, unless you are using DOS. I was going to suggest using a longest-suffix match in the ResourceLoader, so that you can register ".something.json" or ".something.tres" and win out over the handler for just generic ".json" or ".tres". That way, plugins can register their data using whatever encoding makes sense for them, without conflict. That also covers the case you mentioned, where maybe semantically different resource files that happen to share the same container encoding can use ".X.tres."

Actually representing "sub-extensions" was just the straw man that reduz constructed.

@reduz
Copy link
Member Author

reduz commented Sep 4, 2022

@derammo most extension handling code uses string.get_extension(), which returns the last part of the string. This is the safest way to do it because if users use other dots for their resource naming, they may so for their own sake, such as:
myasset.final.png or things like that. Many times you also use assets that come from the internet with more than one dot.

If you enforce first dot instead of last dot, then you will break compatibility with many assets laying around or some people's workflows and this is a much more general annoyance to users. So basically your options are:

  • Make it recognize more than one dot and annoy users.
  • Make the extension handling code far more hackish to deal with double dots and not annoy users.
  • Do not support double extensions.

Given the scope of the "actual problem" here, and the fact that double extensions don't necessarily fix the problem (as as example, Spine will not use this because its unrelated to Godot) I think the third option is the way to go and hence why this PR makes a lot more sense.

@derammo
Copy link
Contributor

derammo commented Sep 4, 2022

[removed my comment as this is getting unhelpful for anyone]

@reduz reduz requested a review from a team as a code owner September 4, 2022 11:09
@reduz
Copy link
Member Author

reduz commented Sep 4, 2022

Changed "data" to be a property, so it can be serialized.

This makes the files ended in ".json" be treated as Godot resources.
This solves two problems:
* Avoid extensions to implement their own handling, which results in conflicts (all must use this one).
* Allow code to still work opening it as a file (since it will not be imported).
@reduz
Copy link
Member Author

reduz commented Sep 4, 2022

Ready for review.

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Github website review, the pull request does what I want and mimics the c++ module I did. Did not test execution. The data property is important because it means that games have json text input but store the final output as a binary dictionary saving serialized size.

@fire
Copy link
Member

fire commented Sep 4, 2022

Need to check if the exposed data property is editable in the property inspector, but the data model is good.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved in PR meeting, though would be good to know from @badlogic and @coppolaemilio if that solves the relevant use cases/conflicts as expected.

@badlogic
Copy link

Sorry for the super late reply, been busy with non-Godot work.

TL;DR: this change looks good to me and will resolve issues for pretty much all extension creators relying on .json, though in our case we may opt to go a different way.

In our case, we support two file encodings of skeletal data:

  1. .json, which is just a plain serialization of the skeleton data.
  2. .skel, which is a much smaller binary encoding of the skeleton data.

We have 2 EditorImportPlugin implementations, one that converts a .json file into a SpineSkeletonDataFileResource, and another one that converts a .skel file into a SpineSkeletonDataFileResource. A SpineSkeletonDataFileResource is then set on a SpineSkeletonDataResource (along with another resource representing the texture atlas), which actually parses the contents of the file resource and instantiates native objects accordingly. The SpineSkeletonDataResource is then used by one or more SpineSprite instances.

With this proposed change, we'd no longer have an EditorImportPlugin for .json files that generates a SpineSkeletonDataFileResource. Instead, the SpineSkeletonDataResource would have to either take a SpineSkeletonDataFileResource in case of a .skel file, or the new in-built JSON resource type in case of a .json file. That's workable, though I'm not happy that the generalization of SpineSkeletonDataFileResource across both .json and .skel files no longer works.

The other issue is that we support both Godot 3.4/3.5 and 4.0. With this change, it will be hard to impossible to keep compatibility between both versions and migration of 3.4/3.5 projects to 4.0 will become very tedious for end users, as they have to manually rewire all their resources. Unless there's a way for our extension to do this automagically, which I currently am unaware of. It is also a documentation issue for us, as there would be 2 different ways for setting up a SpineSkeletonDataResource depending on the Godot version. Historically, that is super confusing to users.

For this reason, we are likely to switch away from the .json extension and use our "own" .spine-json extension instead. This will let us keep our current more generalized architecture and allow users to more easily go from 3.4/3.5 to 4.0.

My initial assumption was, that importers are handed a chance to import a file via EditorImportPlugin::import(). If that returns an error state, I assumed the editor would hand the file to the next importer until success. Welp, turns out that's not how it works, and it's first come/first serve instead. So all of the above issues are entirely self-inflicted :)

@akien-mga akien-mga merged commit d1b2a19 into godotengine:master Sep 16, 2022
@akien-mga
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

7 participants