"Directory must be opened before use" errors while using some for the Directory methods

[Xrayez]

Godot version:
3.2.3.beta 6fd712c, likely due to #40151.

OS/device including version:
Windows 10

Issue description:

E 0:00:00.811   _Directory::change_dir: Directory must be opened before use.
  <C++ Error>   Condition "!is_open()" is true. Returned: ERR_UNCONFIGURED
  <C++ Source>  core\bind\core_bind.cpp:2424 @ _Directory::change_dir()
  <Stack Trace> test.gd:5 @ _ready()

Other methods are also affected: dir_exists, make_dir, make_dir_recursive, file_exists.

Steps to reproduce:

extends Node2D

func _ready():
	var dir = Directory.new()
	dir.change_dir("res://") # Fail here.
	dir.list_dir_begin()

	var file = dir.get_next()
	while not file.empty():
		print(file)
		file = dir.get_next()

This piece of snippet works fine in 3.2.2-stable.

I stumbled upon this while running a project with the plugin refresher, the plugins are not listed in 3.2.3 now.

Minimal reproduction project:
dir_access_chdir.zip

[Xrayez]

I'm not sure whether that's a regression or the fix provided in #40151 simply revealed the wrong usage of this method. Should the directory be opened explicitly now with open()?

Current documentation of change_dir():

Changes 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)

[Xrayez]

Changed the title because it seems like other methods are affected as well.

[akien-mga]

Yeah, this happens because #40151 "fixed" #24149, which is a "bug" that users tend to use as a feature unknowingly. Directory, like File, needs to be opened first before you can use other methods.

But while the change is likely fine for 4.0, it's probably not for 3.2.x so we should likely revert the cherry-pick or improve the implementation to be non-breaking. CC @nathanfranke

[akien-mga]

Alternatively, we could consider that Directory auto-opening res:// is a feature, and then we just need to set dir_open to true in the constructor (and make sure that the state is really exactly the same as if open() has been called).

Edit: But I think that would be a bad solution, as it's dangerous to have Directory auto-open res:// without users necessarily being aware of it, which can lead to weird issues like #24149 or actual loss of data like #40146.

So actually maybe this is justified to change in 3.2.3+, even if it breaks compat. At most we could maybe add some code so that change_dir() keeps working as it's harmless, and it could piggy back to open() if the dir was not opened explicitly yet (but that would only work within res://, which is the default access type).

[Xrayez]

Given how it may be easy to delete stuff within res:// on accident, I'm fine changing all the scripts to explicitly open directories before doing any operation, those are under Utils class anyway in my project, so should be easy.

[Xrayez]

But what about dir_exists and similar? Now you have to:

var dir = Directory.new()
dir.open("res://")
dir.dir_exists("res://")

instead of just:

var dir = Directory.new()
dir.dir_exists("res://")

Related proposal: godotengine/godot-proposals#1101 (CC @KoBeWi).

[Xrayez]

Upon grepping, I figured that I've got quite a bunch of plugins which do this, so that requires fixing some of them gradually by talking to respective authors first, or just fix them locally for now.

https://github.com/bitwes/Gut
https://github.com/godot-extended-libraries/godot-plugin-refresher
https://github.com/KOBUGE-Games/godot-logger

A lot of occurrences are indeed in the form of:

	var fs = Directory.new()
	var exists = fs.file_exists(path)

[nathanfranke]

The directory api seems very confusing to me. What is the difference between change_dir and open?

As for dir_exists, I agree that it shouldn't require the directory to be open

[Xrayez]

It seems like open() uses change_dir() under the hood:

godot/core/os/dir_access.cpp

Lines 229 to 243 in 85220fe

	DirAccess *DirAccess::open(const String &p_path, Error *r_error) {
	DirAccess *da = create_for_path(p_path);
	ERR_FAIL_COND_V_MSG(!da, nullptr, "Cannot create DirAccess for path '" + p_path + "'.");
	Error err = da->change_dir(p_path);
	if (r_error) {
	*r_error = err;
	}
	if (err != OK) {
	memdelete(da);
	return nullptr;
	}
	return da;
	}

I wouldn't really bother figuring the responsibility of those methods given that even reduz admits the current system is not great (from IRC #godotengine-devel):

Akien> Should we stand by the behavior change and enforce the use of `open()` to have a valid state (safer), or define that Directory auto-opening res:// is a feature?
Akien> I tend to prefer the former as the latter is dangerous, but maybe we can at least allow `var dir = Directory.new(); dir.change_dir(some_path)` to act as an alias for `open(some_path)` if nothing was opened.
reduz> Akien: I wish we could redo the fileaccess code in general, it just sucks
reduz> but its so much work
reduz> Godot 4.0 is a great chance for this

As for #40146, this could've also been fixed for the Directory.remove() method handling res:// path, I'm not sure if anyone would want to remove the project's root on purpose. 😃

[KoBeWi]

Looking at #40151, it's simply a matter of reverting the ERR_FAIL_CONDs to previous !d in some methods where opening the directory is unnecessary.