-
-
Notifications
You must be signed in to change notification settings - Fork 22k
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
Scripting: Fix script docs not being searchable without manually recompiling scripts #95821
Conversation
58e4ff3
to
7fde3a4
Compare
7fde3a4
to
88966ec
Compare
59d1345
to
53cc97c
Compare
I think it's better to fix this by checking the tab type. Doc tabs should never truncate the name, only script tabs. |
53cc97c
to
eff26b9
Compare
Why the difference? You can still get the full path by hovering and it's at the top of the help document. This way it feels like behavior is at least consistent? |
Needs another rebase. |
3af34bf
to
75e48c7
Compare
75e48c7
to
4c59be3
Compare
Only did some basic tests but don't think the rebase broke anything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not familiar with this area, but overall the code looks good to me and I believe that anvilfolk and Hilderin have done a good job!
I have tested these changes on several small-medium projects and have not noticed any major issues. The only thing is that the editor startup time without cache has increased noticeably. But this is quite expected, probably caused by the fact that now all project scripts are compiled. Restarting the editor takes much less time.
We could probably avoid full analysis and compilation of all scripts, for full project documentation only the interface resolution is required, function body analysis and script compilation are not really needed. But this is problematic to do, since GDScriptCache
is a rather complicated system and we are forced to compile all scripts to get full documentation.
I also noticed a phantom analyzer error with a circular dependency. But it immediately disappeared when I opened the dependent script and I'm not sure if it's related to this PR, maybe it's caused by the missing editor cache.
So personally I would highly support merging this PR for 4.4. It would be a significant QoL improvement and would fit well with the new documentation tooltips feature. But of course it has concerns about edge cases and editor performance regressions for large projects.
It would probably be better to merge this early into 4.5 dev for more testing. But I think it also makes sense to test it in 4.4 beta. In the worst case, we can revert it before release and postpone to 4.5 if we find major issues.
for (KeyValue<String, DocData::ClassDoc> &E : class_list) { | ||
if (E.value.is_script_doc && E.value.script_path == p_path) { | ||
remove_doc(E.key); | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can multiple ClassDoc
s have the same script_path
? For example inner classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into it!
We could probably avoid full analysis and compilation of all scripts.
Yes! There are a lot of places where we are usingResourceLoader::load(some_script_file)
, which full compiles something, in order to get documentation. This would be a wonderful thing to optimize in the future, but there's definitely a ton of technical challenges to overcome there!
The circular dependency is a little worrying for sure. I'm at a loss as to how it may be happening though, since the loading thread just loads scripts one by one, so circular dependencies should be handled as they normally are 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can multiple
ClassDoc
s have the samescript_path
? For example inner classes.
I'll need to double-check, good catch! I believe fqcn's have a separator from the path for inner classes but I'm not sure what their script_path
is.
Tested with a complex project. The docs weren't loaded during initial launch, I had to open and re-save the scripts. This also caused error prints (something about missing doc data, I didn't copy it). Afterwards the docs work correctly, including between editor sessions. This only happens if the project was already open and imported before. Opening without |
The error print is expected - ideally the cache file is always there, so if it isn't found we alert the user. But it should make all script docs available on first run after a while. All this does is force-load the scripts if the cache isn't found. It may take a while, since we wait until all scripts are loaded to copy docs into I think further work that's worth considering is a force-regenerate cache button (in case the cache is inconsistent), and a project option for whether you want to auto-generate it on startup, to avoid slow load times if e.g. Godot crashes regularly for you. |
We're reaching feature freeze for 4.4 now, so I'll punt this to 4.5, but it should be ready to merge early on for 4.5-dev1. This could have technically made it in 4.4 but since it was approved a bit late in the cycle, I'm not confident we have enough time to really battle test this before stable, especially on very big projects where I'm concerned about some of the performance implications. On the flip side, folks will have another reason to wait for Godot mere days after 4.4-stable is released :P |
Toddler is on/off sick so it's been hard to find time to follow up on issue reports here, so I think that's a good call too. If I don't get to figure out what's happening, we can always iterate in 4.5 dev, and work on adding project options, etc. |
Needs a rebase before this can be merged! |
Oops, misclick. I rebased locally a while back, will try to get this done soon! <3 |
This PR adds a script documentation cache in the project folder. It is loaded at alongside native documentation caches. This makes scripts fully accessible through Search Help, including their members, etc, right from project start, without having to compile every single script. Co-authored-by: Hilderin <81109165+Hilderin@users.noreply.github.com>
4c59be3
to
72045c8
Compare
Rebased, did some basic tests that all looked OK! Some quick reminders:
In the future we should add options to:
|
Thanks! |
This PR adds a script documentation cache in the project folder. It is loaded at alongside native documentation caches. This makes scripts fully accessible through Search Help, including their members, etc, right from project start, without having to compile every single script manually to access its docs.
Testing with GDScript but, in theory, should work for any scripting language that has documentation support. Might be worth splitting the cache into a per-language basis to reduce conflicts, though if there are conflicts with e.g. a MyClass in GDScript and MyClass in C#, that may already lead to problems with the current documentation system.
Ensuring file deletions when Godot closed trigger EditorFileSystem's documentation updates is done by #95965, until then the cache will persist docs from deleted scriptsDone.Diagram of call behavior that I needed to try to organize this in my brain:
Grey background runs on whatever thread called it. Orange background runs in
worker_thread
. Green background runs onloader_thread
, and can includeResourceLoader::load()
calls, orange cannot. Blue background represents waiting for one shotsources_updated
signals depending on whetherEditorFileSystem::is_scanning()
is true.Implementation details:
ResourceLoader::load()
to generate docs always happens inEditorHelp::worker_thread
.ResourceLoader::load()
happen inEditorHelp::loader_thread
. This prevents deadlocks where the main thread needs doc info and waits forworker_thread
, butworker_thread
is waiting for main thread to dispatch loading tasks.worker_thread
connects toEditorFileSystem::filesystem_changed
signal, indicating the end ofEditorFileSystem
's scan, and ends its execution. It spools back up once the signal is fired to regenerate the cache, this time usingEditorFileSystemDirectory
, therefore not accessing underlying OS filesystem calls.EditorFileSystem
catch changes like deleted/added files to ensure docs are kept up to date.DocTool
method forwarding toEditorHelp
to help maintain cache consistency. It is meant to track whether changes affect script docs or other docs. These forwarded methods should be used from now on instead of e.g.get_doc_data()->add_doc()
."new_script.gd"
becomesnew_script.gd
, since this resulted in messy truncations of paths (to test, createnew_folder/new_script.gd
andnew_script.gd
and open both their documentations on stable.Fixes #72406, fixes #86577, fixes #72952 (for GDScript, C# docs are not supported yet).
Added coauthorship with @Hilderin because of their wonderful and insightful help in designing and testing this <3