Reloading Resources, especially Scripts, needs improvement #5744
awardell
started this conversation in
Engine Core
Replies: 0 comments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
Presently there are a number of bugs related to the reloading of project resources, such as godotengine/godot#30302 and godotengine/godot#59115. There are pull-requests to address some of these issues, but none of them affect the root of the matter.
The current system for detecting and acting on resource changes is this:
EditorFileSystem::_scan_fs_changes
looks through all of its resources and checks their timestamps against the timestamps on disk. If the timestamps mismatch, it will create anItemAction
for it to be tested for reload. Afterwards inEditorFileSystem::_update_scan_actions
, it adds all of the file paths to be reloaded to an array and sends it off via emitting the"resources_reload"
signal.EditorNode
connects the"resources_reload"
signal with its_resources_changed
method. That method getsResource
Refs for each path fromResourceCache::get_ref
and then runs some checks on thatResource
. If all of the checks pass, it will callResource::reload_from_file
, which callsResourceLoader::load
, which calls some threaded methods to actually perform the load.There is an exception to this:
EditorNode::_resources_changed
makes a check toResource::editor_can_reload_from_file
. That method gets overridden in script_language.h:virtual bool editor_can_reload_from_file() override { return false; } // this is handled by editor better
Where that is intended to be handled is in
ScriptEditor::_test_script_times_on_disk
within the script_editor_plugin. It performs similar checks toEditorFileSystem
, but it only checks the files for which it currently has open tabs.The other place this might be handled is via the language server protocol, which should be active and connected to by your external editor when you have Use External Editor selected in your Godot editor settings. The method
GDScriptTextDocument::didSave
is called by the LSP when your external editor performs a save.There are a number of issues which arise from this system.
Resource::editor_can_reload_from_file
, of whether or not EditorNode is in charge of loading it seems like poor design. Resource shouldn't need to know about this.So what should be done about this?
I think the system deserves a refactor. My tentative suggestion is this: I think that all resource reloads should go through
EditorNode
. Perhaps evenEditorFileSystem
should be the de-facto location for starting the process. Rather than haveResource::editor_can_reload_from_file
determine which resources get ignored byEditorNode
, perhaps a chain of responsibility pattern or something similar could be implemented.EditorNode
requests a reload of a resource. If there are no handlers for that reload, thenEditorNode
performs the reload itself. Otherwise the handlers perform the reload or they report back if they could/did not handle the reload.ScriptEditor
could subscribe itself as a handler. Depending on what we want, it could pass through requests for scripts it doesn't currently have loaded or it could handle them just without performing the logic for updating the editor view. Other classes could implement similar functionality if it were beneficial to them. This way,ScriptEditor
and other plugins would be easier to decouple, and users could implement new reload handlers within plugins or even GDExtensions without having to modify theResource
classes they are working with.I'm honestly uncertain about the necessity of LSP having control over triggering reloads. In my opinion, that could just be removed if we can ensure that
EditorFileSystem
will always pick up and handle changes.Conclusion
I'd very much like to hear anyone's ideas, corrections, contentions, etc. on this issue. I'm not 100% certain that I have everything right, and so I want to elicit feedback and discussion rather than making this proposal in an Issue format. I would be interested in contributing this programming work myself, if there were no objections. So anyways, please let me know what you think!
Beta Was this translation helpful? Give feedback.
All reactions