Update GDScript 2.0 engine to use weak references for scripts and packed scenes to enable cyclic references by adamscott · Pull Request #65752 · godotengine/godot

adamscott · 2022-09-13T17:09:40Z

Superseded by #67714

TL;DR

This PR makes possible to use cyclic references in GDScript 2.0

Summary

This PR makes the GDScript 2.0 engine compatible with weak references of Scripts and GDScripts. Instead of passing the scripts around, which makes the counter up, this PR stores the scripts inside their own weak references. So, the scripts can be used as much as needed without making the counter up. This makes cyclic references possible.

What it tries to solve

The current GDScript engine uses strong references to store the scripts and scenes that it loads. This usually work well in the standard situations, as those scripts and scenes are RefCounted objects.

Standard case

Let's say Main loads A that loads B.

Here, Main reference count is 1 (referenced by the engine), A is 1 (referenced by Main) and B is 1 (referenced by A).

When the engine unloads, Main reference count will be decremented by 1 and will be deleted, because it's reference count will hit 0. A and B will also be deleted because A lost his reference from Main and B will lose it's reference from A later.

Current cyclic reference issue

Currently, the engine makes it so that using cyclic references is impossible. Why, you say?

Let's bring back our example, but with a cyclic reference. Let's say Main loads A that loads B that loads back A.

Here, Main reference count is 1 (referenced by the engine), A is 2 (referenced by Main and B) and B is 1 (referenced by A).

Do you see the issue? When Main will be deleted, there will be no way to delete A and B. A will lose it's reference to Main and will decrement, but it will not be deleted as A reference count is now 1. B is not deleted because it didn't lose it's reference to A.

So, the answer is: the engine makes cyclic references impossible due to memory leaks concerns. That explains errors thrown by the parser or the analyzer like Constant value uses script from "res://b.gd" which is loaded but not compiled.

Proposed solution

Let's bring back our example.

Main loads A that loads B that loads back A.

What if loading increments a wrapper instead of the script reference? And what if we use a dependency chart to detect when a script needs to be deleted?

Weak references

Instead of using strong references for scripts and scenes, this PR uses weak references. Weak references are referenced values that holds another value. When accessing that later value, it doesn't increase the reference counter, making it so that it does not matter how cyclic references are.

Instead of having a strong reference to A in B (which would increase A to 2), let's use altogether weak references for scripts and scenes. B will still be able to use A, but A reference count will stay at 1.

The engine had to be changed to support values that may be a weak reference or not. Hence the rather heavy PR.

Dependency chart

If we don't use references as the way to dispose of the loaded objects, we need another way to delete them. Here comes the dependency chart.

Take the dependency chart of our example

Main
- loads A
A
- loads B
B
- loads A

With this, we know that A and B cannot be unloaded. Because A and B are found in other dependencies. But when Main will be disposed of, it's only a matter of disposing of every linked dependency.

List of changes:

Add weak references to support preload cyclic references
Add new algorithm for removing scripts based on dependencies
Expose GDScriptCache::remove_script()
Use .notest.gd system to bypass tests
Add ScriptRef
Add weakref for GDScriptParser
Add global classes support (replaces Fix cyclic references issues with global classes #65664)
Add packed scenes support

Known issues

heap-use-after-free or leaks issues

Code example

So, this code works with this PR, which returns this error on master: Constant value uses script from "res://b.gd" which is loaded but not compiled.:

# main.gd
const A = preload("res://a.gd")

func _ready() -> void:
	A.test()  # The console will print: "A.HELLO_WORLD: hello, world!"

# a.gd
extends Node

const B = preload("res://b.gd")

const HELLO_WORLD: = "hello, world!"

static func test() -> void:
	B.test()

	# Test cyclic `is` keyword
	var b_instance: = B.new()
	if b_instance is B:
		prints("b_instance is B")  # The console will print: "b_instance is B"
	b_instance.queue_free()

# b.gd
extends Node

const A = preload("res://a.gd")

static func test() -> void:
	prints("A.HELLO_WORLD:", A.HELLO_WORLD)

	# Test cyclic `is` keyword
	var a_instance: = A.new()
	if a_instance is A:
		prints("a_instance is A")  # The console will print: "a_instance is A"
	a_instance.queue_free()

Minimal reproduction project:

cyclic3.zip

Fixes

Fixes #21461 - Some usages of class_name can produce cyclic errors unexpectedly
Fixes #32165 - Cyclic Errors After Autoload of Imported Asset
Fixes #41027 - Cyclic reference error in GDScript 2.0
Fixes #58551 - Can't preload a scene in AutoLoad script when the scene's script references the AutoLoad
Fixes #58181 - GDScript 2.0: Cannot preload cyclic const script references
Fixes #58871 - Cyclic Autoload Singleton Plugin Bug
Fixes #61043 - New cyclic reference error with autoload and preloaded script

Notes

This PR was the follow-up to #65672.
This PR replaces #65664 as it implements global classes too.

KoBeWi · 2022-09-23T22:01:53Z

I tested it with my project and got this crash:

CrashHandlerException: Program crashed
Engine version: Godot Engine v4.0.beta.custom_build (f74491fdee9bc2d68668137fbacd8f3a7e7e8df7)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[0] ObjectID::is_null (C:\godot_source\core\object\object_id.h:47)
[1] ObjectID::is_null (C:\godot_source\core\object\object_id.h:47)
[2] WeakRef::get_ref (C:\godot_source\core\object\ref_counted.cpp:103)
[3] ScriptRef::get_ref (C:\godot_source\core\object\script_language.h:258)
[4] GDScriptRef::get_ref (C:\godot_source\modules\gdscript\gdscript.h:540)
[5] ResourceFormatLoaderGDScript::load (C:\godot_source\modules\gdscript\gdscript.cpp:2402)
[6] ResourceLoader::_load (C:\godot_source\core\io\resource_loader.cpp:213)
[7] ResourceLoader::_thread_load_function (C:\godot_source\core\io\resource_loader.cpp:240)
[8] ResourceLoader::load (C:\godot_source\core\io\resource_loader.cpp:570)
[9] EditorAutoloadSettings::_create_autoload (C:\godot_source\editor\editor_autoload_settings.cpp:403)
[10] EditorAutoloadSettings::EditorAutoloadSettings (C:\godot_source\editor\editor_autoload_settings.cpp:860)
[11] ProjectSettingsEditor::ProjectSettingsEditor (C:\godot_source\editor\project_settings_editor.cpp:678)
[12] EditorNode::EditorNode (C:\godot_source\editor\editor_node.cpp:6664)
[13] Main::start (C:\godot_source\main\main.cpp:2754)
[14] widechar_main (C:\godot_source\platform\windows\godot_windows.cpp:176)
[15] _main (C:\godot_source\platform\windows\godot_windows.cpp:201)
[16] main (C:\godot_source\platform\windows\godot_windows.cpp:215)
[17] WinMain (C:\godot_source\platform\windows\godot_windows.cpp:229)
[18] __scrt_common_main_seh (D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[19] BaseThreadInitThunk
-- END OF BACKTRACE --

adamscott · 2022-09-24T17:32:12Z

@KoBeWi Thanks! This should be fixed by now.

KoBeWi · 2022-09-24T20:13:00Z

Another crash, this time I didn't get any stack trace, had to use debugger to find it:

CowData<char32_t>::size() Line 133 (c:\godot_source\core\templates\cowdata.h:133)
CowData<char32_t>::get(int p_index) Line 157 (c:\godot_source\core\templates\cowdata.h:157)
String::operator[](int p_index) Line 224 (c:\godot_source\core\string\ustring.h:224)
String::get_data() Line 970 (c:\godot_source\core\string\ustring.cpp:970)
String::hash() Line 2688 (c:\godot_source\core\string\ustring.cpp:2688)
HashMapHasherDefault::hash(const String & p_string) Line 306 (c:\godot_source\core\templates\hashfuncs.h:306)
HashMap<String,Ref<GDScript>,HashMapHasherDefault,HashMapComparatorDefault<String>,DefaultTypedAllocator<HashMapElement<String,Ref<GDScript>>>>::_hash(const String & p_key) Line 85 (c:\godot_source\core\templates\hash_map.h:85)
HashMap<String,Ref<GDScript>,HashMapHasherDefault,HashMapComparatorDefault<String>,DefaultTypedAllocator<HashMapElement<String,Ref<GDScript>>>>::_lookup_pos(const String & p_key, unsigned int & r_pos) Line 106 (c:\godot_source\core\templates\hash_map.h:106)
HashMap<String,Ref<GDScript>,HashMapHasherDefault,HashMapComparatorDefault<String>,DefaultTypedAllocator<HashMapElement<String,Ref<GDScript>>>>::erase(const String & p_key) Line 315 (c:\godot_source\core\templates\hash_map.h:315)
GDScriptCache::remove_dependencies(const String & p_source, const String & p_path, const bool & repeat) Line 136 (c:\godot_source\modules\gdscript\gdscript_cache.cpp:136)
GDScriptCache::remove_dependencies(const String & p_source, const String & p_path, const bool & repeat) Line 141 (c:\godot_source\modules\gdscript\gdscript_cache.cpp:141)
GDScriptCache::remove_dependencies(const String & p_source, const String & p_path, const bool & repeat) Line 141 (c:\godot_source\modules\gdscript\gdscript_cache.cpp:141)
GDScriptCache::remove_dependencies(const String & p_source, const String & p_path, const bool & repeat) Line 141 (c:\godot_source\modules\gdscript\gdscript_cache.cpp:141)
GDScriptCache::remove_dependencies(const String & p_source, const String & p_path, const bool & repeat) Line 141 (c:\godot_source\modules\gdscript\gdscript_cache.cpp:141)
GDScriptCache::remove_dependencies(const String & p_source, const String & p_path, const bool & repeat) Line 141 (c:\godot_source\modules\gdscript\gdscript_cache.cpp:141)
GDScriptCache::remove_dependencies(const String & p_source, const String & p_path, const bool & repeat) Line 141 (c:\godot_source\modules\gdscript\gdscript_cache.cpp:141)
GDScriptCache::remove_dependencies(const String & p_source, const String & p_path, const bool & repeat) Line 141 (c:\godot_source\modules\gdscript\gdscript_cache.cpp:141)
GDScriptCache::remove_dependencies(const String & p_source, const String & p_path, const bool & repeat) Line 141 (c:\godot_source\modules\gdscript\gdscript_cache.cpp:141)
GDScriptCache::remove_dependencies(const String & p_source, const String & p_path, const bool & repeat) Line 141 (c:\godot_source\modules\gdscript\gdscript_cache.cpp:141)
GDScriptCache::remove_dependencies(const String & p_source, const String & p_path, const bool & repeat) Line 141 (c:\godot_source\modules\gdscript\gdscript_cache.cpp:141)

EDIT:
Wait, it's truncated. It's actually stack overflow. Unfortunately I can't get the full stack.
The maximum number of stack frames supported by Visual Studio has been exceeded.

bitbrain · 2022-10-04T17:12:19Z

I opened this in my Godot 4 ported RPG (around 2 years of development on it already) and it gave me no errors either! (apart from regression errors that are already covered by other issues)

h0lley · 2022-10-04T17:50:08Z

for my project any of the errors / crashes I posted earlier in this thread have been resolved as well!
however there are still a couple of errors being logged, although I cannot link them to anything malfunctioning.

I get this one when loading the project in the editor or when starting test play:

E 0:00:01:0015   _ready: Attempt to open script '' resulted in error 'File not found'.
  <C++ Error>    Condition "err" is true. Returning: err
  <C++ Source>   modules/gdscript/gdscript.cpp:1065 @ load_source_code()
  <Stack Trace>  boot.tscn::1:40 @ _ready()

the file in question appears to be a built-in script attached to the main scene.
line 40 in said script contains get_tree().change_scene_to_file("...").
unfortunately I haven't been able to reconstruct it in a minimal project so far.

then, when dragging GD script files that register classes between folders in the FileSystem panel, I get these errros:

  Attempt to open script 'res://cyclic/class_a.gd' resulted in error 'File not found'.
  res://cyclic/class_a.gd:2 - Parse Error: Class "TestA" hides a global script class.

this can be easily reconstructed.
for instance in this project: tmp.zip

drag the class_a.gd from cyclic into res://
notice error: res://class_a.gd:2 - Parse Error: Class "TestA" hides a global script class.
drag it back into cyclic
notice error: Attempt to open script 'res://class_a.gd' resulted in error 'File not found'.

bitbrain · 2022-10-06T10:00:32Z

Hopefully this makes it into Beta 3 keeps fingers crossed

bitbrain · 2022-10-06T12:35:26Z

modules/gdscript/gdscript_cache.cpp

would it make more sense to introduce a method for this that something like clear_cache that unreferences all references and then clears it afterwards? You then can call it on both the shallow_gdscript_cache and full_gdscript_cache

~~This code was removed, as this was quite a ugly hack. Now, as references are not leaking, clearing shallow_gdscript_cache and full_gdscript_cache suffice.~~

Just fixed it.

Add new algorithm for removing scripts based on dependencies Expose GDScriptCache::remove_script() Use .notest.gd system to bypass tests Add ScriptRef Add weakref for GDScriptParser Add global classes support (replaces PR#65664) Add GDScriptResource::get() Add packed scene script support

adamscott · 2022-10-21T13:59:41Z

Superseded by #67714

adamscott requested a review from a team as a code owner September 13, 2022 17:09

adamscott mentioned this pull request Sep 13, 2022

Fix cyclic references issues with global classes #65664

Closed

akien-mga added bug topic:gdscript labels Sep 13, 2022

akien-mga added this to the 4.0 milestone Sep 13, 2022

adamscott force-pushed the fix-preload-cyclic-references branch from 985105d to 79a1203 Compare September 13, 2022 20:37

adamscott marked this pull request as draft September 16, 2022 10:21

adamscott force-pushed the fix-preload-cyclic-references branch 9 times, most recently from 27a4429 to e7a96d3 Compare September 22, 2022 22:35

adamscott marked this pull request as ready for review September 22, 2022 22:36

adamscott requested review from a team as code owners September 22, 2022 22:36

adamscott force-pushed the fix-preload-cyclic-references branch 2 times, most recently from 4c8a512 to f5eb5d7 Compare September 23, 2022 16:36

adamscott changed the title ~~Fix preload cyclic references~~ Update GDScript engine to use weak references for scripts (formely fix preload cyclic references) Sep 23, 2022

adamscott force-pushed the fix-preload-cyclic-references branch from f5eb5d7 to 42787db Compare September 24, 2022 17:31

adamscott force-pushed the fix-preload-cyclic-references branch 4 times, most recently from 44a11de to 582af39 Compare September 24, 2022 23:51

adamscott changed the title ~~Update GDScript engine to use weak references for scripts and packed scenes to enable cyclic references~~ Update GDScript 2.0 engine to use weak references for scripts and packed scenes to enable cyclic references Oct 2, 2022

adamscott force-pushed the fix-preload-cyclic-references branch from bbdcb23 to 36a4c42 Compare October 3, 2022 00:14

rune-scape mentioned this pull request Oct 4, 2022

Fixed wrong base class when using extends Base.Subclass #66781

Closed

adamscott force-pushed the fix-preload-cyclic-references branch from 36a4c42 to 936aa21 Compare October 4, 2022 21:04

bitbrain reviewed Oct 6, 2022

View reviewed changes

adamscott force-pushed the fix-preload-cyclic-references branch 5 times, most recently from 265377d to df5a846 Compare October 9, 2022 16:42

adamscott added 11 commits October 11, 2022 21:05

Refactor gdscript cache clear

a686c19

Add packed scene cache clear

dfd3349

Add nullcheck

265572e

Remove (superflouous?) file check

31ac219

Restore gdscript_byte_codegen.h

b065266

Remove superflouous null check

9276e11

Remove whitespace

a40dba0

Reinstate clear in parse

74cc74f

Remove WeakRef checks in variant_call.cpp

993ebdd

Reinstate global parser reload

032a7d1

adamscott force-pushed the fix-preload-cyclic-references branch from df5a846 to 032a7d1 Compare October 12, 2022 01:06

WIP

3fcdf98

adamscott mentioned this pull request Oct 21, 2022

Fix cyclic references in GDScript 2.0 #67714

Merged

adamscott closed this Oct 21, 2022

KoBeWi added the archived label Oct 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Update GDScript 2.0 engine to use weak references for scripts and packed scenes to enable cyclic references#65752

Update GDScript 2.0 engine to use weak references for scripts and packed scenes to enable cyclic references#65752
adamscott wants to merge 12 commits intogodotengine:masterfrom
adamscott:fix-preload-cyclic-references

adamscott commented Sep 13, 2022 •

edited

Loading

Uh oh!

KoBeWi commented Sep 23, 2022

Uh oh!

adamscott commented Sep 24, 2022

Uh oh!

KoBeWi commented Sep 24, 2022 •

edited

Loading

Uh oh!

bitbrain commented Oct 4, 2022

Uh oh!

h0lley commented Oct 4, 2022 •

edited

Loading

Uh oh!

bitbrain commented Oct 6, 2022

Uh oh!

bitbrain Oct 6, 2022

Uh oh!

adamscott Oct 7, 2022 •

edited

Loading

Uh oh!

adamscott Oct 8, 2022 •

edited

Loading

Uh oh!

adamscott commented Oct 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

adamscott commented Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TL;DR

Summary

What it tries to solve

Standard case

Current cyclic reference issue

Proposed solution

Weak references

Dependency chart

List of changes:

Known issues

Code example

Minimal reproduction project:

Fixes

Notes

Uh oh!

KoBeWi commented Sep 23, 2022

Uh oh!

adamscott commented Sep 24, 2022

Uh oh!

KoBeWi commented Sep 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bitbrain commented Oct 4, 2022

Uh oh!

h0lley commented Oct 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bitbrain commented Oct 6, 2022

Uh oh!

bitbrain Oct 6, 2022

Choose a reason for hiding this comment

Uh oh!

adamscott Oct 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamscott Oct 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamscott commented Oct 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

adamscott commented Sep 13, 2022 •

edited

Loading

KoBeWi commented Sep 24, 2022 •

edited

Loading

h0lley commented Oct 4, 2022 •

edited

Loading

adamscott Oct 7, 2022 •

edited

Loading

adamscott Oct 8, 2022 •

edited

Loading