Hot reload of @tool scripts stops working after first reload

dmlary commented 1 month ago

Tested versions

Reproduces in v4.3.stable.official [77dcf97d8]
Fixed in v4.4.dev.custom_build [621cadcf6] (confirmed fixed in this commit; prior commit still has issue)

System information

Godot v4.3.stable - macOS 14.6.1 - Vulkan (Forward+) - integrated Apple M3 Max - Apple M3 Max (16 Threads)

Issue description

@tool scripts do not hot reload after the first hot reload when using an external editor. During the first hot reload the following error is generated:

modules/gdscript/gdscript.cpp:745 - Condition "!p_keep_state && has_instances" is true. Returning: ERR_ALREADY_IN_USE

Note this is not the same issue as #97238 which is 4.4 specific.

Steps to reproduce

Create a new project
Add a Node3D
Attach a script to that Node3D
Make script a @tool script, and add a print to the _ready() function; save it
Switch back to godot editor
Save & reopen scene in editor (seems to be necessary on macos right now)
Confirm print from _ready()
Edit script to change print message; save script
Switch back to godot editor
Godot will display the ERR_ALREADY_IN_USE error
Close and reopen scene and updated _ready() message will be printed
Edit script, changing the print message again: save script
Switch back to godot editor
Godot will not print any new message
close & reopens scene and the message from the previous edit will be shown

Minimal reproduction project (MRP)

hot-reload-issue.zip

AThousandShips commented 1 month ago

Thank you for your report, consolidating in:

https://github.com/godotengine/godot/issues/97138

This has been fixed, and will be available in 4.4.dev3

dmlary commented 1 month ago

@AThousandShips This bug is present in 4.3. I put a comment in #97168 asking for a cherry-pick to 4.3.1, and they asked for a separate 4.3 issue.

AThousandShips commented 1 month ago

My bad, reopening, didn't notice

dmlary commented 1 month ago

Also confirmed that commit from #97168 (621cadcf651b93757d5dbf8969023ae62a16f1a4) fixes the issue. Prior to that commit, the problem still reproduces.

AThousandShips commented 1 month ago

Prior to that commit, the problem still reproduces.

Did you confirm it was actually occurring in between? Or did you cherry-pick the commit to 4.3?

dmlary commented 1 month ago

@AThousandShips bah, you're right. I only confirmed it was fixed on 4.4 branch. I'll see if I can cherry pick it.

AThousandShips commented 1 month ago

Since this was bisected to [9fd431f078d7560835da2642c9efe9e235618fe9] it's unlikely to be exactly the same, as the bug didn't occur prior to that commit, but a cherry pick will confirm if it fixes it

I'd say the bug was instead fixed at some point between 4.3 and that commit, to check you should also test in the latest 4.3.x development branch in case it might have been cherry picked then

dmlary commented 1 month ago

I cherry-picked 96382204736cbc131fbc2640ba6ba238c53017c0 onto 4.3-stable and the fix worked.

I'll test 4.3.x branch clean, and then cherry-picking it onto there.

dmlary commented 1 month ago

One important node about #97168 is that they went through looking for other places reload(false) and changed them to reload(true). I think that's why I'm seeing the fix on 4.3.

https://github.com/godotengine/godot/pull/97168#issuecomment-2364072932

AThousandShips commented 1 month ago

That implies a 4.3 specific fix is better than a cherry pick as we want to avoid any unforeseen issues from that

dmlary commented 1 month ago

confirmed broken in 4.3 branch as of v4.3.1.rc.custom_build [6699ae789]

cherry-pick 96382204736cbc131fbc2640ba6ba238c53017c0 onto 4.3 does have merge conflicts. Working through them.

That implies a 4.3 specific fix is better than a cherry pick as we want to avoid any unforeseen issues from that

Agreed. I'll see if I can narrow down 9638220 to the specific changes needed for 4.3.

dmlary commented 1 month ago

Worked through the merge conflicts and cherry-picking 9648220 onto the 4.3 branch works. Patch cherry-pick-9638220-onto-4.3.x.txt

At first glance, there's not much to trim down here.

dmlary commented 1 month ago

@AThousandShips I went through 96382204736cbc131fbc2640ba6ba238c53017c0 and tried to cut out anything that was specific to 4.4, but couldn't pinpoint anything that didn't also apply to 4.3.

@Hilderin as the author of #97168 can you think of anything in the fix that is 4.4 specific, and should be dropped from a backport to 4.3?

Hilderin commented 1 month ago

After some digging, I found the source of the issue for tool script reload outside the editor. This is a regression from #89261 which was merged for 4.3. This PR reintroduce the reload call without the true parameter. I was able to reproduce the issue on 4.3 stable with a small tool script.

We have 2 ways to fix the issue for 4.3.1:

First option

Chery pick #96002
Then cherry pick #97168 since it's basically a fix for a regression introduce in #96002 If you take all the modifications from #97168 but not the modifications from #96002, the external modified scripts that are not already in memory will not be reloaded because of the modifications in EditorFileSystem::_update_scan_actions in the first PR (#96002).

Second option

Only take the modifications in Script::reload_from_file from #97168

Personally, I think we need a bit more testing to cherry pick both PRs into 4.3.1 right now, It already created some problematic regressions. It really depends on when 4.3.1 is coming out and how much it was tested.

dmlary commented 1 month ago

@Hilderin thanks for taking a look.

Tomorrow I’ll cherry pick both of those commits on top of the 4.3 branch and use it as my primary godot version while I work on tool scripts. If there’s anything explicit you’d like me to test let me know.

dmlary commented 1 month ago

Cherry-picked 46edd6df5581098d8494f9b51a9791dcdabd0ee4 & 96382204736cbc131fbc2640ba6ba238c53017c0 on to the 4.3 branch (EDIT: fix commits to match edit from next comment). There's a merge conflict cherry-picking the second one, but it's a minor change.

They resolve my issue, but I'll continue using this build to see if anything else breaks.

dmlary commented 1 month ago

Ok, y'all are gonna think I'm nuts, but this is a cursed build when used in combination with godot-cpp (4.3-stable).

I cherry-picked the following commits onto 6699ae7897658e44efc3cfb2cba91c11a8f5aa6a:

46edd6df5581098d8494f9b51a9791dcdabd0ee4
96382204736cbc131fbc2640ba6ba238c53017c0 (EDIT: copied the wrong numbers initially)

I'm working on a gdextension providing hexagon gridmaps. I've spent probably the last 8 hours trying to figure out why cpu cycles are seemingly lost returning from random functions in the extension. For example the function says it took 150us, the calling function says the called function took 3ms. The place it went really sideways is that if I renamed the function, the issue went away. This happened with multiple functions -__-. This even happened with empty functions, so not a destructor issue.

This is related to the build because swapping back to 4.3-stable everything resolved itself. I need to figure out if the issue is that my godot-cpp is technically out of sync with the 4.3.1 godot branch.

~~I'll try that tomorrow.~~

(EDIT: tried godot-cpp/4.3 branch and issue is still there. Something is definitely broken)

Hilderin commented 1 month ago

I'm really not an expert in godot-cpp, but did you build the engine build with production these scons parameters: production=yes optimize=speed lto=full. I suggest you create a new issue or ask in contributor chat about that if you still have issue.

dmlary commented 1 month ago

@Hilderin I had doubts, but yea, you were right. Switching to the production build did cause all of those inexplicable hangs to disappear. I am baffled by what could have been causing the repeatable delay that was resolved by renaming my function.

I've been running the production release with the hotfixes for the past few days, and haven't seen any issues. I only use an external editor (nvim), so I cannot say what other behavior may be impacted by the changes.

dmlary commented 1 month ago

Just an update; I've spent the past few days actively developing scripts, and everything is still working great.

Is there anything else y'all would like tested to ensure this gets into the 4.3.1 release?

AstroStucky commented 3 weeks ago

Cherry-picked 46edd6d & 46edd6d on to the 4.3 branch. There's a merge conflict cherry-picking the second one, but it's a minor change.

You named the same commit twice; I assume by mistake. Could you please tell us what the second commit was?

dmlary commented 3 weeks ago

Cherry-picked 46edd6d & 46edd6d on to the 4.3 branch. There's a merge conflict cherry-picking the second one, but it's a minor change.

You named the same commit twice; I assume by mistake. Could you please tell us what the second commit was?

~~I edited it in the original comment when I first posted, but here again:~~ Nope, it was the next comment.

I cherry-picked the following commits onto https://github.com/godotengine/godot/commit/6699ae7897658e44efc3cfb2cba91c11a8f5aa6a:

https://github.com/godotengine/godot/commit/46edd6df5581098d8494f9b51a9791dcdabd0ee4 https://github.com/godotengine/godot/commit/96382204736cbc131fbc2640ba6ba238c53017c0

dmlary commented 3 weeks ago

And I've been running that build for the past 3 weeks or so with no issues.

godotengine / godot