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.

Sign up for GitHub

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

Jump to bottom

Auto-reload scripts with external editor #51729

Merged
merged 1 commit into from
Aug 17, 2021
Merged

Auto-reload scripts with external editor #51729

merged 1 commit into from
Aug 17, 2021

Conversation

yjh0502
Copy link
Contributor

@yjh0502 yjh0502 commented Aug 16, 2021 •
edited
Loading

The PR allows auto-reload with external editors.

  • It calls _live_auto_reload_running_scripts when appropriates.
  • It uses NOTIFICATION_APPLICATION_FOCUS_IN events to trigger reload as NOTIFICATION_WM_WINDOW_FOCUS_IN is never called on master branch. I'm not sure why, probably a bug?

Here's another patch for 3.x branch. It also fixes error messages when script changed on external editor. (Another resource is loaded from path thing). I could create another PR if you prefer: https://gist.github.com/yjh0502/e2cb62f9216c953c8e54541199d7be09

related: #10946

Bugsquad edit: Fix #10946

@yjh0502 yjh0502 requested a review from a team as a code owner August 16, 2021 10:06
@Chaosus Chaosus changed the title auto-reload scripts with external editor Auto-reload scripts with external editor Aug 16, 2021
@Chaosus Chaosus added this to the 4.0 milestone Aug 16, 2021
@Calinou Calinou mentioned this pull request Aug 16, 2021
Closed
Paulb23
Paulb23 requested changes Aug 16, 2021
View reviewed changes
Copy link
Member

@Paulb23 Paulb23 left a comment

Choose a reason for hiding this comment

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

NOTIFICATION_WM_WINDOW_FOCUS_IN not triggering is a bug see also #47176. Not sure it's worth adding workarounds for it.

editor/plugins/script_text_editor.cpp Outdated
@@ -668,6 +668,10 @@ void ScriptEditor::_update_modified_scripts_for_external_editor(Ref<Script> p_fo
script->update_exports();
}
}

if (auto_reload_running_scripts) {
_live_auto_reload_running_scripts();
Copy link
Member

Choose a reason for hiding this comment

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

Should only call this if a script has been reloaded and pending_auto_reload == false

@yjh0502 yjh0502 mentioned this pull request Aug 17, 2021
Merged
@yjh0502
Copy link
Contributor Author

yjh0502 commented Aug 17, 2021

NOTIFICATION_WM_WINDOW_FOCUS_IN not triggering is a bug see also #47176. Not sure it's worth adding workarounds for it.

Let's fix the bug first (ref: #51781). I'll update this PR after merging #51781

@yjh0502 yjh0502 force-pushed the ext-reload-master branch from af78e0a to d5e4004 Compare August 17, 2021 08:32
@yjh0502
Copy link
Contributor Author

yjh0502 commented Aug 17, 2021

@Paulb23 I tried to address the issue you mentioned. Unfortunately I'm not good at naming, so please feel free to suggest another name :)

@Paulb23
Copy link
Member

Paulb23 commented Aug 17, 2021 •
edited
Loading

Looks good. No worries :) as far as naming goes perhaps _trigger_live_script_reload?

@yjh0502 yjh0502 force-pushed the ext-reload-master branch from d5e4004 to c5c4a23 Compare August 17, 2021 12:43
@yjh0502 yjh0502 force-pushed the ext-reload-master branch from c5c4a23 to 1c61245 Compare August 17, 2021 12:43
@yjh0502
Copy link
Contributor Author

yjh0502 commented Aug 17, 2021

Looks good. No worries :) as far as naming goes perhaps _trigger_live_script_reload?

Thanks for your suggestion, I changed the name.

@akien-mga akien-mga merged commit aa3909c into godotengine:master Aug 17, 2021
@akien-mga
Copy link
Member

Thanks!

@rversteegen
Copy link

Very pleased to see this fixed!

Here's another patch for 3.x branch. It also fixes error messages when script changed on external editor. (Another resource is loaded from path thing). I could create another PR if you prefer: https://gist.github.com/yjh0502/e2cb62f9216c953c8e54541199d7be09

Do I understand that this separate patch is needed to fix the error messages? Will this also get applied? Those errors are annoying and distracting.

@yjh0502 yjh0502 mentioned this pull request Aug 18, 2021
Merged
@yjh0502
Copy link
Contributor Author

yjh0502 commented Aug 19, 2021

@rversteegen The error message problem was already fixed on master branch, and I fixed the error message issue while back-porting this patch to 3.x branch, which is already merged. See also: #51828

@cptchuckles
Copy link
Contributor

This doesn't quite fix the issue I was having with the editor in #49298, particularly where export vars don't get updated after I save a script and return focus to the Godot editor, unless I close and re-open the scene. My PR #49473 addresses that but it needs a review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Paulb23 Paulb23 Paulb23 approved these changes

Assignees
No one assigned
Labels
bug topic:editor
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.

External script editing does not trigger reload of running game
6 participants
@yjh0502 @Paulb23 @akien-mga @rversteegen @cptchuckles @Chaosus