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

Compare localized path against editor scene path when reloading #99741

Merged

Conversation

a-johnston
Copy link
Contributor

Fixes 99232. Tested for res and absolute paths in the gd MRP given in the issue.

@a-johnston a-johnston requested a review from a team as a code owner November 27, 2024 04:44
@AThousandShips AThousandShips added bug topic:editor cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Nov 27, 2024
@AThousandShips AThousandShips added this to the 4.4 milestone Nov 27, 2024
@KoBeWi
Copy link
Member

KoBeWi commented Dec 24, 2024

The issue is not a bug. No editor API returns an absolute path of an opened scene, you need to manually globalize it. There is just no reason to do so and no reason to support it.
At most I'd document in reload_scene_from_path() description that it only supports project-local paths.

EDIT:
Interestingly, if you provide a path to a scene that isn't open, the current scene will be reloaded by default. This could be noted too.

@a-johnston
Copy link
Contributor Author

🤷 I can't say I care too much about this, so I'll close the pr if it's not wanted. I just saw #99232 and noted that EditorNode::load_scene does handle converting absolute paths to local ones (and erroring if that path does not point to a local path) so I saw no reason why this couldn't accept an absolute path as well. It's not as though there is any ambiguity of what scene somebody is pointing at in the case of an absolute path so I don't know why the behavior wouldn't be made to match the other function while removing a quirk that would otherwise need to be documented.

@KoBeWi
Copy link
Member

KoBeWi commented Dec 25, 2024

So I tried loading external scene (with open_scene_from_path()) and got
image
Though even if it was possible, these 2 methods are different. reload_scene() works by comparing the requested path with opened scenes. If you open a scene from external path, it will be referenced by that path and you need to provide it to reload_scene(). The API is awkward, but there is no reason to refer to the same scene from both external and local path.

@a-johnston a-johnston force-pushed the reload_scene_compare_relative_path branch from 6b40183 to 224bafc Compare December 25, 2024 01:47
@a-johnston
Copy link
Contributor Author

Sorry for all the text, but I'm not sure that I understand the issue/pushback. I think there is confusion about the current behavior of the methods in question.

So I tried loading external scene (with open_scene_from_path()) and got image

To be sure, this pr doesn't change handling for "truly" external scenes as they couldn't have been opened in the first place due to this warning/error. open_scene_from_path/load_scene does allow (with a caveat, see below) absolute paths so long as they can be localized to a resource within the project data.

Though even if it was possible, these 2 methods are different. reload_scene() works by comparing the requested path with opened scenes. If you open a scene from external path, it will be referenced by that path and you need to provide it to reload_scene().

Both methods actually do this. I had considered this and it seemed like all of the scene path checks would exclusively use and pass along the localized path, but I manually verified that to be the case as well. This did uncover a new and related bug where if you do pass load_scene an absolute path to a scene within the project, it fails to detect if the scene is already open and always creates a new tab for the same scene, even if the scene was previously opened with the absolute path. The change to reload_scene didn't reveal this bug on its own because the scene gets removed before the path is loaded again. The PR has been updated to fix it though.

The API is awkward, but there is no reason to refer to the same scene from both external and local path.

IMHO the reason is that both paths resolve to the same data on disk and having inconsistent handling of absolute and relative paths for data within the project leads to bugs and a less permissive api. Both load_scene and reload_scene currently accept but do not correctly work with absolute paths, and in both cases you can easily end up with diverging or lost scene changes.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

It's not like localizing the path will hurt, so whatevs.

@akien-mga akien-mga merged commit e9b18fc into godotengine:master Jan 13, 2025
20 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release topic:editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ReloadSceneFromPath not reloading the scene
4 participants