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... again #76378

Merged

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Apr 23, 2023

Fixes #62916 again (regression from #76114)

CC @ajreckof

const Variant &original_array = object->get_array();

if (original_array.get_type() == Variant::ARRAY) {
// Needed to preserve type of TypedArrays in meta pointer properties.
Copy link
Member Author

@KoBeWi KoBeWi Apr 23, 2023

Choose a reason for hiding this comment

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

tbh I don't know when exactly it's needed, but it's specifically to fix Node arrays.
Regular TypedArrays might not need it, because they don't use hacks.

@ajreckof
Copy link
Member

ajreckof commented Apr 23, 2023

I am so sorry, I didn't know Node-typed Arrays where acting different so I only tested with one typed array and figured every other one would work the same. If i understand well what is happening here for node-typed exports _property_changed do not return a Node but only its NodePath (which i feel is strange). So you need to untype the array to be able to set and then retype the array later and at the same type convert the NodePath to a Node. If Nodes are the only strange case maybe we should detect if the value is a NodePath and if the subtype of the array is Object and in this case directly convert the NodePath to a Node.

I've given a test to your fix and it seems that it breaks if you try to use a property with a setter. What is happening is that because of the mismatch in type the setter is never called (but no error which is strange).

So I went a bit further and discovered that it is also a problem with direct Node export. I feel it would be better to try and fix the Node export so that they effectively emit property changed with a Node and not a NodePath.

Enregistrement2.mp4

resolving the problem shown just above would also resolve the current problem.

Edit : I'm working on the fix talked just above it should be ready in an hour or so i think
Edit 2 : okay it took more than an hour

@KoBeWi
Copy link
Member Author

KoBeWi commented Apr 24, 2023

I think this PR could be merged for 4.0.3, as it's less risky than your new solution.
Or maybe we could only cherry-pick the original fix and live with #76113 (as it's a bit corner-case).

@ajreckof
Copy link
Member

ajreckof commented Apr 24, 2023

As both the original issue and the issue found above are two corner cases, so i feel it won't be a problem if either of them is kept for a few more iterations until my solution is polished and thoroughly tested.
On whether my PR being undone or your fix being applied i feel it would be better to have your PR. It would improve a little and it is already done so why not take advantage of it.

@akien-mga akien-mga merged commit e2e870c into godotengine:master Apr 26, 2023
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the fix_typed_array_export_2-fix_harder branch April 26, 2023 12:16
@YuriSizov
Copy link
Contributor

We have discussed this with the production team and the overall opinion is that cherry-picking it would risk the stability of 4.0. There are follow-up fixes some of which haven't been merged or battle-tested yet, and the more of them we have to apply, the bigger the scope and the risk gets. So I'm removing the label for that reason. Please update to 4.1 if this affects your projects.

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
4 participants