Scripting: Fix script docs not being searchable without manually recompiling scripts #95821

anvilfolk · 2024-08-19T21:11:08Z

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 scripts~~ Done.

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 on loader_thread, and can include ResourceLoader::load() calls, orange cannot. Blue background represents waiting for one shot sources_updated signals depending on whether EditorFileSystem::is_scanning() is true.

Implementation details:

Doc tasks that don't use ResourceLoader::load() to generate docs always happens in EditorHelp::worker_thread.
Regenerating script docs using ResourceLoader::load() happen in EditorHelp::loader_thread. This prevents deadlocks where the main thread needs doc info and waits for worker_thread, but worker_thread is waiting for main thread to dispatch loading tasks.
Script cache should not reload when GDExtension docs change, overwriting doc changes of the current session ❗ someone with GDExtensions please test!❗ .
If cache isn't present, worker_thread connects to EditorFileSystem::filesystem_changed signal, indicating the end of EditorFileSystem's scan, and ends its execution. It spools back up once the signal is fired to regenerate the cache, this time using EditorFileSystemDirectory, therefore not accessing underlying OS filesystem calls.
Cache is saved & loaded as-is in memory or in the disk. We let EditorFileSystem catch changes like deleted/added files to ensure docs are kept up to date.
Cache only saves on exit, since it requires bulk-saving every script doc to disk as a Resource. That feels like too much to do every time a script is saved/compiled to keep disk & memory docs consistent.
Added DocTool method forwarding to EditorHelp 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().
Disk cache is deleted after successful load. It saves on editor exit, if successfully loaded/regenerated previously.
Somewhat unrelated but removed quotes around documentation pages in the script list, e.g. "new_script.gd" becomes new_script.gd, since this resulted in messy truncations of paths (to test, create new_folder/new_script.gd and new_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

editor/plugins/script_editor_plugin.cpp

editor/editor_help.cpp

dalexeev · 2024-08-23T07:46:03Z

Somewhat unrelated but removed quotes around documentation pages in the script list, e.g. "new_script.gd" becomes new_script.gd, since this resulted in messy truncations of paths (to test, create new_folder/new_script.gd and new_script.gd and open both their documentations on stable.

I think it's better to fix this by checking the tab type. Doc tabs should never truncate the name, only script tabs.

anvilfolk · 2024-08-23T18:26:25Z

Somewhat unrelated but removed quotes around documentation pages in the script list, e.g. "new_script.gd" becomes new_script.gd, since this resulted in messy truncations of paths (to test, create new_folder/new_script.gd and new_script.gd and open both their documentations on stable.

I think it's better to fix this by checking the tab type. Doc tabs should never truncate the name, only script tabs.

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?

editor/editor_help.cpp

anvilfolk · 2024-09-25T02:09:13Z

That project would help - I could at least inspect where the threads are and figure out what's causing the deadlock.

Turns out shaders are considered scripts for doc purposes and will have their documentation cached when they are compiled, e.g. when a scene with a shader is loaded in the editor. Might need to look into how to remove those docs if the shader is ever deleted.

editor/editor_help.h

anvilfolk · 2024-09-27T16:09:40Z

Latest update:

Changed _script_docs_loaded to SafeFlag
Removed race condition in reading/writing to docs_to_add/remove/etc by pulling setting _script_docs_loaded to before those are read. In that case, other threads will wait until this thread is finished to add those docs.

I noticed that filesystem_changed or sources_changed aren't always emitted. @Hilderin, I'm thinking it may be safer to emit a new first_scan_complete signal to make sure. What are your thoughts?

anvilfolk · 2024-09-27T20:14:05Z

New update:

Fixed reason for deadlock found by @KoBeWi, thanks for sharing the project so I could replicate it
Changed filesystem_changed which isn't always fired, to sources_changed which should always be fired.

The reason for the deadlock was that non-main threads like the EditorHelp worker thread will queue resource load tasks and wait for them to be dispatched (I assume) on the main thread and loaded from Godot's general worker threads. But, if the main thread waits on the EditorHelp worker thread by calling e.g. EditorHelp::get_doc_data() before dispatching the load tasks, then it deadlocks.

I added a verbose error message to catch when deadlocks might happen. There's a chance more of them happen in other projects with different load pathways that end up calling EditorHelp::get_doc_data(). When I get more time I could go around and replace usages of EditorHelp::get_doc_data() with the new helper methods :)

Also, force-reloading all the scripts from a worker thread in a larger project like @KoBeWi's has resulted in Invalid task ID. a few times @RandomShaper.

anvilfolk · 2024-09-27T21:27:43Z

I'm going to create a new worker thread just for the ResourceLoader::load() calls. It should prevent all deadlocks without having to change all existing calls to EditorHelp::get_doc_data() to use the new helper functions.

anvilfolk · 2024-09-28T13:41:02Z

Latest update:

Added a loader_thread to EditorHelp. It is used to make the ResourceLoader::load() calls, which were deadlocking when the main thread wait for the worker_thread. There should be no more deadlocking possible at all.
Changed EditorHelpBit and ScriptEditorPlugin to use new EditorHelp doc access methods. Can be replicated throughout the rest of the codebase where appropriate, but it's not necessary.
Will run more comprehensive tests when I get back to the right computer

magian1127 · 2024-09-29T11:11:45Z

EDIT2: I saved the cache as .tres and inspected it. It seems to also contain shaders?

It shows in search, but you can't open it !

@KoBeWi

This was caused by my previous PR, and I've now submitted a fix. #97616

anvilfolk · 2024-09-30T20:12:36Z

Testing is showing docs are not when changed when Godot is closed, or when changed from outside Godot with it open. Looking into it. Could be either because something changed in EditorFileSystem which stopped _update_scripts_documentation() from being called (maybe #95678? I'll try reverting to before that), or from my changing from filesystem_changed to sources_changed.

Or, you know, some other skill issue of mine 🤣

anvilfolk · 2024-10-07T19:35:12Z

editor/editor_help.cpp

+void EditorHelp::regenerate_script_doc_cache() {
+	if (EditorFileSystem::get_singleton()->is_scanning()) {
+		// Wait until EditorFileSystem scanning is complete to use updated filesystem structure.
+		EditorFileSystem::get_singleton()->connect(SNAME("sources_changed"), callable_mp_static(_regenerate_script_doc_cache), CONNECT_ONE_SHOT);


Changed to sources_changed from filesystem_changed. It looks like that might be a more reliable signal, since in some situations where no change has happened, filesystem_changed may not emit. I also like that OS::get_singleton()->benchmark_end_measure("Editor", "First Scan"); happens in EditorNode::_sources_changed, which is good evidence that it's reliable to detect the end of the first scan.

anvilfolk · 2024-10-07T19:36:15Z

editor/editor_help.cpp

+	ERR_FAIL_COND_MSG(!ProjectSettings::get_singleton()->is_project_loaded(), "Error: cannot load script doc cache without a project.");
+	ERR_FAIL_COND_MSG(!ResourceLoader::exists(get_script_doc_cache_full_path()), "Error: cannot load script doc cache from inexistent file.");
+
+	Ref<Resource> script_doc_cache_res = ResourceLoader::load(get_script_doc_cache_full_path(), "", ResourceFormatLoader::CACHE_MODE_IGNORE);


Added CACHE_MODE_IGNORE here since it feels silly to cache something we're deleting anyway :)

Perhaps this is worth doing for the native docs as well?

anvilfolk · 2024-10-07T19:47:31Z

Issue mentioned above fixed in Fix GDScript docs not updating when modified externally #97710
See comments above for latest changes: changed signal used and added CACHE_MODE_IGNORE when loading script doc cache.

Ran the following tests.

With Godot open:

	Add script	Modify script	Remove script
Internal script editor	✅	✅	✅
External script editor	✅	✅	✅

With Godot closed:

	Regenerate docs	Added script	Modified script	Removed script
Empty project, no extra scripts	✅	✅	✅	✅
Empty project, extra scripts	✅	✅	✅	✅
Complex project, no extra scripts	✅ ⚠️	✅	✅	✅
Complex project, extra scripts	✅⚠️	✅	✅	✅

The extra scripts are 4096 extra scripts that exist just to make script regen take longer and hopefully bring concurrency issues to light. None did.

Notes:

The ⚠️ warnings ⚠️ above refer to getting the occasional Invalid task ID error, presumably because loader_thread is loading gdscript files that may be used & being loaded elsewhere. It is more of a concurrent resource loading problem than something to do with this PR, I think.
I also ran into an issue where Godot would straight up close repeatedly near startup with the complex project, but never within VS. It stopped happening, and I couldn't get a log or replicate at all after a while. I think one time before Godot exited, the docs hadn't loaded, so there might be an edge case here somewhere.
I'm also getting ObjectDB leaked at exit, but I don't really see how that might be from this. Unless it's something to do with saving a resource on exit? I'm not sure.

Again, bit thanks to @KoBeWi who provided a larger project to bugfix & test on.

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 <[email protected]>

RandomShaper · 2024-11-05T15:50:10Z

I've been trying to diagnose the two issues related to ResourceLoader:

Deadlock if not using a thread to start the load.
Invalid task id error.

But I don't know how to reproduce them. I know no. 1 has been worked around, but I'd still like to see the deadlock happening in front of me with the version of the code prior to the workaround that I can retrieve from GitHub.

Also, regarding no. 1, could you tell me what happens if instead of using the blocking ResourceLoader::load(), you use ResourceLoader::load_threaded_request() with p_use_sub_threads = true?

anvilfolk · 2024-11-05T16:28:38Z

Thank you for following up! <3 I'll try to find some time in the coming days to get back to a previous commit and try your suggestion! :)

If @KoBeWi feels good about sharing the project with you, you should be able to replicate 2. by opening it when the script docs cache isn't present (because it was never generated it or you deleted it).

Calinou added enhancement topic:gdscript topic:editor performance labels Aug 19, 2024

Calinou added this to the 4.x milestone Aug 19, 2024

anvilfolk force-pushed the cache-gd-docs branch 8 times, most recently from 58e4ff3 to 7fde3a4 Compare August 21, 2024 21:07

anvilfolk commented Aug 22, 2024

View reviewed changes

editor/plugins/script_editor_plugin.cpp Show resolved Hide resolved

anvilfolk mentioned this pull request Aug 22, 2024

The documentation for a class defined by the keyword 'class_name' does not appear when searching using the '(doc) Search Help' #86577

Open

anvilfolk force-pushed the cache-gd-docs branch from 7fde3a4 to 88966ec Compare August 22, 2024 03:39

anvilfolk commented Aug 22, 2024

View reviewed changes

editor/editor_help.cpp Outdated Show resolved Hide resolved

anvilfolk force-pushed the cache-gd-docs branch 5 times, most recently from 59d1345 to 53cc97c Compare August 22, 2024 23:03

Hilderin mentioned this pull request Aug 22, 2024

Update script documentation removed files on startup #95965

Merged

anvilfolk force-pushed the cache-gd-docs branch from 53cc97c to eff26b9 Compare August 23, 2024 13:27

anvilfolk marked this pull request as ready for review August 23, 2024 18:31

anvilfolk requested a review from a team as a code owner August 23, 2024 18:31

anvilfolk changed the title ~~Scripting: Add script documentation cache to project~~ Scripting: fix script docs not being searchable without manually recompiling scripts Aug 23, 2024

anvilfolk commented Aug 23, 2024

View reviewed changes

editor/editor_help.cpp Outdated Show resolved Hide resolved

anvilfolk force-pushed the cache-gd-docs branch from 873d775 to 67c22e6 Compare September 25, 2024 02:04

dalexeev mentioned this pull request Sep 25, 2024

Custom tooltips for GDS exports are ineffective unless the script is saved again #97422

Closed

magian1127 mentioned this pull request Sep 26, 2024

C#: Implement tooltips for Signals and Properties in the inspector. #83505

Open

huwpascoe reviewed Sep 27, 2024

View reviewed changes

editor/editor_help.h Outdated Show resolved Hide resolved

anvilfolk force-pushed the cache-gd-docs branch from 67c22e6 to 70edfc4 Compare September 27, 2024 16:07

anvilfolk requested a review from a team as a code owner September 27, 2024 16:07

anvilfolk force-pushed the cache-gd-docs branch from 70edfc4 to 65f33aa Compare September 27, 2024 20:08

anvilfolk force-pushed the cache-gd-docs branch from 65f33aa to 0caaf73 Compare September 28, 2024 13:27

RandomShaper self-requested a review September 28, 2024 21:29

anvilfolk mentioned this pull request Sep 29, 2024

Optimize the custom doc for shaders. #97616

Open

anvilfolk force-pushed the cache-gd-docs branch from 0caaf73 to 3ebb4d3 Compare October 7, 2024 19:22

anvilfolk commented Oct 7, 2024

View reviewed changes

Hilderin mentioned this pull request Oct 17, 2024

Remove unnecessary reload_from_file when updating documentation. #98269

Draft

anvilfolk force-pushed the cache-gd-docs branch from 3ebb4d3 to 3af34bf Compare October 22, 2024 15:58

anvilfolk requested review from a team as code owners October 22, 2024 15:58

dalexeev mentioned this pull request Oct 28, 2024

Documentation comments don't appear in tooltips before the script is edited and saved #98592

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scripting: Fix script docs not being searchable without manually recompiling scripts #95821

Scripting: Fix script docs not being searchable without manually recompiling scripts #95821

anvilfolk commented Aug 19, 2024 •

edited

Loading

dalexeev commented Aug 23, 2024

anvilfolk commented Aug 23, 2024

anvilfolk commented Sep 25, 2024

anvilfolk commented Sep 27, 2024

anvilfolk commented Sep 27, 2024 •

edited

Loading

anvilfolk commented Sep 27, 2024

anvilfolk commented Sep 28, 2024

magian1127 commented Sep 29, 2024

anvilfolk commented Sep 30, 2024 •

edited

Loading

anvilfolk Oct 7, 2024

anvilfolk Oct 7, 2024

anvilfolk commented Oct 7, 2024 •

edited

Loading

RandomShaper commented Nov 5, 2024

anvilfolk commented Nov 5, 2024

Scripting: Fix script docs not being searchable without manually recompiling scripts #95821

Are you sure you want to change the base?

Scripting: Fix script docs not being searchable without manually recompiling scripts #95821

Conversation

anvilfolk commented Aug 19, 2024 • edited Loading

dalexeev commented Aug 23, 2024

anvilfolk commented Aug 23, 2024

anvilfolk commented Sep 25, 2024

anvilfolk commented Sep 27, 2024

anvilfolk commented Sep 27, 2024 • edited Loading

anvilfolk commented Sep 27, 2024

anvilfolk commented Sep 28, 2024

magian1127 commented Sep 29, 2024

anvilfolk commented Sep 30, 2024 • edited Loading

anvilfolk Oct 7, 2024

Choose a reason for hiding this comment

anvilfolk Oct 7, 2024

Choose a reason for hiding this comment

anvilfolk commented Oct 7, 2024 • edited Loading

RandomShaper commented Nov 5, 2024

anvilfolk commented Nov 5, 2024

anvilfolk commented Aug 19, 2024 •

edited

Loading

anvilfolk commented Sep 27, 2024 •

edited

Loading

anvilfolk commented Sep 30, 2024 •

edited

Loading

anvilfolk commented Oct 7, 2024 •

edited

Loading