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? |
75e48c7 to
4c59be3
Compare
|
Only did some basic tests but don't think the rebase broke anything. |
dalexeev
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Can multiple ClassDocs have the same script_path? For example inner classes.
There was a problem hiding this comment.
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.
Can multiple
ClassDocs 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_updatedsignals 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_threadis waiting for main thread to dispatch loading tasks.worker_threadconnects toEditorFileSystem::filesystem_changedsignal, 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.EditorFileSystemcatch changes like deleted/added files to ensure docs are kept up to date.DocToolmethod forwarding toEditorHelpto 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.gdandnew_script.gdand 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