Fix typed array export #73256

KoBeWi · 2023-02-13T23:00:30Z

Borrowed from #67454
Fixes #62916

The issue has multiple duplicates, so I think it's better to have it fixed sooner than later. I rebased the PR and applied my comments. I also removed references to _editor_prop_ptr_indices, because it doesn't seem to be used anywhere (leftover from some old code?).

The code overall needs checking probably, I mostly copied the PR and made it work (it just needed a fix in array assigning).

YuriSizov · 2023-02-13T23:31:15Z

The issue has multiple duplicates, so I think it's better to have it fixed sooner than later.

Yes, but for all intents and purposes, this is a missing feature, not a bug. So it won't be too bad if we address this in 4.1.

The code overall needs checking probably, I mostly copied the PR and made it work (it just needed a fix in array assigning).

Reduz gave it a cursory look before, and said

that looks pretty good, but I think I would fix this after 4.0, at this point
I think its too risky to touch PackedScene
any issue and you can get corrupted files

KoBeWi · 2023-02-13T23:36:39Z

I wouldn't call it a missing feature. We do support node export and array export, they are just broken when used together.
But yeah, the changes seem a bit risky.

GuilhermeGSousa · 2023-02-14T09:02:43Z

Thanks for doing this, I didn't have time to work on my PR but I'll be testing you branch today! The array assigning fix was broken by the changes made to TypedArray, I found no fix for it yet but its the only thing missing from this PR.

akien-mga · 2023-02-14T10:08:13Z

scene/resources/packed_scene.cpp

+							if (Engine::get_singleton()->is_editor_hint()) {
+								// If editor, just set the metadata and be it
+								node->set(prop_name, prop_variant);
+								continue;
+							}


Why does it need to be special cased here? The only difference seems to be not using the META_POINTER_PROPERTY_BASE prefix, does it break Node arrays?

This will assign the raw array of NodePaths. META_POINTER_PROPERTY_BASE doesn't seem to work here. The condition can't be moved elsewhere and removing it crashes, so it's likely the only way.

akien-mga · 2023-02-14T10:12:36Z

scene/resources/packed_scene.cpp

+		if (string_property.contains("/indices/")) {
+			const Vector<String> properties = string_property.split("/");
+			Array array = dnp.base->get(properties[0]);
+
+			if (array.size() >= properties[2].to_int()) {
+				array.push_back(other);
+			} else {
+				array.set(properties[2].to_int(), other);
+			}


Is it possible for a non-Array property path to include /indices/ too, and possibly trigger this logic in an unexpected way and crash?

Or can an Array property include / in the first place, leading to the "/indices/" + itos(k)suffix being at different indices when splitting by/`?

Is it possible for a non-Array property path to include /indices/ too, and possibly trigger this logic in an unexpected way and crash?

Yes and no. This code only runs for "deferred NodePath properties", i.e. properties stored as NodePath that are assigned a node after the scene is complete. This limits severely what properties are affected by this code. Users can't use / in their properties without using _get_property_list(), so it's extremely unlikely that this logic is triggered when it shouldn't. And even if it does, assigning non-Array Variant will result in empty array, I think. So it shouldn't crash.

Or can an Array property include / in the first place, leading to the "/indices/" + itos(k)suffix being at different indices when splitting by/`?

The /indices/ properties are created further above, near your first comment. Wrong / are only possible if someone uses it wrongly.

akien-mga

Just left some comments for things which may warrant more eyes on them. I didn't check the existing code to see if what I point out are valid potential problems, I'm just trying to make sure we don't cut corners as PackedScene is very sensitive.

akien-mga · 2023-02-18T14:59:10Z

Given the current timetable and the risks from this PR, let's keep it for 4.1 and consider for a 4.0.x cherry-pick once merged and well tested.

In the meantime, it would be good to add a note in the relevant documentation to point to the missing feature.

reduz · 2023-04-05T09:38:27Z

I would appreciate the code is commented a bit more because given its hacky, its hard to understand exactly what is going on. Additionally there are some unanswered comments in the review.

YuriSizov · 2023-04-11T09:30:44Z

@KoBeWi Bump :)

Co-authored-by: Guilherme Sousa <guilherme.sousa1994@gmail.com>

KoBeWi · 2023-04-12T10:23:48Z

I would appreciate the code is commented a bit more because given its hacky, its hard to understand exactly what is going on.

I checked the code in detail and Node properties were already hacky. This PR just extends the hack to support arrays.

The changes are pretty safe, as they only affect the "deferred NodePath" properties. I rebased/fixed the PR and added some comments.

EDIT:
I also did a small refactoring regarding META_POINTER_PROPERTY_BASE, to avoid hard-coding it in the EditorPropertyArrayObject.

akien-mga · 2023-04-12T11:30:05Z

Thanks!

MaxIsJoe · 2023-04-13T00:43:35Z

Does this also fix it for C#?

romifauzi · 2023-04-23T02:27:49Z

Tested 4.1 Dev snapshot, upon assigning sprite2d node onto an Array of type Sprite2D, got this error:

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

Zireael07 · 2023-04-23T08:31:08Z

IIRC NodePath is a separate issue

romifauzi · 2023-04-23T10:59:31Z

Okay, I was under the assumption this PR fixed issue with dragging nodes into an exported typed array variable @Zireael07. So what does it fixed actually?

KoBeWi · 2023-04-23T11:25:26Z

This IS what the PR fixed, but somehow it's broken again on master.

KoBeWi · 2023-04-23T18:13:18Z

I opened #76378
It should be included when cherry-picking, because it fixes another issue.

YuriSizov · 2023-04-26T12:56:59Z

Skipping this from being cherry-picked for now, as there seem to be problems uncovered by this PR. Once those are addressed, we can cherry-pick in a 4.0.x release. For now, this should be available for testing in 4.1 dev snapshots, if you want to provide feedback and help catch all the bugs.

KoBeWi · 2023-04-26T13:00:31Z

The uncovered bugs are corner-case, while this PR fixes a very requested feature. Also the only so far discovered regression is fixed by #76378
Maaybe we can merge it for rc and revert before stable it causes too many problems? (which i doubt)

YuriSizov · 2023-04-26T13:02:19Z

We would prefer to test the changes with 4.1 builds before picking it for a patch release that doesn't get much testing before publishing.

thornySoap · 2023-05-29T11:08:02Z

It seems like it's not working on (at least my) Android export. If I OS.alert() the same Array on desktop debug and Android export, on desktop I get an Array with my Nodes, on Android just an empty one.

KoBeWi · 2023-05-30T00:01:16Z

@thornySoap Open a new issue, with a minimal reproduction project.
This isn't something that would have platform-specific issues 🤔

YuriSizov · 2023-06-29T11:19:55Z

Regarding the cherry-pick, see my comment here: #76378 (comment). We recommend updating to 4.1 if this issue greatly affects you.

KoBeWi added bug topic:editor labels Feb 13, 2023

KoBeWi added this to the 4.0 milestone Feb 13, 2023

YuriSizov requested review from a team February 13, 2023 23:33

GuilhermeGSousa mentioned this pull request Feb 14, 2023

Fix typed array export #67454

Closed

akien-mga reviewed Feb 14, 2023

View reviewed changes

akien-mga modified the milestones: 4.0, 4.1 Feb 18, 2023

akien-mga added the cherrypick:4.0 label Apr 3, 2023

Fix typed array export

2026101

Co-authored-by: Guilherme Sousa <guilherme.sousa1994@gmail.com>

KoBeWi force-pushed the OUR_pr branch from e7bfc2c to 2026101 Compare April 12, 2023 10:23

reduz approved these changes Apr 12, 2023

View reviewed changes

akien-mga merged commit 412721d into godotengine:master Apr 12, 2023

KoBeWi deleted the OUR_pr branch April 12, 2023 11:29

YuriSizov mentioned this pull request Apr 26, 2023

Fix PackedScenes created with PackedScene.pack() have their exported nodes become null #75429

Merged

akien-mga mentioned this pull request May 12, 2023

Can't assign nodes to exported array #62916

Closed

thornySoap mentioned this pull request May 30, 2023

Exported node arrays only work when debugging in the editor, but not with exported projects #77641

Closed

YuriSizov removed the cherrypick:4.0 label Jun 29, 2023

KoBeWi mentioned this pull request Mar 1, 2024

Remove useless if condition in EditorPropertyArrayObject as they always evaluate to false #88750

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix typed array export #73256

Fix typed array export #73256

KoBeWi commented Feb 13, 2023

YuriSizov commented Feb 13, 2023 •

edited

Loading

KoBeWi commented Feb 13, 2023

GuilhermeGSousa commented Feb 14, 2023 •

edited

Loading

akien-mga Feb 14, 2023

KoBeWi Apr 12, 2023

akien-mga Feb 14, 2023

KoBeWi Apr 12, 2023 •

edited

Loading

akien-mga left a comment

akien-mga commented Feb 18, 2023

reduz commented Apr 5, 2023

YuriSizov commented Apr 11, 2023

KoBeWi commented Apr 12, 2023 •

edited

Loading

akien-mga commented Apr 12, 2023

MaxIsJoe commented Apr 13, 2023

romifauzi commented Apr 23, 2023

Zireael07 commented Apr 23, 2023

romifauzi commented Apr 23, 2023

KoBeWi commented Apr 23, 2023

KoBeWi commented Apr 23, 2023

YuriSizov commented Apr 26, 2023

KoBeWi commented Apr 26, 2023 •

edited

Loading

YuriSizov commented Apr 26, 2023

thornySoap commented May 29, 2023 •

edited

Loading

KoBeWi commented May 30, 2023

YuriSizov commented Jun 29, 2023 •

edited

Loading

Fix typed array export #73256

Fix typed array export #73256

Conversation

KoBeWi commented Feb 13, 2023

YuriSizov commented Feb 13, 2023 • edited Loading

KoBeWi commented Feb 13, 2023

GuilhermeGSousa commented Feb 14, 2023 • edited Loading

akien-mga Feb 14, 2023

Choose a reason for hiding this comment

KoBeWi Apr 12, 2023

Choose a reason for hiding this comment

akien-mga Feb 14, 2023

Choose a reason for hiding this comment

KoBeWi Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

akien-mga left a comment

Choose a reason for hiding this comment

akien-mga commented Feb 18, 2023

reduz commented Apr 5, 2023

YuriSizov commented Apr 11, 2023

KoBeWi commented Apr 12, 2023 • edited Loading

akien-mga commented Apr 12, 2023

MaxIsJoe commented Apr 13, 2023

romifauzi commented Apr 23, 2023

Zireael07 commented Apr 23, 2023

romifauzi commented Apr 23, 2023

KoBeWi commented Apr 23, 2023

KoBeWi commented Apr 23, 2023

YuriSizov commented Apr 26, 2023

KoBeWi commented Apr 26, 2023 • edited Loading

YuriSizov commented Apr 26, 2023

thornySoap commented May 29, 2023 • edited Loading

KoBeWi commented May 30, 2023

YuriSizov commented Jun 29, 2023 • edited Loading

YuriSizov commented Feb 13, 2023 •

edited

Loading

GuilhermeGSousa commented Feb 14, 2023 •

edited

Loading

KoBeWi Apr 12, 2023 •

edited

Loading

KoBeWi commented Apr 12, 2023 •

edited

Loading

KoBeWi commented Apr 26, 2023 •

edited

Loading

thornySoap commented May 29, 2023 •

edited

Loading

YuriSizov commented Jun 29, 2023 •

edited

Loading