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

New scene save system with JSON #4400

Closed
theludovyc opened this issue Apr 14, 2022 · 6 comments
Closed

New scene save system with JSON #4400

theludovyc opened this issue Apr 14, 2022 · 6 comments
Labels

Comments

@theludovyc
Copy link

theludovyc commented Apr 14, 2022

Describe the project you are working on

I work on this project in a team https://github.com/TeamConfiture/GWJ-43 . You can play it there https://pepotrouille.itch.io/slime-patrick.

Describe the problem or limitation you are having in your project

When we work on features, we create a branch on master, work on it, and merge it. Sometimes we work on same scene, but not on same things. So in .tscn sub and external Ids can be differents. Plus this thing [gd_scene load_steps=79 format=2].

When I merge, and have conflicts with Ids. I need to change them by hand.

Sometimes merge do not have conflicts but broke things.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

So to improve scene save system, I think use JSON format can be a good start. And after, remove Id numbers, and use Id strings.

I think sub resources can be register in node wich use them to avoid references.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

JSON is already supported by GodotEngine.

First, just change format. I think I can rewrite this function https://github.com/godotengine/godot/blob/12cb05b3040242deebb87262aed8c04218b69044/scene/resources/resource_format_text.cpp#L1587
And do same for load.
I give a link in master, but I'll start with 3.X branch.
I will start try to do this here https://github.com/theludovyc/godot (I am bored to resolve conflicts by hand ^^').

Two, remove Id numbers

Three, test if there are still conflicts in git

Four, make a tool to convert files in old system to new system

If this enhancement will not be used often, can it be worked around with a few lines of script?

N / A

Is there a reason why this should be core and not an add-on in the asset library?

Beacause it's save file system

@Mickeon
Copy link

Mickeon commented Apr 14, 2022

When we work on features, we create a branch on master, work on it, and merge it. Sometimes we work on same scene, but not on same things. So in .tscn sub and external Ids can be differents. Plus this thing [gd_scene load_steps=79 format=2].

Regarding Unique Subresource IDs, it's been implemented and it may be backported to 3.x. It seems like the issue described in the proposal would be solved with it?

@KoBeWi
Copy link
Member

KoBeWi commented Apr 14, 2022

Resource::generate_scene_unique_id() generates a unique ID and you are extremely unlikely to run into a conflict. The only issue that remains is that load_steps might differ.

Also changing format to JSON won't really help as the IDs need to somehow be stored anyway. So you'd get the same result, but with ugly JSON syntax.

@Mickeon
Copy link

Mickeon commented Apr 14, 2022

Yeah, I thought the main focus of .tres and .tscn was its readability and general ease of access. JSON is widely used, and there's some editors out there for it, but you can't do this nearly as easily, for instance: #3942 (comment)

SpriteFrames copy pasting example

Not to mention how awkward it'd be to store all the Variants Godot has to offer within it, since doing it simple wouldn't be standard ("position": Vector2(0,0) isn't valid JSON), and doing it appropriately would seriously impact readability.

"position": {
    "type" : "Vector2",
    "x" : 2.0,
    "y" : 3
}

@KoBeWi
Copy link
Member

KoBeWi commented Apr 14, 2022

Ah right, JSON doesn't support ints, so we can't even use it.

@theludovyc
Copy link
Author

Resource::generate_scene_unique_id() generates a unique ID and you are extremely unlikely to run into a conflict. The only issue that remains is that load_steps might differ.

Yep sub is great. But I worried about use of itos(index). I found this https://github.com/godotengine/godot/blob/12cb05b3040242deebb87262aed8c04218b69044/scene/resources/resource_format_text.cpp#L484. So I think everything is good. Just wait new version 😄

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

No branches or pull requests

3 participants