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

Enable dependency fixer for missing InstancePlaceholder paths #75103

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Flavelius
Copy link
Contributor

@Flavelius Flavelius commented Mar 19, 2023

Fixes #37818. Fixes #44720.

This treats placeholder paths as a dependency to invoke the regular ext_resource dependency fixer.
This also fixes file-reimport not retaining existing uids for resources that were renamed, and resource dependency renaming also not transitioning existing uids.

I chose to string-replace occurrences, as it was much more straightforward than parsing and rewriting everything and to not duplicate a lot of code in the process.

@Flavelius Flavelius changed the title Enable dependency fixer for missing placeholder node paths Enable dependency fixer for missing InstancePlaceholder paths Mar 19, 2023
@YuriSizov YuriSizov added this to the 4.x milestone Mar 20, 2023
@YuriSizov YuriSizov requested a review from a team March 20, 2023 14:06
@Rindbee
Copy link
Contributor

Rindbee commented Apr 29, 2023

Em, is there any reason why the scene referenced by InstancePlaceholder cannot be a recorded external resource?

Make it like an external resource, with additional flags to mark it, and it will be easier to update. May break the format of tscn/res files.

@Flavelius
Copy link
Contributor Author

Flavelius commented Apr 30, 2023

Em, is there any reason why the scene referenced by InstancePlaceholder cannot be a recorded external resource?

Make it like an external resource, with additional flags to mark it, and it will be easier to update. May break the format of tscn/res files.

In my understanding the main reason is that this would create a hidden reference loop of the scene instance to itself which Godot currently cannot handle. I guess you could technically try to convert the path to a reference on save and transform it back on load but that would equally be a hack. But maybe I didn't completely understand your proposed solution.

Edit: and my initial reasoning was to not change the behavior and api of something whose reasoning I do not fully understand, but maybe you're right and it can be designed like a regular instance reference

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants