Fix cyclic references in GDScript 2.0#67714
Conversation
|
Please note: cyclic references ( |
1409d9d to
f28d2ab
Compare
I renamed the PR to "Add support for cyclic references in GDScript 2.0". |
38bc7bd to
268bd1c
Compare
There was a problem hiding this comment.
I added the .notest.gd exception here, as it was defined elsewhere, but not used. Practical for .gd files that are loaded by tests, but not tests themselves.
3739d03 to
e2a35ba
Compare
|
Renamed the PR to "Fix cyclic references in GDScript 2.0", as this fixes a bug (missing cyclic references support). It is not a "new feature" per se. |
f19f994 to
74859af
Compare
|
I fixed #61386 in this PR because I used code that I added in this one, but I could separate the fix into another PR, if needed. |
There was a problem hiding this comment.
Here's the fix for #61386 (the whole else if).
74859af to
2e39524
Compare
|
I just tried beta 6. This isn't quite working. Player and Enemy both inherit Unit. Many classes including Enemy reference In previous versions we had CRC errors in Enemy when Player was typed as shown above, and we referenced Now in Beta 6, I get the following error in the console upon loading the editor or the game, and there's no reference at all to the GDScript location. The editor does load scenes, but the game does not run. Upon running a scene, it breaks, and in the editor I'm presented with no info: It does not break on any gdscript. The Someone else has a similar issue, perhaps also caused by a CRC error they don't know about. #69213. If I can make an MRP, I'll make an issue or contribute to that one. However hopefully the cpp line and error will highlight the bug. |
|
@tinmanjuggernaut Please create an issue and a MRP if possible. The @rune-scape Does it ring a bell to you? |
|
#69259 fixed my errors above using cyclic references. Thanks @adamscott . |

TL;DR
This PR makes possible to use cyclic references in GDScript 2.0, i.e. preload class (or use by class name)
AinBandBinA, preload cyclic scenes, use typing in addons.Summary
This PR tweaks the engine to accept and support cyclic references.
What it tries to solve
The current GDScript engine blocks cyclic references as this would normally lead to memory leaks. Why? Because cyclic references artificially increase the refcount of gdscripts.
Standard case
Let's say
MainloadsAthat loadsB.Here,
Mainreference count is 1 (referenced by the engine),Ais 1 (referenced byMain) andBis 1 (referenced byA).When the engine unloads,
Mainreference count will be decremented by 1 and will be deleted, because its reference count will hit 0.AandBwill also be deleted becauseAlost his reference fromMainandBwill lose its reference fromAlater.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
MainloadsAthat loadsBthat loads backA.Here,
Mainreference count is 1 (referenced by the engine),Ais 2 (referenced byMainandB) andBis 1 (referenced byA).Do you see the issue? When
Mainwill be deleted, there will be no way to deleteAandB.Awill lose its reference toMainand will decrement, but it will not be deleted asAreference count is now 1.Bis not deleted because it didn't lose its reference toA.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
Cyclic dependency detection
There is no way to use the reference count as a mean to know if a cyclic
Ref<RefCounted>needs to be disposed of. IfAandBrefers to themselves a number of times, the reference count will skyrocket and it would not be easy to track down the exact number of times the count has been incremented.If, someday or someone, comes up with a reliable way to detect these, it would be the ideal solution.
Meanwhile, to fix this issue, I propose to use the existing
SelfList<GDScript> script_listfound in theGDScriptclass. This list holds eachGDScriptinstance.Take this table as basis for my following example:
MainA,B,XABCCD,EDEFFEXYYF,ZZOn the left column, each script contained in
script_listand on the right one, each script referenced, contained in its constants (script constants, function constants, ...).In the example, if
Bis disposed,CandDwill be too, but notEandF. How to make sure they are disposed correctly? And how to make sure to not dispose any script that can be legitimately used by another one?When we dispose of
B, we can check the dependencies of its dependencies. After all, there's a good chance that those will be disposed of withB. So,Bwill check itsget_must_clear_dependencies()."Must clear" dependencies
GDScript::get_must_clear_dependencies()is called inGDScript::clear()to know which dependencies must be forcibly cleared.The process may seem complicated, but it's quite simple. Let's say
Mainrunsget_must_clear_dependencies().With this, we know for a fact that each dependency in the "must_clear" set can be deleted without impacting the rest of the scripts, even if their reference count is high. That means that it's high only because of cyclic references.
By clearing each of those scripts (
GDScript::clear()clears all the constants holding scripts), their reference count will necessarily drop to zero, so they will be disposed without any need of hacks.Example
Must clear dependencies of
MainMust clear dependencies of
Mainis quite simple. It's everyGDScriptbecause every script comes fromMain.Step 1
The direct dependencies of
MainareA,BandX, but as it includes all the dependencies of the dependencies, it includes every class in the diagram.Step 2
AMainBMainCBDCEC,FFE,YXMainYXZYStep 3
As every class is a dependency of
Main, "can't clear" set is empty.Step 4
As "can't clear" is empty, the dependencies stay the same.
Step 5
It returns every classes.
Must clear dependencies of
BThis is where
must_clear_dependenciesshine, because it doesn't clear everything this time. Based on the algorithm, onlyCandDcan be cleared without any consequences, asEandFare used byY.Step 1
BC,D,E,FThe direct dependency of
BisC, but as it includes all the dependencies of the dependencies, it includesD,EandF.Step 2
CBDCEC,FFE,YStep 3
Yis outside ofB(itself),C,D,EandF. So, we addF,EandYto the "can't clear" set.Step 4
We remove
FandEfrom the dependencies.Step 5
It returns
CandD.Backup
If, for some reason, a script was able to slip out of the process, it still has a reference to
script_list. Then, we can callclear()in the loop found inGDScriptLanguage::~GDScriptLanguage()for script instances still left. This adds the ability to force the clear of internal variables of a script, which can trigger the reference count decrement of other scripts.Known issues
Code completion isn't working for cyclic referencesThe code works, but the cleaning isn't optimal. I found a reliable way to calculate cyclic references (seeGDScript::get_cyclic_references_count()), but my efforts to clear dynamically the script failed. It seems theunreference()function is somewhat unreliable.GDScript::clear()andGDScript::get_must_clear_dependencies();Shape2Dreferences, even ifPhysicsServer2Dsingleton was destructed, which leads to memory leaks.Shape3DCode 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.:Minimal reproduction project:
cyclic3.zip
Fixes
Fixes #21461 - Some usages of
class_namecan produce cyclic errors unexpectedlyFixes #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
Fixes #65263 - GDScript reload generates error when created dynamically (no backing script)
Fixes #68211 - Can't preload a scene in autoload if that scene uses the autoload
Does not fix (yet)
#61386 - [4.x regression] AutoLoad scenes (not scripts) do not support implicit types
Notes
This PR follows up #65752.