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

Fix auto reload scripts on external change #49473

Merged
merged 1 commit into from
Dec 9, 2021

Conversation

cptchuckles
Copy link
Contributor

@cptchuckles cptchuckles commented Jun 10, 2021

fixes #49298

@KoBeWi
Copy link
Member

KoBeWi commented Sep 26, 2021

So I tested this and it doesn't seem to work? I tested with a simple script

extends Node
@export var test = 4

When I change value of test, it's supposed to be updated in the inspector, no? It doesn't happen.

@cptchuckles
Copy link
Contributor Author

cptchuckles commented Sep 27, 2021

@KoBeWi Without this patch, the editor actually doesn't reload the script at all and the major problem is when you add or remove export variables; you would have to close and reopen the entire scene in order to see the most current exports. This does get fixed. As I discovered and wrote in the related issue #49298, the problem is that GDScriptCache::get_full_script() doesn't check file modified time before returning what's in the cache when the editor regains focus, so it would never know if the script changed since it was cached.

However, like you I noticed recently that half of the time, the editor appears to first save the old value in the .tscn as a property of the node (and the reset button appears next to the value in the inspector), and other times the editor wouldn't save the old value but instead populate it with the new value from the script as expected. I don't know why that happens now, though, nor especially why it's intermittent. Months ago, when I first opened this PR, it actually did update values and would never save old values in the scene file, but now it does, so I'll probably have to track that down too.

@cptchuckles
Copy link
Contributor Author

I tracked the latest bug related to this (new defaults getting overridden by old defaults) down to 4e6efd1, which introduced C++ iterators. There were several places where E->get() was changed to just E where it shouldn't have been, one such occurrence being now fixed by #55350 which solves it. Too bad I didn't check the latest build before going on a bisect... lol

This patch is now once again working as expected on the latest revision. I still need a reviewer to tell me whether my solution is stupid or not though

@akien-mga akien-mga merged commit 1a8741a into godotengine:master Dec 9, 2021
@akien-mga
Copy link
Member

Thanks!

@nathanfranke
Copy link
Contributor

I get a SIGSEGV in my main project after this commit.

#0  0x000055555825084e in Map<StringName, GDScriptCodeGenerator::Address, Comparator<StringName>, DefaultAllocator>::_find (
    this=<error reading variable: Cannot access memory at address 0x7fffff7fefc8>, 
    p_key=<error reading variable: Cannot access memory at address 0x7fffff7fefc0>) at ./core/templates/map.h:299
#1  0x000055555824dc92 in Map<StringName, GDScriptCodeGenerator::Address, Comparator<StringName>, DefaultAllocator>::find (this=0x7fffff800748, 
    p_key=...) at ./core/templates/map.h:598
#2  0x000055555824c3fb in Map<StringName, GDScriptCodeGenerator::Address, Comparator<StringName>, DefaultAllocator>::operator[] (this=0x7fffff800748, 
    p_key=...) at ./core/templates/map.h:671
#3  0x0000555558239312 in GDScriptCompiler::_parse_expression (this=0x7fffff801b80, codegen=..., r_error=@0x7fffff80027c: OK, 
    p_expression=0x5555b4816b40, p_root=false, p_initializer=false, p_index_addr=...) at modules/gdscript/gdscript_compiler.cpp:226
#4  0x000055555823c49a in GDScriptCompiler::_parse_expression (this=0x7fffff801b80, codegen=..., r_error=@0x7fffff80027c: OK, 
    p_expression=0x5555b4816ca0, p_root=false, p_initializer=false, p_index_addr=...) at modules/gdscript/gdscript_compiler.cpp:670

@fire
Copy link
Member

fire commented Dec 10, 2021

Can report this breaks our project. I recommend to revert this change. We have a large project and it broke.

Teammate.
"Generally using timestamps for change checking is problematic."

fire pushed a commit to V-Sekai/godot that referenced this pull request Dec 10, 2021
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.

Auto Reload Scripts On External Change does not work
6 participants