Make InstancePlaceholders in charge of resolving node references in instances

[TheOrioli]

This is my attempt at resolving #88662

From what I could figure out with my limited understanding of the codebase, the issue seems deeper than just the suspected PR #83343

I have created an even bigger repro project that tries to really abuse node properties, arrays and their resolutions during the instantiation process.

PlaceholderInstanceRepro.zip

This project not only fails in versions 4.3-dev.X, but also in 4.2.1, where arrays of external and mixed node references hit the error here

godot/scene/resources/packed_scene.cpp

Line 493 in b09f793

ERR_CONTINUE(!valid);

I am not nearly comfortable enough with the system to truly say that this is the correct solution, but I hope it can through reviews and discussion serve as a base of finding one.

Bugsquad edit:

• Fixes Regression in InstancePlaceholder behavior, instanced scene missing Node property values #88662

[TheOrioli]

@AThousandShips appreciate the review 🙌

I would just like to try and write some actual tests as well, seems there are no tests for instance placeholders and it might be cool to get some coverage on all this new code.

[AThousandShips]

Sounds great!

[TheOrioli]

@AThousandShips I've added some tests for the new behavior. There are still a couple of test cases that might be good to cover, specifically concerning properties that should contain Node's but their NodePath is overridden with the same value, so they reference an internal node after creation. This happens quite often now in the editor, since the editor seems to like setting the same value on the properties which is then stored as override in the PackedScene resource, as seen here:

But it would be good to get some eyes with testing experience to tell me if my current approach is actually good or not 🙌

[AThousandShips]

I'll try take a look soon

[TheOrioli]

There we go 😁 bda230e is a test that will fail on the current master branch, while it works with this PR. I will add a few additional tests in the same vein later today 💪

Is there an official/best practice way of creating and deleting temporary files created during tests?

[AThousandShips]

I'm not sure if there's any good practice for the creation or deletion, I'd suggest checking some of the other tests, like core/io, should be something in there

Code looks good I believe, wasn't able to do a functional evaluation at the moment though

[TheOrioli]

Seems like the core/io tests write into the OS cache directory. I've changed my tests to do the same. I am surprised there doesn't seem to be an OS singleton method to get the temporary directory of the filesystem, but that is outside the scope of this PR.

I don't know where more to take this, so I would say it's as ready as it will get until further reviews are done 👍

[TheOrioli]

Merged the latest master into the branch 👍

[AThousandShips]

In the future please use rebasing to update your branch, see here

[SaracenOne]

Thanks for this! This mostly looks good, but could you examine one issue I raised in regards to the editor losing direct node references. Also could you squash this PR into a single commit? This is generally the preferred way to merge PRs in Godot.

[TheOrioli]

@SaracenOne thank you for the review. Added the fix, squashed into one commit, rebased on master.

[SaracenOne]

LGTM 👍

[KoBeWi]

I'm not that familiar with InstancePlaceholder, but the code seems fine.

Is there an official/best practice way of creating and deleting temporary files created during tests?

The official practice seems to be polluting a "cache" directory 🙃
#92097

[TheOrioli]

@KoBeWi

I'm not that familiar with InstancePlaceholder, but the code seems fine.

Is there an official/best practice way of creating and deleting temporary files created during tests?

The official practice seems to be polluting a "cache" directory 🙃 #92097

Thank for the review, I'll remove the comment and push a new version.

Concerning #92097 I noticed OS seems to lack a temporary directory method, so I'm thinking about making a PR later adding support for it and seeing what happens to the tests if they use that instead.

[KoBeWi]

As I noted in the issue, it'd be fine if there was a test-specific helper method that returns a fixed sub-directory, so at least the files are isolated.

[akien-mga]

Thanks! And congrats for your first merged Godot contribution 🎉