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

Directory::change_dir Does Not Work Outside of res:// #35346

Closed
Capital-EX opened this issue Jan 20, 2020 · 6 comments · Fixed by #82849
Closed

Directory::change_dir Does Not Work Outside of res:// #35346

Capital-EX opened this issue Jan 20, 2020 · 6 comments · Fixed by #82849

Comments

@Capital-EX
Copy link

Godot version:
vGodot 3.1.2.stable.official

OS/device including version:
Windows 10

Issue description:
The method Directory::change_dir is not able to move to directories outside res:// directory. This is despite the documentation show that Directory::change_dir is capable of preforming such an action.
image
image

Steps to reproduce:

  1. Create a new project
  2. Create a node with a script
  3. Have the script try accessing somewhere outside res:// using Directory::change_dir.

Minimal reproduction project:
ChangeDirBug.zip

@timothyqiu
Copy link
Member

Directory opens res:// by default. In this scope, a path that starts with res:// will be transformed to a valid OS path. user:// will be accessed as is, failing the chdir call.

var directory = Directory.new()
directory.open("user://")

directory.change_dir("user://")

@Capital-EX
Copy link
Author

@timothyqiu thank you for explaining this; However, this still doesn't explain why change_dir fails when using OS.get_user_data_dir(). Going off of the current documention of Directory::change_dir, it is not behaving as it is described:

Change the currently opened directory to the one passed as an argument. The argument can be relative to the current directory (e.g. newdir or ../newdir), or an absolute path (e.g. /tmp/newdir or res://somedir/newdir).

Additionally, pathes starting with res:// are properly changed to. Therefor, one would assume pathes starting with user:// would work, too.

@sumit0190
Copy link
Contributor

So there is definitely some weirdness going on here, as my observations found out:

  1. user:// is not expected to work, and that makes sense, based on the explanation already provided above.
  2. But get_user_data_dir() should work. And on Linux, it doesn't error out! So you'd think that the directory really has changed, but printing out the current directory using get_current_dir() returns res://. Weird, right?
  3. So I look at the code, starting with Linux. The change_dir method in dir_access_unix.cpp
	if (base != String() && !try_dir.begins_with(base)) {
		ERR_FAIL_COND_V(getcwd(real_current_dir_name, 2048) == NULL, ERR_BUG);
		String new_dir;
		new_dir.parse_utf8(real_current_dir_name);
		if (!new_dir.begins_with(base)) {
			try_dir = current_dir; //revert
		}
	}

	// the directory exists, so set current_dir to try_dir
	current_dir = try_dir;
	ERR_FAIL_COND_V(chdir(prev_dir.utf8().get_data()) != 0, ERR_BUG);
	return OK;

Now at this point, we have already changed the directory. But here, base is the root directory of the project (i.e., where the project.godot file is located). And in the first if condition, we check whether try_dir (the directory we wish to change to, i.e., the path returned by get_user_data_dir()) starts with base. Now in most cases it won't, so we go inside the if, and using the getcwd command, we check for this condition...again. And since it fails (again), we 'revert' our changes, where try_dir is assigned the value from current_dir, which stored our original current directory (when execution began).

After exiting the if, we do a chdir again, this time to revert our changes (if the if branch was entered, which in our case it was), and so we end up back in our original directory.

  1. The Windows code in dir_access_windows.cpp is slightly different, even though it goes through the same basic steps:
bool worked = (SetCurrentDirectoryW(p_dir.c_str()) != 0);

	String base = _get_root_path();
	if (base != "") {

		GetCurrentDirectoryW(2048, real_current_dir_name);
		String new_dir;
		new_dir = String(real_current_dir_name).replace("\\", "/");
		if (!new_dir.begins_with(base)) {
			worked = false;
		}
	}

	if (worked) {

		GetCurrentDirectoryW(2048, real_current_dir_name);
		current_dir = real_current_dir_name; // TODO, utf8 parser
		current_dir = current_dir.replace("\\", "/");

	} //else {

	SetCurrentDirectoryW(prev_dir.c_str());
	//}

	return worked ? OK : ERR_INVALID_PARAMETER;
}

Note how it's pretty much the same thing as the Unix code, with the sole exception that we use a variable named worked to track if we have really changed to the right directory. After the second check to see if the newly changed location starts with base, we change this variable to false. And so even though we "revert" back to being in the original directory, the method does error out.

I am not sure exactly what conditions should and shouldn't be tested here, but with further help I can go deeper down the rabbit-hole.

@KoBeWi
Copy link
Member

KoBeWi commented Dec 17, 2020

Seems still valid in 3.2.4 beta4

@sszigeti
Copy link

sszigeti commented Sep 12, 2023

And in 3.5.2, at least on macOS.

Two, very similar error messages are shown when my game is accessing or deleting a save file in user://:

ERROR: Condition "getcwd(real_current_dir_name, 2048) == nullptr" is true.
   at: DirAccessUnix (drivers/unix/dir_access_unix.cpp:470)
ERROR: Condition "getcwd(real_current_dir_name, 2048) == nullptr" is true. Returned: ERR_BUG
   at: change_dir (drivers/unix/dir_access_unix.cpp:312)

Thankfully, the file gets deleted, but these messages are still unnerving.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 5, 2023

In Godot 4 with the new DirAccess class you have to open a directory when creating an instance. Both DirAccess.open("user://") and DirAccess.open("res://") work correctly, however you can't use change_dir() to change from res to user and vice versa; the directory has to be in the same scope. I think documenting it should be enough to resolve the issue.

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

Successfully merging a pull request may close this issue.

6 participants