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 typed array export #67454

Closed

Conversation

GuilhermeGSousa
Copy link
Contributor

@GuilhermeGSousa GuilhermeGSousa commented Oct 15, 2022

Fixes #62916.

Also confirmed that this works nicely together with #67055 to get exported arrays of custom node types.

@kleonc kleonc added this to the 4.0 milestone Oct 15, 2022
@GuilhermeGSousa
Copy link
Contributor Author

I'm closing this for now as this PR isn't really ready yet, there are still some issues on runtime. I'll reopen it once its ready

@GuilhermeGSousa GuilhermeGSousa marked this pull request as draft October 21, 2022 16:07
@GuilhermeGSousa
Copy link
Contributor Author

GuilhermeGSousa commented Oct 22, 2022

Some known issues to fix before undrafting this:

  • Exported arrays are not being saved correctly
  • Exported typed arrays dont work for custom Resources, although that is outside of the scope of the issue this PR is trying to fix, and would probably deserve its own issue/PR

@GuilhermeGSousa GuilhermeGSousa force-pushed the fix-array-export branch 4 times, most recently from a87e484 to 5321015 Compare October 23, 2022 09:46
@GuilhermeGSousa GuilhermeGSousa marked this pull request as ready for review October 23, 2022 11:44
@GuilhermeGSousa
Copy link
Contributor Author

@Paulb23 @YeldhamDev could I get a review for this one?

@mhilbrunner mhilbrunner requested review from Paulb23, YeldhamDev and a team November 29, 2022 11:15
@DmitriySalnikov
Copy link
Contributor

I didn't know about this PR and tried to fix the problem myself, my code is below. Here I just cut off metadata/_editor_prop_ptr_ if there is one (I don't know which option would be preferable 🤷‍♂️)

diff --git a/editor/editor_properties_array_dict.cpp b/editor/editor_properties_array_dict.cpp
index 451cb7cfee..606fc8239b 100644
--- a/editor/editor_properties_array_dict.cpp
+++ b/editor/editor_properties_array_dict.cpp
@@ -36,9 +36,10 @@
 #include "editor/editor_scale.h"
 #include "editor/editor_settings.h"
 #include "editor/inspector_dock.h"
+#include "scene/resources/packed_scene.h"
 
 bool EditorPropertyArrayObject::_set(const StringName &p_name, const Variant &p_value) {
-	String name = p_name;
+	String name = String(p_name).replace(SceneState::get_meta_pointer_property(""), "");
 
 	if (name.begins_with("indices")) {
 		int index = name.get_slicec('/', 1).to_int();
@@ -50,7 +51,7 @@ bool EditorPropertyArrayObject::_set(const StringName &p_name, const Variant &p_
 }
 
 bool EditorPropertyArrayObject::_get(const StringName &p_name, Variant &r_ret) const {
-	String name = p_name;
+	String name = String(p_name).replace(SceneState::get_meta_pointer_property(""), "");
 
 	if (name.begins_with("indices")) {
 		int index = name.get_slicec('/', 1).to_int();
@@ -159,8 +160,9 @@ EditorPropertyDictionaryObject::EditorPropertyDictionaryObject() {
 ///////////////////// ARRAY ///////////////////////////
 
 void EditorPropertyArray::_property_changed(const String &p_property, Variant p_value, const String &p_name, bool p_changing) {
-	if (p_property.begins_with("indices")) {
-		int index = p_property.get_slice("/", 1).to_int();
+	String name = p_property.replace(SceneState::get_meta_pointer_property(""), "");
+	if (name.begins_with("indices")) {
+		int index = name.get_slice("/", 1).to_int();
 		Variant array = object->get_array();
 		array.set(index, p_value);
 		emit_changed(get_edited_property(), array, "", true);

My changes and your changes for PackedScene work correctly (I couldn't apply all your changes via git apply). But only without @tool mode. With the @tool mode active, several errors appear when opening the scene and the array becomes null. Is this expected?

@lejar
Copy link

lejar commented Jan 25, 2023

Hi, is there anything I can do to help move this along? I'd like to use this feature in a project.

@GuilhermeGSousa
Copy link
Contributor Author

@lejar I just pushed @KoBeWi 's change request. I don't currently have my godot dev environment setup so I can't currently work on this PR, but everything should be here to fix the issue, its only missing a review.

@KoBeWi
Copy link
Member

KoBeWi commented Feb 2, 2023

Needs rebase. Also this doesn't seem to work anymore, probably after TypedArray changes.

Attempted to set a variable of type 'NodePath' into a TypedArray of type 'Object'.
core\variant\array.cpp:410 - Condition "!_p->typed.validate(value, "set")" is true.

@@ -404,7 +425,20 @@ Node *SceneState::instantiate(GenEditState p_edit_state) const {

for (const DeferredNodePathProperties &dnp : deferred_node_paths) {
Node *other = dnp.base->get_node_or_null(dnp.path);
dnp.base->set(dnp.property, other);
if (String(dnp.property).contains("/indices/")) {
Copy link
Member

Choose a reason for hiding this comment

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

Avoid converting StringName to String more than once. Do

String string_property = dnp.property;

@@ -238,16 +238,37 @@ Node *SceneState::instantiate(GenEditState p_edit_state) const {

if (nprops[j].name & FLAG_PATH_PROPERTY_IS_NODE) {
uint32_t name_idx = nprops[j].name & (FLAG_PATH_PROPERTY_IS_NODE - 1);
NodeData::Property nprop = nprops[j];
Variant prop_variant = props[nprop.value];
StringName prop_name = snames[name_idx];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
StringName prop_name = snames[name_idx];
const StringName &prop_name = snames[name_idx];

(technically applies to other variables too)

@GuilhermeGSousa
Copy link
Contributor Author

Thanks @KoBeWi I'll check this out

@YuriSizov YuriSizov modified the milestones: 4.0, 4.1 Feb 11, 2023
@dannflor
Copy link

@GuilhermeGSousa are you still working on this? It's a pretty bad blocker for me and I'd like to see it merged sooner than later.

@KoBeWi KoBeWi mentioned this pull request Feb 13, 2023
@GuilhermeGSousa
Copy link
Contributor Author

Closing this in favor of #73256

@GuilhermeGSousa GuilhermeGSousa deleted the fix-array-export branch February 14, 2023 09:22
@YuriSizov YuriSizov removed this from the 4.1 milestone Feb 21, 2023
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.

Can't assign nodes to exported array
8 participants