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

Refactor toggling of command-line logging for import/export #90806

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+22 −15
Open

Refactor toggling of command-line logging for import/export #90806

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions editor/editor_node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1087,9 +1087,7 @@ void EditorNode::_sources_changed(bool p_exist) {
}

// Start preview thread now that it's safe.
if (!singleton->cmdline_export_mode) {
EditorResourcePreview::get_singleton()->start();
}
EditorResourcePreview::get_singleton()->start();
}
}

Expand Down Expand Up @@ -1703,9 +1701,7 @@ void EditorNode::_save_scene_with_preview(String p_file, int p_idx) {
save.step(TTR("Saving Scene"), 4);
_save_scene(p_file, p_idx);

if (!singleton->cmdline_export_mode) {
EditorResourcePreview::get_singleton()->check_for_invalidation(p_file);
}
EditorResourcePreview::get_singleton()->check_for_invalidation(p_file);
Copy link
Member

Choose a reason for hiding this comment

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

I saw the rationale for removing the guards for start(), but this one doesn't seem to be a no-op on headless and will still create a MutexLock and possibly trigger more logic.

I haven't evaluated what impact this actually has, maybe it's fine. But I'm just flagging it as historically EditorResourcePreview has been the source of many deadlocks due to it acquiring a lock on the renderer to make previews, at the same time where we're trying to import assets also using the renderer.

Copy link
Member

@akien-mga akien-mga Apr 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Likewise, this was also the issue with ProgressDialog and the reason for cmdline_export_mode to bypass both. See #85222 for details. This was a major pain during the 4.2 beta cycle, so we should tread with caution here.

CC @clayjohn

Copy link
Contributor Author

@mihe mihe Apr 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Right, yeah, I should probably have justified that particular if (!singleton->cmdline_export_mode) removal. The reason for removing that one was because I couldn't see why EditorNode::_save_scene_with_preview would be called during import/export. In the projects I've tested it never seems to be.

I don't quite see the relevance of #85222 with regards to progress_dialog though. The mutually exclusive part of cmdline_export_mode and progress_dialog seems to have originated from a tentative fix of #24031, which I suppose may still be relevant. Again, I didn't see any issues on my end, but the reporter there mentions discrepancies between Windows and macOS, so I suppose that could be why.

Anyway, I can just dial back this PR to just be about setting cmdline_export_mode from Main::start(). Having some sort of command-line output for --import seems more important than fixing the issue of the editor freezing up (temporarily) when not using headless.

Copy link
Member

Choose a reason for hiding this comment

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

Well I guess we can give this PR a try and see if anything breaks, and if so we can go back to a more conservative approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also note that if #24031 is indeed still a problem, then that means it could potentially have been reintroduced in the context of --import, which is obviously live in 4.2.2 right now.

}

bool EditorNode::_validate_scene_recursive(const String &p_filename, Node *p_node) {
Expand Down Expand Up @@ -4638,28 +4634,33 @@ bool EditorNode::is_object_of_custom_type(const Object *p_object, const StringNa
}

void EditorNode::progress_add_task(const String &p_task, const String &p_label, int p_steps, bool p_can_cancel) {
if (singleton->cmdline_export_mode) {
if (singleton->verbose_tasks) {
print_line(p_task + ": begin: " + p_label + " steps: " + itos(p_steps));
} else if (singleton->progress_dialog) {
}

if (singleton->progress_dialog) {
singleton->progress_dialog->add_task(p_task, p_label, p_steps, p_can_cancel);
}
}

bool EditorNode::progress_task_step(const String &p_task, const String &p_state, int p_step, bool p_force_refresh) {
if (singleton->cmdline_export_mode) {
if (singleton->verbose_tasks) {
print_line("\t" + p_task + ": step " + itos(p_step) + ": " + p_state);
return false;
} else if (singleton->progress_dialog) {
}

if (singleton->progress_dialog) {
return singleton->progress_dialog->task_step(p_task, p_state, p_step, p_force_refresh);
} else {
return false;
}
}

void EditorNode::progress_end_task(const String &p_task) {
if (singleton->cmdline_export_mode) {
if (singleton->verbose_tasks) {
print_line(p_task + ": end");
} else if (singleton->progress_dialog) {
}

if (singleton->progress_dialog) {
singleton->progress_dialog->end_task(p_task);
}
}
Expand Down Expand Up @@ -4864,7 +4865,6 @@ Error EditorNode::export_preset(const String &p_preset, const String &p_path, bo
export_defer.debug = p_debug;
export_defer.pack_only = p_pack_only;
export_defer.android_build_template = p_android_build_template;
cmdline_export_mode = true;
return OK;
}

Expand Down
5 changes: 4 additions & 1 deletion editor/editor_node.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,7 @@ class EditorNode : public Node {
bool script_distraction_free = false;

bool changing_scene = false;
bool cmdline_export_mode = false;
bool verbose_tasks = false;
bool convert_old = false;
bool immediate_dialog_confirmed = false;
bool opening_prev = false;
Expand Down Expand Up @@ -896,6 +896,9 @@ class EditorNode : public Node {

int execute_and_show_output(const String &p_title, const String &p_path, const List<String> &p_arguments, bool p_close_on_ok = true, bool p_close_on_errors = false, String *r_output = nullptr);

void set_verbose_tasks(bool p_enabled) { verbose_tasks = p_enabled; }
bool are_tasks_verbose() const { return verbose_tasks; }

EditorNode();
~EditorNode();

Expand Down
4 changes: 4 additions & 0 deletions main/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3651,6 +3651,10 @@ int Main::start() {
editor_node = memnew(EditorNode);
sml->get_root()->add_child(editor_node);

if (!_export_preset.is_empty() || wait_for_import) {
editor_node->set_verbose_tasks(true);
}

if (!_export_preset.is_empty()) {
editor_node->export_preset(_export_preset, positional_arg, export_debug, export_pack_only, install_android_build_template);
game_path = ""; // Do not load anything.
Expand Down
Loading