Skip to content
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

Re-enable docs cache with fixes #78615

Merged
merged 1 commit into from
Jul 26, 2023

Conversation

RandomShaper
Copy link
Member

@RandomShaper RandomShaper commented Jun 23, 2023

This is built on top of #78614, to be tested after 4.1 is out.

  • No more custom resource type.
  • Fixed the API scope of EditorSettings.

Fixes #77878.
Fixes #78647.

@RandomShaper RandomShaper added this to the 4.2 milestone Jun 23, 2023
@RandomShaper RandomShaper marked this pull request as draft June 23, 2023 15:10
@RandomShaper RandomShaper force-pushed the fix_doc_cache branch 2 times, most recently from 51e0523 to 86ba005 Compare June 26, 2023 12:25
@RandomShaper RandomShaper changed the title Re-enable docs cache with more strict invalidation Re-enable docs cache with fixes Jun 26, 2023
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, this PR appears to break editor settings loading:

❯ Godot Engine v4.1.rc.custom_build.a629515b3 - https://godotengine.org
Vulkan API 1.3.242 - Forward+ - Using Vulkan Device #0: NVIDIA - NVIDIA GeForce RTX 4090
 
ERROR: Class 'EditorSettings' or its base class cannot be instantiated.
   at: instantiate (./core/object/class_db.cpp:341)
ERROR: Could not load editor settings from path: /home/hugo/.config/godot/editor_settings-4.tres
   at: create (./editor/editor_settings.cpp:929)

Trying to change a setting then restart the editor means the setting is ignored. Currently, this PR will also remove your existing editor settings, so beware.

@RandomShaper
Copy link
Member Author

Thanks for testing.

It seems that ClassDB::class_exists("EditorSettings") is always true, so I'm removing that check. There's no harm in redundant GDREGISTER_CLASS(), as far as I can tell.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, it works as expected. I've tried starting the editor several times and can always see the EditorSettings class reference.

Code looks good for me at a glance.

@YuriSizov
Copy link
Contributor

@RandomShaper So, do we want to merge it now? It's still marked as a draft.

@RandomShaper RandomShaper marked this pull request as ready for review July 26, 2023 11:36
@RandomShaper RandomShaper requested a review from a team as a code owner July 26, 2023 11:36
@RandomShaper
Copy link
Member Author

I forgot to unmark it. I've smoke-tested it just in case. Seems good to go.

@YuriSizov YuriSizov merged commit 92960b7 into godotengine:master Jul 26, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@RandomShaper RandomShaper deleted the fix_doc_cache branch July 26, 2023 17:32
@aaronfranke
Copy link
Member

aaronfranke commented Jul 26, 2023

As of this PR being merged (92960b7) I am getting an immediate crash when opening a project:

libc++abi: terminating due to uncaught exception of type std::__1::system_error: recursive_mutex lock failed: Invalid argument
Thread 0 Crashed::  Dispatch queue: com.apple.main-thread
0   libsystem_kernel.dylib        	       0x1842fc724 __pthread_kill + 8
1   libsystem_pthread.dylib       	       0x184333c28 pthread_kill + 288
2   libsystem_c.dylib             	       0x184241ae8 abort + 180
3   libc++abi.dylib               	       0x1842ecb84 abort_message + 132
4   libc++abi.dylib               	       0x1842dc3b4 demangling_terminate_handler() + 320
5   libobjc.A.dylib               	       0x183fb303c _objc_terminate() + 160
6   libc++abi.dylib               	       0x1842ebf48 std::__terminate(void (*)()) + 16
7   libc++abi.dylib               	       0x1842eed34 __cxxabiv1::failed_throw(__cxxabiv1::__cxa_exception*) + 36
8   libc++abi.dylib               	       0x1842eece0 __cxa_throw + 140
9   libc++.1.dylib                	       0x18426a8b4 std::__1::__throw_system_error(int, char const*) + 100
10  libc++.1.dylib                	       0x18425f11c std::__1::recursive_mutex::lock() + 40
11  godot.macos.editor.arm64      	       0x104ba1abc ResourceImporterTexture::update_imports() + 96
12  godot.macos.editor.arm64      	       0x104596958 EditorNode::_notification(int) + 2764
13  godot.macos.editor.arm64      	       0x1045fea98 0x102ce4000 + 26323608
14  godot.macos.editor.arm64      	       0x10721e178 Object::notification(int, bool) + 32
15  godot.macos.editor.arm64      	       0x10535d530 SceneTree::_process_group(SceneTree::ProcessGroup*, bool) + 280
16  godot.macos.editor.arm64      	       0x10535b398 SceneTree::_process(bool) + 676
17  godot.macos.editor.arm64      	       0x10535bafc SceneTree::process(double) + 240
18  godot.macos.editor.arm64      	       0x10309ae88 Main::iteration() + 952
19  godot.macos.editor.arm64      	       0x10304ca2c OS_MacOS::run() + 116
20  godot.macos.editor.arm64      	       0x103075db8 main + 304
21  dyld                          	       0x183fdbf28 start + 2236

The previous merge commit c4e5822 works fine.

EDIT: Actually this is weird. If I open the project in the latest master it crashes, then in 92960b7 it crashes, then in c4e5822 it works. But if I try to go forward in time again, 92960b7 works, and 7c20487 is the first commit that starts crashing.

@YuriSizov
Copy link
Contributor

@aaronfranke Make sure to remove editor cache or use the self-contained mode.

@aaronfranke
Copy link
Member

aaronfranke commented Jul 27, 2023

@YuriSizov Thanks, clearing the editor cache fixed it. I had previously tried clearing the project cache which did not work, but clearing the editor cache works. Also I'm not sure but I think you have to clear both the editor cache and the project cache at the same time, since I did have one crash occur after clearing the editor cache, but then I cleared the project cache again and it worked.

@aaronfranke
Copy link
Member

Well, the crash is back again, and now it keeps happening even after clearing all the caches. Maybe #79936.

@YuriSizov
Copy link
Contributor

Well that stack points to ResourceImporterTexture::update_imports() for some reason. Latest PRs to touch the texture importer don't seem to have anything to do with this, #68460 and #78456.

This is with debug symbols already? We can't get more information from the stack trace?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DocCache in the resource list EditorSettings doc is not present inside the editor when using doc cache
4 participants