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 using Resource objects as keys in the tres format #64812

Merged
merged 1 commit into from
Jan 19, 2023

Conversation

Life4gal
Copy link
Contributor

@Life4gal Life4gal commented Aug 24, 2022

Version of Godot based on:
v4.0.alpha.custom_build [14f8a54a3]

The changed files are as follows:
godot\scene\resources\resource_format_text.cpp

The problem solved is as follows:

Fixes #57506

The minimal case that can be tested is as follows:
https://github.com/godotengine/godot/files/7977119/ResourcesTest.zip

Precautions:
ResourceSaver.save: now the first parameter is resource and the second is path, the code in this case is just the opposite.

Roughly the process of solving the problem:

After many tests, the execution process of saving resources is roughly as follows:

ResourceSaver::save(const Ref<Resource> &p_resource, const String &p_path, BitField<SaverFlags> p_flags);
ResourceSaver::save(const Ref<Resource> &p_resource, const String &p_path, uint32_t p_flags);
ResourceFormatSaverX->save(p_resource, path, p_flag);
ResourceFormatSaverX::save(const Ref<Resource> &p_resource, const String &p_path, uint32_t p_flags);
ResourceFormatSaverXInstance::save(const String &p_path, const Ref<Resource> &p_resource, uint32_t p_flags);
ResourceFormatSaverXInstance::_find_resources(const Variant &p_variant, bool p_main);
	--> Variant value = res->get(E.name);
 	--> Object::get(const StringName &p_name, bool *r_valid) const;
	--> script_instance->get(p_name, ret); --> OK

And the member variables in the script_instance of the incoming p_resource all exist, that is to say, they are the same in terms of parameters.

But I noticed that after the execution of

_find_resources(p_resource, true);

in

ResourceFormatSaverTextInstance::save(const String &p_path, const Ref<Resource> &p_resource, uint32_t p_flags);

The value of load_steps is significantly different(compared to ResourceFormatSaverBinaryInstance), and saved_resources does not save the resource of the key-value pair with resource as the key. But ResourceFormatSaverBinaryInstance doesn't have this problem.

So I checked

ResourceFormatSaverBinaryInstance::_find_resources(const Variant &p_variant, bool p_main);

And found that the key is also cached in the case where p_variant is Variant::DICTIONARY.

So I guess the problem with ResourceFormatSaverTextInstance is that it doesn't cache the key.

@nathanfranke
Copy link
Contributor

nathanfranke commented Aug 24, 2022

If your PR fixes an issue, please link it like this:

Fixes #57506

I also highly recommend checking how other PRs are formatted. 😃


Once your PR is ready for review again, squash your commits. PRs should be composed of a single commit, with no merge commits.

@Life4gal
Copy link
Contributor Author

If your PR fixes an issue, please link it like this:

Fixes #57506

I also highly recommend checking how other PRs are formatted. 😃

Once your PR is ready for review again, squash your commits. PRs should be composed of a single commit, with no merge commits.

I just force-pushed the commits before merging the master branch, don't know if it's OK. 😃

@Calinou Calinou added this to the 4.0 milestone Aug 24, 2022
@Calinou Calinou added cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release labels Aug 24, 2022
@fire fire changed the title fix issue 57506: Saving the resource as tres format will cause the resource as the key to be null because the key is not cached. Saving the resource as tres format will cause the resource as the key to be null because the key is not cached. Aug 24, 2022
@akien-mga
Copy link
Member

There's an issue with the GitHub workflows on this PR (not your fault), just testing a close and reopen to see if that solves it.

@akien-mga akien-mga closed this Aug 25, 2022
@akien-mga akien-mga reopened this Aug 25, 2022
@iamtoaster
Copy link
Contributor

Any news on this? Would be nice to be able to use custom resources as keys in dictionaries and have them serialize properly.

@reduz
Copy link
Member

reduz commented Jan 19, 2023

This looks good, but it needs to be fixed to pass CI

@akien-mga
Copy link
Member

Force pushed a rebase which should fix CI.

@akien-mga akien-mga changed the title Saving the resource as tres format will cause the resource as the key to be null because the key is not cached. Fix using Resource objects as keys in the tres format Jan 19, 2023
@akien-mga akien-mga merged commit 122106c into godotengine:master Jan 19, 2023
@akien-mga
Copy link
Member

akien-mga commented Jan 19, 2023

Thanks! And congrats for your first merged Godot contribution 🎉

@akien-mga akien-mga removed the cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release label Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:3.x Considered for cherry-picking into a future 3.x release topic:core
Projects
None yet
6 participants