godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.6k stars 21.28k forks source link

Godot 3 Beta 1 crashed when moving files #13976

Closed gamaralf closed 6 years ago

gamaralf commented 7 years ago

Godot 3 Beta 1 on Ubuntu 17.10.

I created a new folder using the "FilesSystem" pane. Then I selected 2 files (one .gd and one .tscn), and selected "Move to" on the context menu, selected the destination folder, and the files were moved without errors. Then I repeated the same steps and Godot simply died after selecting the destination folder.

Godot's output:

→ OPENING: /home/USER/godot3/invasores (::home:USER::godot3::invasores)
OpenGL ES 3.0 Renderer: Mesa DRI Intel(R) Haswell Desktop 
GLES3: max ubo light: 409
GLES3: max ubo reflections: 455, ubo size: 144
ARVR: Registered interface: Native mobile
EditorSettings::_get - Warning, not found: text_editor/completion/callhint_tooltip_offset
EditorSettings::_get - Warning, not found: text_editor/completion/put_callhint_tooltip_below_current_line
ERROR: get_edited_scene_root: Index current_edited_scene=-1 out of size (edited_scene.size()=0)
   At: editor/editor_data.cpp:607.
ERROR: set_path: Another resource is loaded from path: res://beams.png
   At: core/resource.cpp:77.
fragment uses function: zero_if_equal
Making folder projetil in res://
call rescan!
Moving res://projetil.gd -> res://projetil/projetil.gd
  Remap: res://projetil.gd -> res://projetil/projetil.gd
Moving res://projetil.tscn -> res://projetil/projetil.tscn
  Remap: res://projetil.tscn -> res://projetil/projetil.tscn
Remapping dependencies for: res://jogador/nave.tscn
MUST RELOAD? 1
call rescan!
Making folder explosao in res://
call rescan!
Moving res://explosao.gd -> res://explosao/explosao.gd
  Remap: res://explosao.gd -> res://explosao/explosao.gd
Moving res://explosao.tscn -> res://explosao/explosao.tscn
  Remap: res://explosao.tscn -> res://explosao/explosao.tscn
Remapping dependencies for: res://explosao/explosao.tscn
MUST RELOAD? 0
Remapping dependencies for: res://main.tscn
call rescan!
handle_crash: Program crashed with signal 11
Dumping the backtrace. Please include this when reporting the bug on https://github.com/godotengine/godot/issues
[1] /lib/x86_64-linux-gnu/libc.so.6(+0x37140) [0x7f86264bc140] (??:0)
[2] /home/p_7269/programas/Godot_v3.0-beta1_x11.64() [0x1ae7440] (??:?)
[3] /home/p_7269/programas/Godot_v3.0-beta1_x11.64() [0xa7ed52] (??:?)
[4] /home/p_7269/programas/Godot_v3.0-beta1_x11.64() [0x1764729] (??:?)
[5] /home/p_7269/programas/Godot_v3.0-beta1_x11.64() [0x1a99baa] (??:?)
[6] /home/p_7269/programas/Godot_v3.0-beta1_x11.64() [0x19bc91f] (??:?)
[7] /home/p_7269/programas/Godot_v3.0-beta1_x11.64() [0xf3e6a6] (??:?)
[8] /home/p_7269/programas/Godot_v3.0-beta1_x11.64() [0x8de7ef] (??:?)
[9] /home/p_7269/programas/Godot_v3.0-beta1_x11.64() [0x450e45] (??:0)
[10] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf1) [0x7f86264a61c1] (??:0)
[11] /home/p_7269/programas/Godot_v3.0-beta1_x11.64() [0x4539cf] (??:0)
-- END OF BACKTRACE --
kubecz3k commented 7 years ago

I have seen this today

kubecz3k commented 7 years ago

Also this is probably quite recent issue since I was moving some files previously and it was fine

MillionOstrich commented 7 years ago

I've been having a bit of difficulty reproducing this crash reliably (though it did occur once). I have been able to see some errors in the console by doing the following steps:

The most common log i am getting for this move looks like:

Moving res://test/SceneB.tscn -> res://SceneB.tscn
  Remap: res://test/SceneB.tscn -> res://SceneB.tscn
Moving res://test/test_icon.png -> res://test_icon.png
  Remap: res://test/test_icon.png -> res://test_icon.png
Remapping dependencies for: res://SceneB.tscn
erasing <path>/SceneB.tscn
no access to <path>/test/SceneB.tscn
ERROR: FileAccessWindows::_get_modified_time: Method/Function Failed, returning: 0
   At: drivers\windows\file_access_windows.cpp:254
MUST RELOAD? 1
Remapping dependencies for: res://SceneA.tscn
erasing <path>/SceneA.tscn
call rescan!

Which I believe is occurring because it is trying to reload scene B because the icon moved, but the dependencies on scene B haven't yet been updated (See FileSystemDock::_update_dependencies_after_move).

By renaming the folder I have also seen the following error:

Moving res://test/ -> res://test2/
  Remap: res://test/SceneB.tscn -> res://test2/SceneB.tscn
  Remap: res://test/test_icon.png -> res://test2/test_icon.png
Remapping dependencies for: res://test2/SceneB.tscn
erasing <path>/test2/SceneB.tscn
no access to <path>/test/SceneB.tscn
ERROR: FileAccessWindows::_get_modified_time: Method/Function Failed, returning: 0
   At: drivers\windows\file_access_windows.cpp:254
MUST RELOAD? 1
ERROR: ResourceFormatLoaderText::load_interactive: Condition ' err != OK ' is true. returned: Ref<ResourceInteractiveLoader>()
   At: scene\resources\scene_format_text.cpp:904
ERROR: Failed loading resource: res://test/SceneB.tscn
   At: core\io\resource_loader.cpp:184
ERROR: SceneState::pack: Method/Function Failed, returning: err
   At: scene\resources\packed_scene.cpp:861
ERROR: EditorData::check_and_update_scene: Condition ' err != OK ' is true. returned: false
   At: editor\editor_data.cpp:569
Remapping dependencies for: res://SceneA.tscn
erasing <path>/SceneA.tscn
call rescan!

I think a potential solution to this case is to reload scenes after all the files affected by the dependencies being moved have been updated.

Edit: On closer look it appears this issue is in how scenes are reloaded - though i think making sure are the dependencies are renamed before attempting to reload is still probably a step in the right direction.

capnm commented 7 years ago

It also crashes on linux, when renaming a directory with some .tscn, .gltf, *.png, etc. files inside. After a godot restart everything works, i.e. the configuration change and the move action was actually successful.

RayKoopa commented 7 years ago

Without testing (not home), can it be this is EditorFileSystems or the resource import systems fault? I noticed it also crashes immediately when deleting the .import folder which is why I removed that option from the UI in this PR https://github.com/godotengine/godot/pull/13724 When looking over the code I just realized that it barely checks if files and folders still exist in the cached locations (which failed to update) and tries to read them anyway.

MillionOstrich commented 7 years ago

Further information on the second error log posted in my earlier comment.

The error is triggered because the scene we are reloading is instanced in another scene: https://github.com/godotengine/godot/blob/master/editor/editor_node.cpp#L4443-L4450

Godot handles this by reloading the currently active tab. The _get_modified_time error (and later errors) occurs because the active scene references the old dependencies, and then calls editor_data.check_and_update_scene which tries to get the modified time of the instanced scene which has already moved: https://github.com/godotengine/godot/blob/master/editor/editor_node.cpp#L2940 https://github.com/godotengine/godot/blob/master/editor/editor_data.cpp#L557

bool must_reload = _find_updated_instances(edited_scene[p_idx].root, edited_scene[p_idx].root, checked_scenes);

Because the file doesn't exist must_reload is set to true. It then attempts pack the scene, causing the remaining errors. Specifically when encountering the instanced node:

https://github.com/godotengine/godot/blob/master/scene/resources/packed_scene.cpp#427

    } else {
        //must instance ourselves
        Ref<PackedScene> instance = ResourceLoader::load(p_node->get_filename());
        if (!instance.is_valid()) {
            return ERR_CANT_OPEN;
        }

        nd.instance = _vm_get_variant(instance, variant_map);
    }

It appears to want to load the instanced node, which isn't working because the path on that node is not updated.

I think another potential issue here is that we are updating dependencies for scenes by updating the file only. What happens if that file is open and has unsaved changes in the editor (especially if these unsaved changes point to new instanced scenes)? Do we need some way to fix up dependencies in open editor scenes too?

reduz commented 6 years ago

@MillionOstrich I suppose what is missing here is updating path on loaded resources and scenes..

reduz commented 6 years ago

It should be fixed, but just in case please test.

ghost commented 6 years ago

Well, the crashes seem to be fixed now, but I still get a few problems.

After moving a folder containing open scripts, the scripts' names disappear from the script list: image

A few errors also get printed to Output, one for each blanked script: image

Those blanked scripts don't seem to point to the script files anymore, as whenever I get an error in my script the original script is opened (the one at the bottom of the list): image

Modifying those broken scripts and saving doesn't seem to do anything (I actually managed to crash Godot by trying to close one, but sadly I can't reproduce it)

There's also this, which happens when a scene containing those broken scrips is run (note that the scene still runs fine) image

The scenes and scripts do actually work correctly after being moved. It's probably just the script referencing within the editor that needs a little work.

Xrayez commented 4 years ago

Posting this in case someone stumbles upon this just like me.

  1. The blanked scripts are fixed by #40647 for when you delete a script.

  2. The errors base.begins_with("local://") are fixed by #40640.

  3. Another resource is loaded from path was attempted to be fixed in #40634. And well, it did fix those errors on simple renames/moving, but when you rename a folder with assets (images), it causes all assets to be re-imported, and it seems like I've got more warnings/errors popping up than before, so I wasn't confident in what I did... Moving or renames scripts currently affects class_name scripts in 3.2 and 4.0: #35442.

The scenes and scripts do actually work correctly after being moved. It's probably just the script referencing within the editor that needs a little work.

Likely yes, but it seems like it also requires to update the resource paths within the ScriptServer perhaps, I got stuck on this.