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

Fix editor needs restart after adding GDExtensions #93972

Conversation

Hilderin
Copy link
Contributor

@Hilderin Hilderin commented Jul 5, 2024

Fixes #77478

This PR should prevent the need to restart the editor when a new GDExtension is installed or when starting the editor for the first time without the extension_list.cfg file.

Modifications:

  • Added a scan for new or invalid extensions before the first scan.
  • Added registering and unregistering for extension classes in GDScript globals so scripts can work without restarting the editor.
  • Moved the addition and removal of extensions and the writing of extension_list.cfg to GDExtensionManager::ensure_extensions_loaded to facilitate the emission of the extensions_reloaded signal and to call _reload_all_scripts when extensions are added or removed.
  • Added ScriptEditor::get_singleton()->reload_scripts on extensions_reloaded in EditorNode to revalidate the opened scripts and fix GDScript errors when extensions are added while the editor is running and GDScripts are opened that use classes from the extension.

Known issues:

  • When an extension is removed (removing the .gdextension file) and a scene containing nodes from this extension is opened, the editor crashes. This issue was already present. I'm wondering how to fix this, maybe by reloading the scenes when adding or removing extensions? I could be problematic for the user to always reload scene when reloading extensions??
  • The editor crashes when using a class from an extension as the base class for an autoload script with @tool. This issue was already present.

Not tested:

  • Extensions with custom resource importers or types.
  • Plugin extensions.
  • Extension with HotReload.

MRP used to test this (only built for Windows):

It's based on the MRP from #77478 but modified for Godot 4.3 master.
test-godot-minimal_repro.zip

@Hilderin Hilderin requested review from a team as code owners July 5, 2024 14:52
@Hilderin Hilderin force-pushed the fix-editor-needs-restart-after-adding-gdextensions branch from 13652f3 to 875f898 Compare July 5, 2024 14:54
@AThousandShips AThousandShips added this to the 4.4 milestone Jul 5, 2024
@Hilderin Hilderin force-pushed the fix-editor-needs-restart-after-adding-gdextensions branch 2 times, most recently from 965b848 to 1c0cb31 Compare July 5, 2024 15:57
@Hilderin Hilderin requested a review from a team as a code owner July 5, 2024 15:57
@Hilderin Hilderin force-pushed the fix-editor-needs-restart-after-adding-gdextensions branch from 1c0cb31 to 10a13dc Compare July 5, 2024 16:05
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

As far as documentation for the two new signals, it's fine.

@Hilderin Hilderin force-pushed the fix-editor-needs-restart-after-adding-gdextensions branch from 10a13dc to b01beba Compare July 6, 2024 11:15
@Hilderin Hilderin force-pushed the fix-editor-needs-restart-after-adding-gdextensions branch from b01beba to 6ced7c1 Compare July 9, 2024 14:41
@Hilderin
Copy link
Contributor Author

@dsnopek
Following your message in #92667, I was not able to reproduce the problem. Do you have an MRP or a procedure to reproduce?

@dsnopek
Copy link
Contributor

dsnopek commented Jul 12, 2024

Sure! I'm testing with https://github.com/GodotVR/godot_openxr_vendors using the project in demo/.

If I delete the .godot directory and then open the project with an editor built with this PR, I still get these errors in the console:

ERROR: Cannot get class 'OpenXRFbPassthroughExtensionWrapper'.
   at: set_object_extension_instance (core/object/class_db.cpp:639)
ERROR: Cannot get class 'OpenXRFbRenderModelExtensionWrapper'.
   at: set_object_extension_instance (core/object/class_db.cpp:639)
ERROR: Cannot get class 'OpenXRFbSceneCaptureExtensionWrapper'.
   at: set_object_extension_instance (core/object/class_db.cpp:639)
ERROR: Cannot get class 'OpenXRFbSpatialEntityExtensionWrapper'.
   at: set_object_extension_instance (core/object/class_db.cpp:639)
ERROR: Cannot get class 'OpenXRFbSpatialEntityQueryExtensionWrapper'.
   at: set_object_extension_instance (core/object/class_db.cpp:639)
ERROR: Cannot get class 'OpenXRFbSpatialEntityContainerExtensionWrapper'.
   at: set_object_extension_instance (core/object/class_db.cpp:639)
ERROR: Cannot get class 'OpenXRFbSceneExtensionWrapper'.
   at: set_object_extension_instance (core/object/class_db.cpp:639)
ERROR: Cannot get class 'OpenXRFbHandTrackingAimExtensionWrapper'.
   at: set_object_extension_instance (core/object/class_db.cpp:639)
ERROR: Cannot get class 'OpenXRFbHandTrackingCapsulesExtensionWrapper'.
   at: set_object_extension_instance (core/object/class_db.cpp:639)
ERROR: Cannot get class 'OpenXRFbCompositionLayerSettingsExtensionWrapper'.
   at: set_object_extension_instance (core/object/class_db.cpp:639)
ERROR: Cannot get class 'OpenXRHtcFacialTrackingExtensionWrapper'.
   at: set_object_extension_instance (core/object/class_db.cpp:639)
ERROR: Cannot get class 'OpenXRHtcPassthroughExtensionWrapper'.
   at: set_object_extension_instance (core/object/class_db.cpp:639)
ERROR: GDScript bug: Native class "OpenXRFbRenderModel" not found.
   at: _gdtype_from_datatype (modules/gdscript/gdscript_compiler.cpp:123)
ERROR: GDScript bug: Native class "OpenXRFbRenderModel" not found.
   at: _gdtype_from_datatype (modules/gdscript/gdscript_compiler.cpp:123)
ERROR: GDScript bug: Native class "OpenXRFbRenderModel" not found.
   at: _gdtype_from_datatype (modules/gdscript/gdscript_compiler.cpp:123)
ERROR: GDScript bug: Native class "OpenXRFbRenderModel" not found.
   at: _gdtype_from_datatype (modules/gdscript/gdscript_compiler.cpp:123)
ERROR: GDScript bug: Native class "OpenXRFbRenderModel" not found.
   at: _gdtype_from_datatype (modules/gdscript/gdscript_compiler.cpp:123)
ERROR: GDScript bug: Native class "OpenXRFbRenderModel" not found.
   at: _gdtype_from_datatype (modules/gdscript/gdscript_compiler.cpp:123)
ERROR: GDScript bug: Native class "OpenXRFbRenderModel" not found.
   at: _gdtype_from_datatype (modules/gdscript/gdscript_compiler.cpp:123)
ERROR: Cannot get class 'OpenXRFbHandTrackingMeshExtensionWrapper'.
   at: set_object_extension_instance (core/object/class_db.cpp:639)
ERROR: In Object of type 'OpenXRExtensionWrapperExtension': Attempt to connect nonexistent signal 'openxr_fb_hand_tracking_mesh_data_fetched' to callable '<CallableCustom>'.
   at: connect (core/object/object.cpp:1423)
ERROR: In Object of type 'OpenXRExtensionWrapperExtension': Attempt to connect nonexistent signal 'openxr_fb_hand_tracking_mesh_data_fetched' to callable '<CallableCustom>'.
   at: connect (core/object/object.cpp:1423)
ERROR: In Object of type 'OpenXRExtensionWrapperExtension': Attempt to connect nonexistent signal 'openxr_fb_hand_tracking_mesh_data_fetched' to callable '<CallableCustom>'.
   at: connect (core/object/object.cpp:1423)

These errors aren't present when I open the Godot editor every subsequent time, after the .godot/ directory is populated.

@Hilderin Hilderin force-pushed the fix-editor-needs-restart-after-adding-gdextensions branch from 6ced7c1 to d97b0c6 Compare July 15, 2024 13:52
@Hilderin
Copy link
Contributor Author

I fixed this issue. It was caused because the GDExtensionManager::initialize_library was never called at level GDEXTENSION_INITIALIZATION_CORE and GDEXTENSION_INITIALIZATION_SERVERS for new extensions in the first scan process. I added a parameter to GDExtensionManager::_load_extension_internal to manage this situation.

@DanielKinsman
Copy link
Contributor

This isn't just about a good editor experience, it apparently fixes #92833 which stops people from being able to implement headless CI builds. Thanks for the fix!

Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks!

With the latest version of the PR, I'm not seeing the issue I was seeing previously. Skimming the code, the GDExtension parts look good to me - I'm not qualified to review the GDScript parts.

Anyway, this is a really great improvement! I'm really excited for this to be merged once Godot 4.4 development opens up :-)

@dalexeev
Copy link
Member

I haven't reviewed this in detail, but the GDScript part looks good to me. However, it may not be enough to make tool scripts and hot reloading work correctly. The GDScript implementation relies in multiple places on the assumption that GDExtension classes are indistinguishable from native classes, i.e. they can't be removed/changed. We probably need to invalidate some scripts when unloading GDExtensions and/or do some other work similar to rune-scape's fixes.

However, I don't think this should be treated as a blocker for merging this PR if everything else is fine. GDExtensions can be used independently of tool / hot reloading scripts or may not be affected by the above issues. Just pointing out potential problems that can be addressed in future PR(s).

@Hilderin Hilderin force-pushed the fix-editor-needs-restart-after-adding-gdextensions branch 2 times, most recently from 11fcfb8 to ebbbae4 Compare August 29, 2024 10:14
@Hilderin Hilderin force-pushed the fix-editor-needs-restart-after-adding-gdextensions branch from ebbbae4 to ef6f873 Compare August 29, 2024 10:15
@Hilderin
Copy link
Contributor Author

Rebased to fix the merge conflict and adjusted to documentation for extension_loaded and extension_unloading signals. I moved the following lines that were added in 4dd6e8e into GDExtensionManager::ensure_extensions_loaded:

// The extension may not have a .gdextension file.
if (!FileAccess::exists(loaded_extensions[i])) {

@akien-mga akien-mga merged commit f0ee0bd into godotengine:master Aug 30, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks! Awesome improvement 🎉

@ryevdokimov
Copy link
Contributor

ryevdokimov commented Aug 30, 2024

I'm trying this with Godot Jolt and it mostly works, sometimes the editor crashes, but when it does load, I get this error.

My understanding is the Godot Jolt doesn't include the pdb files, and I'm not sure why it needs to load them.

image

@dsnopek
Copy link
Contributor

dsnopek commented Aug 30, 2024

@ryevdokimov I think the error you posted is unrelated, but does represent something that could be improved! Can you open a new issue for that? It sounds like we're detecting from the DLL that it has a PDB file (could have been built in Godot Jolts build process) but not finding the file on disk (probably just not included with the release files). We should probably just silently skip copying the PDB in that case.

With regard to your editor crashes: Do they only happen with this PR?

@mihe
Copy link
Contributor

mihe commented Aug 30, 2024

It sounds like we're detecting from the DLL that it has a PDB file (could have been built in Godot Jolts build process) but not finding the file on disk (probably just not included with the release files). We should probably just silently skip copying the PDB in that case.

I can confirm that this is the case. I do build with debug symbols, but I distribute them separately as part of the GitHub release for whomever wants/needs them, as opposed to shipping them with the binaries. This is fairly common on Windows in general, but maybe not so much with GDExtension.

This particular error seems to have slipped past me because it's typically only emitted to stderr, since EditorLog isn't normally up and running when extensions are loaded, but I guess that isn't necessarily the case any longer as of this PR.

Anyway, the PDB renaming introduced in #87117 should indeed not assume that the PDB file is present on disk even if there's an entry for it in the header.

@ryevdokimov
Copy link
Contributor

ryevdokimov commented Aug 30, 2024

Yeah, I suppose this is more an exposed issue as a result of a PR rather than a direct consequence of it. Prior to this PR, it wouldn't get to this step because the editor needed to be restarted. Just wanted to confirm. I can report this as a new issue using the feedback from mihe above.

@TokisanGames
Copy link
Contributor

@ryevdokimov If interested, you could also test with Terrain3D, which is downloadable from the asset library. I don't distribute debug builds, but do provide icons for our editor buttons. Typically they need a restart so Godot will properly import them. So our normal instructions are to restart twice, once for the library, once for imported graphics.

@ryevdokimov
Copy link
Contributor

@TokisanGames,

The library loads fine however as probably expected we get errors for the missing graphics:

image

I believe this PR would fix this: #92667

@Hilderin
Copy link
Contributor Author

I believe this PR would fix this: #92667

I believe so, yes.

@fire
Copy link
Member

fire commented Aug 31, 2024

There’s interest in chat.godotengine.org for cherry picking this to 4.3

@dsnopek
Copy link
Contributor

dsnopek commented Aug 31, 2024

I think we should wait to see if there's any issues after using it in a couple dev releases before cherry-picking it. But, yeah, if there are no issues, I know lots of folks would love this change. :-)

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.

global_script_class_cache.cfg doesn't get properly generated if extension_list.cfg doesn't exist on first load