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

Assign temporary path to preloaded resources #80281

Merged
merged 1 commit into from
Nov 12, 2023

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Aug 5, 2023

Fixes #72854 in a safe and very hacky way.
The issue is waiting for a "proper" fix (see the discussion in #72257), but it kept annoying me in the meantime so I'm coming with another hack 🤷‍♂️

I added a method to Resource that sets the path completely bypassing the cache, so Resource.get_path() will return the "correct" path, while not causing unwanted side-effects. The method is called for built-in resources loaded with CACHE_MODE_IGNORE.

It was not enough, because GDScript keeps its own path, set with the regular set_path() method. I could make set_temporary_path() virtual, but instead I added a flag to track whether the path is valid and try returning get_path() if it's not.

Here's an extended MRP for the debugger bug that this fixes:
PreloadBug.zip
Preloaded built-in scripts still can't be clicked for some reason, but at least you can see their path.

EDIT:
Fixes #80812

@@ -103,6 +103,7 @@ class Resource : public RefCounted {

virtual void set_path(const String &p_path, bool p_take_over = false);
String get_path() const;
void set_temporary_path(const String &p_path);
Copy link
Member

Choose a reason for hiding this comment

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

Have not looked in depth but since its hacky, maybe better to use a metadata, then clear it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Clear when? Assigning temporary metadata is prone to getting it saved accidentally.
(on a side note, it would be nice if we had a way to add metadata that isn't saved, like with a special prefix)

Copy link
Member

Choose a reason for hiding this comment

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

I would name it for what it does.

Suggested change
void set_temporary_path(const String &p_path);
void set_path_cache(const String &p_path);

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Seems relatively safe. Let's merge and test it in the next beta.

@akien-mga akien-mga merged commit 8928b20 into godotengine:master Nov 12, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the unacceptable branch November 12, 2023 13:14
@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 14, 2024

Well, that code isn't known to cause problems, so it wasn't necessary. This solution is basically a hack, so it should be used only where relevant.

@nikitalita
Copy link
Contributor

nikitalita commented Jan 14, 2024

Given your current fix, I'm not really sure why those instances above wouldn't cause problems. Right now, you're assigning the path_cache to both the internal resources and the main resource in resource_format_binary, but in resource_format_text, you're only assigning the path_cache to the internal resources. Unless you intended to only assign the path_cache to the internal resources in resource_format_binary?

@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 14, 2024

Right now, you're assigning the path_cache to both the internal resources and the main resource in resource_format_binary

I'm not, the code I added for both is the same.
The bug affects only internal resources.

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.

Crash when opening preloaded scene Opening a currently preloaded scene will remove paths from sub-resources
6 participants