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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mihe
Copy link
Contributor

@mihe mihe commented Apr 17, 2024

Relates to #90431.

This PR refactors EditorNode::cmdline_export_mode a bit, allowing the new --import command-line option to be able to use it as well.

The changes are as follows:

  1. cmdline_export_mode is now called verbose_tasks, since it now needs to encompass importing as well, and I figured maybe something else in the future.
  2. This flag is now set from Main::start when importing/exporting, as opposed to from EditorNode::export_preset.
  3. The command-line logging that this flag enables is no longer mutually exclusive with the graphical progress dialog, but instead complements it. I couldn't see any reason why this shouldn't be the case. This also means the editor won't freeze up completely for a couple of minutes when doing --export-* without headless.
  4. The checks related to EditorResourcePreview have been removed, since they have (from my understanding) largely been made redundant by the headless check in EditorResourcePreview::start() that was introduced by #73838.

@mihe mihe requested a review from a team as a code owner April 17, 2024 15:51
@mihe mihe force-pushed the verbose-resource-tasks branch from 28b8065 to bb3f7ba Compare April 17, 2024 15:53
@mihe mihe changed the title Refactor command-line logging for import/export Refactor toggling of command-line logging for import/export Apr 17, 2024
@AThousandShips AThousandShips added this to the 4.x milestone Apr 17, 2024
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

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

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.

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.

3 participants