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

Hotreload of externally edited GDScript #20910

Closed
wants to merge 2 commits into from
Closed

Hotreload of externally edited GDScript #20910

wants to merge 2 commits into from

Conversation

T4g1
Copy link

@T4g1 T4g1 commented Aug 11, 2018

When editing a GDScript in an external editor, Godot tries to reload it when it gets modified. The problem was that Godot already has the script instanciated and tried to instanciate a new one from the same resource to reload the source code. This was prevented by the ResourceCache so it failed every time (see bug report #17044 for the triggered error).

This is now working correctly but the exception still triggers.

Fix #17044

@neikeq
Copy link
Contributor

neikeq commented Aug 12, 2018

Does this fix #10946 as well?

@T4g1
Copy link
Author

T4g1 commented Aug 13, 2018

I just tested it and it does not fix #10946

@T4g1
Copy link
Author

T4g1 commented Aug 13, 2018

I may be able to spend some more time this week to fix this other issue too and enhance the script management process altogether so it can handle reloading cached resources (or at least, try my best to). Do I update this PR or use another branch ?

@akien-mga akien-mga added this to the 3.1 milestone Aug 13, 2018
@akien-mga
Copy link
Member

Could you squash the commits together?

@tavurth
Copy link
Contributor

tavurth commented Aug 14, 2018

@T4g1 It would be nice to have this feature first and then work on enhancements in another PR if you have the interest.

I was about to write a script to implement this behaviour, but it's great to see that we're already working on a solution.

I can test on windows/osx/linux if you need any checks done.

@akien-mga akien-mga changed the title [fix] Hotreload of externaly edited GDScript [fix] Hotreload of externally edited GDScript Aug 15, 2018
@akien-mga akien-mga changed the title [fix] Hotreload of externally edited GDScript Hotreload of externally edited GDScript Aug 15, 2018
@@ -1649,6 +1649,17 @@ void GDScriptLanguage::reload_tool_script(const Ref<Script> &p_script, bool p_so
for (Map<Ref<GDScript>, Map<ObjectID, List<Pair<StringName, Variant> > > >::Element *E = to_reload.front(); E; E = E->next()) {

Ref<GDScript> scr = E->key();
Ref<Script> rel_scr = ResourceLoader::load(scr->get_path(), scr->get_class(), true);
Copy link
Member

Choose a reason for hiding this comment

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

This may break scripts that were edited in the built-in editor but not saved? I think there has to be a different way to do this.

Copy link
Member

Choose a reason for hiding this comment

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

This is still unresolved @T4g1.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I'm finally back into gamedev, I'll work on that this week.

Copy link
Author

Choose a reason for hiding this comment

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

I looked at it, this fix is no longer working and I don't have a good enough understanding of the engine to fix it right now. I doubt that I will dive more on this part as I do not use an external editor anymore but here is where I'm stuck: https://github.com/godotengine/godot/blob/master/editor/plugins/script_editor_plugin.cpp#L785 So we keep the edited script and we then want to script->reload() it, problem is, "has_instances" is true in GDScript::reload so the fail condition triggers: What instances does and why it needs to be empty for reload is unclear for me but depending on that, one could: a) discard it before reload, b) clear it and re-populate it after reload, c) remove the fail condition on has_instances

@akien-mga
Copy link
Member

Moving to 3.2 milestone as we're about to enter the beta stage and release freeze for Godot 3.1. Only bug fixes against issues of the 3.1 milestone (or enhancements to core features of the 3.1 roadmap) will be considered until 3.1-stable is released.

Note: If it's merged after 3.1-stable is released, this change may be considered for cherry-picking into a later 3.1.x release.

@akien-mga akien-mga modified the milestones: 3.1, 3.2 Dec 14, 2018
@akien-mga
Copy link
Member

Moving to next milestone as this seems not to be working properly as per #20910 (comment)

@akien-mga akien-mga modified the milestones: 3.2, 4.0 Oct 30, 2019
@aaronfranke
Copy link
Member

Any update? There is still this issue that needs to be resolved.

@T4g1 This is now working correctly but the exception still triggers.

If you would like to take another look at the problem, perhaps you can solve this issue too?

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.

Using an external code editor prevents Godot from reloading tool and editor scripts
8 participants