-
-
Notifications
You must be signed in to change notification settings - Fork 21.9k
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
When saving a resource with an exported typed array, check whether the type is an external resource #85024
When saving a resource with an exported typed array, check whether the type is an external resource #85024
Conversation
This needs to be solved assuming it's introduced by this PR
This should also not surface due to this PR if avoidable |
This one doesn't, will edit the OP to be clearer |
I don't see how this is a gdscript issue, exporting a typed array is not language specific. |
Based it on the specifics of the error report, wasn't possible to tell from it that it wasn't GDScript specific, updated both |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't find any issues with resource_format_text.cpp
, but it doesn't work well with resource_format_binary.cpp
. However, it seems that this PR does not introduce new issues, although it does not fix all.
A few notes:
- Renaming only works through the editor, since scripts do not yet support UID.
- Old scenes must be saved at least once so that the script path is replaced with
ExtResource
. Otherwise the issue persists. ResourceLoaderText
usesVariantParser
which supportsSubResource
/ExtResource
in a unified manner. AndResourceLoaderBinary
uses a custom format (even notMarshalls
which only support absoluteres://
paths for scripts). We should add support for typed arrays in the following places if possible without breaking compatibility:
godot/core/io/resource_format_binary.cpp
Lines 1843 to 1849 in a7b8602
case Variant::ARRAY: { | |
f->store_32(VARIANT_ARRAY); | |
Array a = p_property; | |
f->store_32(uint32_t(a.size())); | |
for (int i = 0; i < a.size(); i++) { | |
write_variant(f, a[i], resource_map, external_resources, string_map); | |
} |
godot/core/io/resource_format_binary.cpp
Lines 495 to 506 in a7b8602
case VARIANT_ARRAY: { | |
uint32_t len = f->get_32(); | |
Array a; //last bit means shared | |
len &= 0x7FFFFFFF; | |
a.resize(len); | |
for (uint32_t i = 0; i < len; i++) { | |
Variant val; | |
Error err = parse_variant(val); | |
ERR_FAIL_COND_V_MSG(err, ERR_FILE_CORRUPT, "Error when trying to parse Variant."); | |
a[i] = val; | |
} | |
r_v = a; |
Given that users typically use tscn
/tres
(not scn
/res
) during development, I think we could merge this PR as is, and leave exploring the possibility of fixing binary scenes/resources for later. @Jordyfel Let me know if you want to work on this, otherwise I'll add it to my TODO list.
@Jordyfel There's no conflicts, but could you rebase to make sure this passes CI and tests on latest master? The last rebase was a while ago. |
Rebased. I do want to try to fix the binary format properly |
Note that there are currently three kinds of typed arrays. You can see #78219 as an example. |
I see that reading and writing would have to be reimplemented in the binary format much like in the marshalls This makes me wonder, is there even a benefit to the binary format supporting typed arrays. It doesn't now and it hasn't come up. |
Thanks! |
Fix #84498
A type of an exported typed array (when it is a script) would not get recognized as an external resource if it was one, which prevented the script from being recognized as a dependency of the resource, which prevented the script path from being updated when moved (since scripts don't have UIDs, that couldn't help either).
Test with test_proj.zip by moving
res.gd
in and out of the additional dir.Before:
This doesn't allow the script path for the array type to be updated when that script is moved, making the scene unopenable.
After: