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

Improve error reporting in the asset library and in related types #87628

Merged

Conversation

YuriSizov
Copy link
Contributor

@YuriSizov YuriSizov commented Jan 26, 2024

Right now opening asset library, both in the project manager and in the editor, may result in a barrage of error messages. All of them are related to various problems with assets listed on the frontend. But that's not really relevant to end users. And for people who may find some relevance in these messages, they remain obscure and and confusing; require debugging the engine to actually understand.

So this PR tries to improve related error messages, adds early checks to avoid invalid calls to internal APIs, and hides some of the messages behind the --verbose flag.

This is what the output can look like:

## PM

editor\plugins\asset_library_editor_plugin.cpp:872 - Asset Library: Error getting image from 'https://raw.githubusercontent.com/ShatReal/drag-drop-inventory/main/icon.png' for asset # 2026.
WARNING: Asset Library: Error getting image from 'https://raw.githubusercontent.com/ShatReal/drag-drop-inventory/main/icon.png' for asset # 2026.
     at: EditorAssetLibrary::_image_request_completed (editor\plugins\asset_library_editor_plugin.cpp:872)
WARNING: Asset Library: Error getting image from 'https://raw.githubusercontent.com/ShatReal/Infinite-Save-System-4/master/icon.png' for asset # 2380.
     at: EditorAssetLibrary::_image_request_completed (editor\plugins\asset_library_editor_plugin.cpp:872)
editor\plugins\asset_library_editor_plugin.cpp:872 - Asset Library: Error getting image from 'https://raw.githubusercontent.com/ShatReal/Infinite-Save-System-4/master/icon.png' for asset # 2380.
ERROR: ImageLoaderSVG: Failed to create SVG from buffer, error code 30.
   at: (modules\svg\image_loader_svg.cpp:75)
ImageLoaderSVG: Failed to create SVG from buffer, error code 30.
ERROR: Asset Library: Invalid image downloaded from 'https://gitee.com/L_HM/backpack_grid/blob/master/icon.png' for asset # 2363
   at: EditorAssetLibrary::_image_update (editor\plugins\asset_library_editor_plugin.cpp:799)
editor\plugins\asset_library_editor_plugin.cpp:799 - Asset Library: Invalid image downloaded from 'https://gitee.com/L_HM/backpack_grid/blob/master/icon.png' for asset # 2363

## Editor

editor\plugins\asset_library_editor_plugin.cpp:927 - Asset Library: Badly formatted image URL '	https://raw.githubusercontent.com/Daylily-Zeleen/GDScript-Formatter/main/icon.png' for asset # 2540.

The latter for example currently manifests as

ERROR: Invalid URL scheme: 	https://.
   at: (scene\main\http_request.cpp:57)
Invalid URL scheme: 	https://.

Some of the touched up error messages are outside of the scope for the asset library specifically, but the improvements should still be welcome. Namely, we seem to have this beauty across the codebase: "It's not a reference to a valid ... object." And despite the claim of the commit message that added it, it's not actually an obvious error, especially if repeated 8 times verbatim.

PS. Changes to EditorAssetLibrary::_image_update include whitespace and nesting removal. They aren't as scary as they look, use the whitespace off mode when reviewing 🙃

This also makes errors related to asset image loading
verbose-only, because, frankly, users can't do much about
those errors. Spamming them with error messages
about some assets on the frontend being broken
is pointless.
Comment on lines +871 to +873
if (is_print_verbose_enabled()) {
WARN_PRINT(vformat("Asset Library: Error getting image from '%s' for asset # %d.", image_queue[p_queue_id].image_url, image_queue[p_queue_id].asset_id));
}
Copy link
Member

Choose a reason for hiding this comment

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

Makes me think we want to add a dedicated macro for this, a WARN_PRINT_VERBOSE or something

Copy link
Contributor Author

@YuriSizov YuriSizov Jan 26, 2024

Choose a reason for hiding this comment

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

So far this is very situational, and normally we want all errors and warnings to be always printed. But yes, I was thinking about this as well.

Edit: Also I was thinking about defining a local method instead, which would print a generic message once, if verbose mode is disabled. Not sure if there is a need for this though.

Copy link
Member

Choose a reason for hiding this comment

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

We have a WARN_PRINT_ONCE so a macro for WARN_PRINT_VERBOSE could be adapted to that too, but good to discuss in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

The reason why I mentioned a dedicated local method is because I want WARN_PRINT_ONCE to be called once for all warnings from all other methods. Since WARN_PRINT_ONCE works based on a scope, I need to create a funnel for all messages.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks great!

@akien-mga akien-mga merged commit b457a30 into godotengine:master Feb 9, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@YuriSizov YuriSizov deleted the assets-bigger-better-errors branch February 9, 2024 22:39
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