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

Fix saving .tres file with Dictionary/Array containing Resources does not work #57077

Closed
wants to merge 1 commit into from

Conversation

Maran23
Copy link
Contributor

@Maran23 Maran23 commented Jan 23, 2022

NOTE: Probably superseded by #62318

Fixes #55885

After some discussion (see comments) and thinking I came to this solution.
The check, if a resource should be saved by checking whether it or its sub-resources have been edited now takes arrays and dictionaries into account.

I also tried changing the logic in the saving done by the ResourceFormatSaverTextInstance(resource_format_text.cpp), but found some problems/insecurities doing so:

  1. The logic in there is a bit different for PROPERTY_USAGE_RESOURCE_NOT_PERSISTENT
    and res->get_path().is_resource_file().
  2. I'm not sure if editor_node.cpp is only calling the save method of ResourceFormatSaverText. If not, then other subclasses of the ResourceFormatSaver needs to be adjusted as well (e.g. ResourceFormatSaverBinaryInstance, logic in here looks very similar).
  3. If a subresource was edited, we still need to save the whole main resource, otherwise data will be lost.

Example: A.tres

Array (first)
- [0] Array (second)
---- [0] String (edited)
- [1] String

Now if we were to save only the edited part, we would save the second array because the string in it was edited, and lose the first array and the file will be corrupt as a result.

So if we want to remove the logic in editor_node.cpp and let the resource_format_text.cpp do the whole job, it needs to be refactored to save the whole resource when at least one subresource was edited (and of course the other two points need to be clarified).

@Chaosus Chaosus added this to the 4.0 milestone Jan 23, 2022
editor/editor_node.cpp Outdated Show resolved Hide resolved
editor/editor_node.cpp Outdated Show resolved Hide resolved
@Maran23 Maran23 force-pushed the tres-dict-array-fix branch 2 times, most recently from d02cacb to 0c151e1 Compare January 23, 2022 22:39
@Maran23 Maran23 changed the title Fix saving .tres file with Dictionary/Array does not work Fix saving .tres file with Dictionary/Array containing Resources does not work Jan 23, 2022
@akien-mga akien-mga requested a review from a team March 29, 2022 21:26
@Maran23 Maran23 force-pushed the tres-dict-array-fix branch 5 times, most recently from f4f5e4d to f5b05f4 Compare April 2, 2022 13:12
@reduz
Copy link
Member

reduz commented Apr 5, 2022

I will give it a check soon. From what I remember this checked whether resources that were part of sub-resources forced the main one to be saved.

@Maran23
Copy link
Contributor Author

Maran23 commented May 2, 2022

I will give it a check soon. From what I remember this checked whether resources that were part of sub-resources forced the main one to be saved.

Nearly right. With my PR the code is more aligned to the actual saving algorithm. While the saving algorithm will also save all sub resources, so e.g. an Array in a .tres file. But the before called _find_edited_resources does not check whether sub resouces are edited and therefore should be saved later on.
Actually, my code can be further improved to also check nested resources of a nested resource and so on. Currently is does only check the first sub resource without going deeper, e.g. Array in Array won't be found with this.
That is probably more work given the current structure.

@reduz
Copy link
Member

reduz commented May 18, 2022

Okay sorry it took so long, I understand this bug well now as this is very old code. I am not entirely sure at this point whether we should even keep this code and instead now iterate through all loaded resources, check that they were edited and simply save them. I will discuss this a bit with other contributors and get back.

@Maran23
Copy link
Contributor Author

Maran23 commented May 24, 2022

Okay sorry it took so long, I understand this bug well now as this is very old code. I am not entirely sure at this point whether we should even keep this code and instead now iterate through all loaded resources, check that they were edited and simply save them. I will discuss this a bit with other contributors and get back.

No problem. I had the same thought. It is probably better to do that when saving. This will simplify the code since only location does the check and saving now - and it will probably also increase the performance a bit.

@Maran23 Maran23 force-pushed the tres-dict-array-fix branch from f5b05f4 to 10646b0 Compare June 12, 2022 18:15
@Maran23 Maran23 marked this pull request as draft June 12, 2022 18:42
… not work

The check, if a resource should be saved by checking whether it or it's subresources have been edited now takes arrays and dictionaries into account.
@Maran23 Maran23 force-pushed the tres-dict-array-fix branch from 10646b0 to 317b6e1 Compare June 12, 2022 20:46
@Maran23 Maran23 marked this pull request as ready for review June 12, 2022 21:14
@Maran23
Copy link
Contributor Author

Maran23 commented Jun 12, 2022

I updated this PR and it is now ready for review. I have written my results and findings in the description.
The solution now fixes the problem and will also work for nested arrays/dictionaries (unlike my previous approach).
So if we edit a value inside an array in an array in an array... the resource will be recognized and saved now.

@Maran23
Copy link
Contributor Author

Maran23 commented Jun 18, 2022

Btw. the CI error does not look like something caused by my changes (status code 503). Can someone retrigger the JavaScript Build? I think I can't.

@kleonc
Copy link
Member

kleonc commented Jun 18, 2022

Btw. the CI error does not look like something caused by my changes (status code 503). Can someone retrigger the JavaScript Build? I think I can't.

Done (and it indeed succeeded now).

@reduz
Copy link
Member

reduz commented Jun 22, 2022

I attempted an even simpler version of this in #62318 which should solve this problem too.

@akien-mga
Copy link
Member

Superseded by #62318. Thanks for the contribution nevertheless!

@akien-mga akien-mga closed this Jun 27, 2022
@Maran23 Maran23 deleted the tres-dict-array-fix branch October 5, 2022 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants