From 815038962f0d917bae7d6f2dae17da3a0aebf7c8 Mon Sep 17 00:00:00 2001 From: Yuri Sizov Date: Fri, 26 Jan 2024 20:33:22 +0100 Subject: [PATCH] Improve error reporting in the asset library and in related types 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. --- core/io/image.cpp | 12 +- core/io/image.h | 2 +- core/io/image_loader.cpp | 2 +- .../plugins/asset_library_editor_plugin.cpp | 180 +++++++++++------- editor/plugins/asset_library_editor_plugin.h | 5 +- modules/svg/image_loader_svg.cpp | 2 +- scene/main/http_request.cpp | 6 +- 7 files changed, 126 insertions(+), 83 deletions(-) diff --git a/core/io/image.cpp b/core/io/image.cpp index 9aa7c9794a2d..384a33399a28 100644 --- a/core/io/image.cpp +++ b/core/io/image.cpp @@ -2781,7 +2781,7 @@ void Image::_get_clipped_src_and_dest_rects(const Ref &p_src, const Rect2 } void Image::blit_rect(const Ref &p_src, const Rect2i &p_src_rect, const Point2i &p_dest) { - ERR_FAIL_COND_MSG(p_src.is_null(), "It's not a reference to a valid Image object."); + ERR_FAIL_COND_MSG(p_src.is_null(), "Cannot blit_rect an image: invalid source Image object."); int dsize = data.size(); int srcdsize = p_src->data.size(); ERR_FAIL_COND(dsize == 0); @@ -2823,8 +2823,8 @@ void Image::blit_rect(const Ref &p_src, const Rect2i &p_src_rect, const P } void Image::blit_rect_mask(const Ref &p_src, const Ref &p_mask, const Rect2i &p_src_rect, const Point2i &p_dest) { - ERR_FAIL_COND_MSG(p_src.is_null(), "It's not a reference to a valid Image object."); - ERR_FAIL_COND_MSG(p_mask.is_null(), "It's not a reference to a valid Image object."); + ERR_FAIL_COND_MSG(p_src.is_null(), "Cannot blit_rect_mask an image: invalid source Image object."); + ERR_FAIL_COND_MSG(p_mask.is_null(), "Cannot blit_rect_mask an image: invalid mask Image object."); int dsize = data.size(); int srcdsize = p_src->data.size(); int maskdsize = p_mask->data.size(); @@ -2873,7 +2873,7 @@ void Image::blit_rect_mask(const Ref &p_src, const Ref &p_mask, co } void Image::blend_rect(const Ref &p_src, const Rect2i &p_src_rect, const Point2i &p_dest) { - ERR_FAIL_COND_MSG(p_src.is_null(), "It's not a reference to a valid Image object."); + ERR_FAIL_COND_MSG(p_src.is_null(), "Cannot blend_rect an image: invalid source Image object."); int dsize = data.size(); int srcdsize = p_src->data.size(); ERR_FAIL_COND(dsize == 0); @@ -2908,8 +2908,8 @@ void Image::blend_rect(const Ref &p_src, const Rect2i &p_src_rect, const } void Image::blend_rect_mask(const Ref &p_src, const Ref &p_mask, const Rect2i &p_src_rect, const Point2i &p_dest) { - ERR_FAIL_COND_MSG(p_src.is_null(), "It's not a reference to a valid Image object."); - ERR_FAIL_COND_MSG(p_mask.is_null(), "It's not a reference to a valid Image object."); + ERR_FAIL_COND_MSG(p_src.is_null(), "Cannot blend_rect_mask an image: invalid source Image object."); + ERR_FAIL_COND_MSG(p_mask.is_null(), "Cannot blend_rect_mask an image: invalid mask Image object."); int dsize = data.size(); int srcdsize = p_src->data.size(); int maskdsize = p_mask->data.size(); diff --git a/core/io/image.h b/core/io/image.h index be308b0ac173..e35b359a7974 100644 --- a/core/io/image.h +++ b/core/io/image.h @@ -431,7 +431,7 @@ class Image : public Resource { void set_as_black(); void copy_internals_from(const Ref &p_image) { - ERR_FAIL_COND_MSG(p_image.is_null(), "It's not a reference to a valid Image object."); + ERR_FAIL_COND_MSG(p_image.is_null(), "Cannot copy image internals: invalid Image object."); format = p_image->format; width = p_image->width; height = p_image->height; diff --git a/core/io/image_loader.cpp b/core/io/image_loader.cpp index c6452f1033ea..bef515f2597c 100644 --- a/core/io/image_loader.cpp +++ b/core/io/image_loader.cpp @@ -81,7 +81,7 @@ void ImageFormatLoaderExtension::_bind_methods() { } Error ImageLoader::load_image(String p_file, Ref p_image, Ref p_custom, BitField p_flags, float p_scale) { - ERR_FAIL_COND_V_MSG(p_image.is_null(), ERR_INVALID_PARAMETER, "It's not a reference to a valid Image object."); + ERR_FAIL_COND_V_MSG(p_image.is_null(), ERR_INVALID_PARAMETER, "Can't load an image: invalid Image object."); Ref f = p_custom; if (f.is_null()) { diff --git a/editor/plugins/asset_library_editor_plugin.cpp b/editor/plugins/asset_library_editor_plugin.cpp index 20129556860a..d5b7ab572bd3 100644 --- a/editor/plugins/asset_library_editor_plugin.cpp +++ b/editor/plugins/asset_library_editor_plugin.cpp @@ -745,86 +745,97 @@ void EditorAssetLibrary::_select_asset(int p_id) { _api_request("asset/" + itos(p_id), REQUESTING_ASSET); } -void EditorAssetLibrary::_image_update(bool use_cache, bool final, const PackedByteArray &p_data, int p_queue_id) { +void EditorAssetLibrary::_image_update(bool p_use_cache, bool p_final, const PackedByteArray &p_data, int p_queue_id) { Object *obj = ObjectDB::get_instance(image_queue[p_queue_id].target); + if (!obj) { + return; + } - if (obj) { - bool image_set = false; - PackedByteArray image_data = p_data; + bool image_set = false; + PackedByteArray image_data = p_data; - if (use_cache) { - String cache_filename_base = EditorPaths::get_singleton()->get_cache_dir().path_join("assetimage_" + image_queue[p_queue_id].image_url.md5_text()); + if (p_use_cache) { + String cache_filename_base = EditorPaths::get_singleton()->get_cache_dir().path_join("assetimage_" + image_queue[p_queue_id].image_url.md5_text()); - Ref file = FileAccess::open(cache_filename_base + ".data", FileAccess::READ); - if (file.is_valid()) { - PackedByteArray cached_data; - int len = file->get_32(); - cached_data.resize(len); + Ref file = FileAccess::open(cache_filename_base + ".data", FileAccess::READ); + if (file.is_valid()) { + PackedByteArray cached_data; + int len = file->get_32(); + cached_data.resize(len); - uint8_t *w = cached_data.ptrw(); - file->get_buffer(w, len); + uint8_t *w = cached_data.ptrw(); + file->get_buffer(w, len); - image_data = cached_data; - } + image_data = cached_data; + } + } + + int len = image_data.size(); + const uint8_t *r = image_data.ptr(); + Ref image = memnew(Image); + + uint8_t png_signature[8] = { 137, 80, 78, 71, 13, 10, 26, 10 }; + uint8_t jpg_signature[3] = { 255, 216, 255 }; + uint8_t webp_signature[4] = { 82, 73, 70, 70 }; + uint8_t bmp_signature[2] = { 66, 77 }; + + if (r) { + Ref parsed_image; + + if ((memcmp(&r[0], &png_signature[0], 8) == 0) && Image::_png_mem_loader_func) { + parsed_image = Image::_png_mem_loader_func(r, len); + } else if ((memcmp(&r[0], &jpg_signature[0], 3) == 0) && Image::_jpg_mem_loader_func) { + parsed_image = Image::_jpg_mem_loader_func(r, len); + } else if ((memcmp(&r[0], &webp_signature[0], 4) == 0) && Image::_webp_mem_loader_func) { + parsed_image = Image::_webp_mem_loader_func(r, len); + } else if ((memcmp(&r[0], &bmp_signature[0], 2) == 0) && Image::_bmp_mem_loader_func) { + parsed_image = Image::_bmp_mem_loader_func(r, len); + } else if (Image::_svg_scalable_mem_loader_func) { + parsed_image = Image::_svg_scalable_mem_loader_func(r, len, 1.0); } - int len = image_data.size(); - const uint8_t *r = image_data.ptr(); - Ref image = Ref(memnew(Image)); - - uint8_t png_signature[8] = { 137, 80, 78, 71, 13, 10, 26, 10 }; - uint8_t jpg_signature[3] = { 255, 216, 255 }; - uint8_t webp_signature[4] = { 82, 73, 70, 70 }; - uint8_t bmp_signature[2] = { 66, 77 }; - - if (r) { - if ((memcmp(&r[0], &png_signature[0], 8) == 0) && Image::_png_mem_loader_func) { - image->copy_internals_from(Image::_png_mem_loader_func(r, len)); - } else if ((memcmp(&r[0], &jpg_signature[0], 3) == 0) && Image::_jpg_mem_loader_func) { - image->copy_internals_from(Image::_jpg_mem_loader_func(r, len)); - } else if ((memcmp(&r[0], &webp_signature[0], 4) == 0) && Image::_webp_mem_loader_func) { - image->copy_internals_from(Image::_webp_mem_loader_func(r, len)); - } else if ((memcmp(&r[0], &bmp_signature[0], 2) == 0) && Image::_bmp_mem_loader_func) { - image->copy_internals_from(Image::_bmp_mem_loader_func(r, len)); - } else if (Image::_svg_scalable_mem_loader_func) { - image->copy_internals_from(Image::_svg_scalable_mem_loader_func(r, len, 1.0)); + if (parsed_image.is_null()) { + if (is_print_verbose_enabled()) { + ERR_PRINT(vformat("Asset Library: Invalid image downloaded from '%s' for asset # %d", image_queue[p_queue_id].image_url, image_queue[p_queue_id].asset_id)); } + } else { + image->copy_internals_from(parsed_image); } + } - if (!image->is_empty()) { - switch (image_queue[p_queue_id].image_type) { - case IMAGE_QUEUE_ICON: + if (!image->is_empty()) { + switch (image_queue[p_queue_id].image_type) { + case IMAGE_QUEUE_ICON: + image->resize(64 * EDSCALE, 64 * EDSCALE, Image::INTERPOLATE_LANCZOS); + break; - image->resize(64 * EDSCALE, 64 * EDSCALE, Image::INTERPOLATE_LANCZOS); + case IMAGE_QUEUE_THUMBNAIL: { + float max_height = 85 * EDSCALE; - break; - case IMAGE_QUEUE_THUMBNAIL: { - float max_height = 85 * EDSCALE; + float scale_ratio = max_height / (image->get_height() * EDSCALE); + if (scale_ratio < 1) { + image->resize(image->get_width() * EDSCALE * scale_ratio, image->get_height() * EDSCALE * scale_ratio, Image::INTERPOLATE_LANCZOS); + } + } break; - float scale_ratio = max_height / (image->get_height() * EDSCALE); - if (scale_ratio < 1) { - image->resize(image->get_width() * EDSCALE * scale_ratio, image->get_height() * EDSCALE * scale_ratio, Image::INTERPOLATE_LANCZOS); - } - } break; - case IMAGE_QUEUE_SCREENSHOT: { - float max_height = 397 * EDSCALE; + case IMAGE_QUEUE_SCREENSHOT: { + float max_height = 397 * EDSCALE; - float scale_ratio = max_height / (image->get_height() * EDSCALE); - if (scale_ratio < 1) { - image->resize(image->get_width() * EDSCALE * scale_ratio, image->get_height() * EDSCALE * scale_ratio, Image::INTERPOLATE_LANCZOS); - } - } break; - } + float scale_ratio = max_height / (image->get_height() * EDSCALE); + if (scale_ratio < 1) { + image->resize(image->get_width() * EDSCALE * scale_ratio, image->get_height() * EDSCALE * scale_ratio, Image::INTERPOLATE_LANCZOS); + } + } break; + } - Ref tex = ImageTexture::create_from_image(image); + Ref tex = ImageTexture::create_from_image(image); - obj->call("set_image", image_queue[p_queue_id].image_type, image_queue[p_queue_id].image_index, tex); - image_set = true; - } + obj->call("set_image", image_queue[p_queue_id].image_type, image_queue[p_queue_id].image_index, tex); + image_set = true; + } - if (!image_set && final) { - obj->call("set_image", image_queue[p_queue_id].image_type, image_queue[p_queue_id].image_index, get_editor_theme_icon(SNAME("FileBrokenBigThumb"))); - } + if (!image_set && p_final) { + obj->call("set_image", image_queue[p_queue_id].image_type, image_queue[p_queue_id].image_index, get_editor_theme_icon(SNAME("FileBrokenBigThumb"))); } } @@ -857,7 +868,10 @@ void EditorAssetLibrary::_image_request_completed(int p_status, int p_code, cons _image_update(p_code == HTTPClient::RESPONSE_NOT_MODIFIED, true, p_data, p_queue_id); } else { - WARN_PRINT("Error getting image file from URL: " + image_queue[p_queue_id].image_url); + 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)); + } + Object *obj = ObjectDB::get_instance(image_queue[p_queue_id].target); if (obj) { obj->call("set_image", image_queue[p_queue_id].image_type, image_queue[p_queue_id].image_index, get_editor_theme_icon(SNAME("FileBrokenBigThumb"))); @@ -906,22 +920,48 @@ void EditorAssetLibrary::_update_image_queue() { } } -void EditorAssetLibrary::_request_image(ObjectID p_for, String p_image_url, ImageType p_type, int p_image_index) { +void EditorAssetLibrary::_request_image(ObjectID p_for, int p_asset_id, String p_image_url, ImageType p_type, int p_image_index) { + // Remove extra spaces around the URL. This isn't strictly valid, but recoverable. + String trimmed_url = p_image_url.strip_edges(); + if (trimmed_url != p_image_url && is_print_verbose_enabled()) { + WARN_PRINT(vformat("Asset Library: Badly formatted image URL '%s' for asset # %d.", p_image_url, p_asset_id)); + } + + // Validate the image URL first. + { + String url_scheme; + String url_host; + int url_port; + String url_path; + Error err = trimmed_url.parse_url(url_scheme, url_host, url_port, url_path); + if (err != OK) { + if (is_print_verbose_enabled()) { + ERR_PRINT(vformat("Asset Library: Invalid image URL '%s' for asset # %d.", trimmed_url, p_asset_id)); + } + + Object *obj = ObjectDB::get_instance(p_for); + if (obj) { + obj->call("set_image", p_type, p_image_index, get_editor_theme_icon(SNAME("FileBrokenBigThumb"))); + } + return; + } + } + ImageQueue iq; - iq.image_url = p_image_url; + iq.image_url = trimmed_url; iq.image_index = p_image_index; iq.image_type = p_type; iq.request = memnew(HTTPRequest); setup_http_request(iq.request); iq.target = p_for; + iq.asset_id = p_asset_id; iq.queue_id = ++last_queue_id; iq.active = false; iq.request->connect("request_completed", callable_mp(this, &EditorAssetLibrary::_image_request_completed).bind(iq.queue_id)); image_queue[iq.queue_id] = iq; - add_child(iq.request); _image_update(true, false, PackedByteArray(), iq.queue_id); @@ -1299,7 +1339,7 @@ void EditorAssetLibrary::_http_request_completed(int p_status, int p_code, const item->connect("category_selected", callable_mp(this, &EditorAssetLibrary::_select_category)); if (r.has("icon_url") && !r["icon_url"].operator String().is_empty()) { - _request_image(item->get_instance_id(), r["icon_url"], IMAGE_QUEUE_ICON, 0); + _request_image(item->get_instance_id(), r["asset_id"], r["icon_url"], IMAGE_QUEUE_ICON, 0); } } @@ -1350,7 +1390,7 @@ void EditorAssetLibrary::_http_request_completed(int p_status, int p_code, const } if (r.has("icon_url") && !r["icon_url"].operator String().is_empty()) { - _request_image(description->get_instance_id(), r["icon_url"], IMAGE_QUEUE_ICON, 0); + _request_image(description->get_instance_id(), r["asset_id"], r["icon_url"], IMAGE_QUEUE_ICON, 0); } if (d.has("previews")) { @@ -1371,11 +1411,11 @@ void EditorAssetLibrary::_http_request_completed(int p_status, int p_code, const description->add_preview(i, is_video, video_url); if (p.has("thumbnail")) { - _request_image(description->get_instance_id(), p["thumbnail"], IMAGE_QUEUE_THUMBNAIL, i); + _request_image(description->get_instance_id(), r["asset_id"], p["thumbnail"], IMAGE_QUEUE_THUMBNAIL, i); } if (!is_video) { - _request_image(description->get_instance_id(), p["link"], IMAGE_QUEUE_SCREENSHOT, i); + _request_image(description->get_instance_id(), r["asset_id"], p["link"], IMAGE_QUEUE_SCREENSHOT, i); } } } diff --git a/editor/plugins/asset_library_editor_plugin.h b/editor/plugins/asset_library_editor_plugin.h index 2dff1869ef25..bc6167d1da98 100644 --- a/editor/plugins/asset_library_editor_plugin.h +++ b/editor/plugins/asset_library_editor_plugin.h @@ -255,14 +255,15 @@ class EditorAssetLibrary : public PanelContainer { String image_url; HTTPRequest *request = nullptr; ObjectID target; + int asset_id = -1; }; int last_queue_id; HashMap image_queue; - void _image_update(bool use_cache, bool final, const PackedByteArray &p_data, int p_queue_id); + void _image_update(bool p_use_cache, bool p_final, const PackedByteArray &p_data, int p_queue_id); void _image_request_completed(int p_status, int p_code, const PackedStringArray &headers, const PackedByteArray &p_data, int p_queue_id); - void _request_image(ObjectID p_for, String p_image_url, ImageType p_type, int p_image_index); + void _request_image(ObjectID p_for, int p_asset_id, String p_image_url, ImageType p_type, int p_image_index); void _update_image_queue(); HBoxContainer *_make_pages(int p_page, int p_page_count, int p_page_len, int p_total_items, int p_current_items); diff --git a/modules/svg/image_loader_svg.cpp b/modules/svg/image_loader_svg.cpp index a542bbc23413..affe163aeb2d 100644 --- a/modules/svg/image_loader_svg.cpp +++ b/modules/svg/image_loader_svg.cpp @@ -72,7 +72,7 @@ Ref ImageLoaderSVG::load_mem_svg(const uint8_t *p_svg, int p_size, float img.instantiate(); Error err = create_image_from_utf8_buffer(img, p_svg, p_size, p_scale, false); - ERR_FAIL_COND_V(err, Ref()); + ERR_FAIL_COND_V_MSG(err != OK, Ref(), vformat("ImageLoaderSVG: Failed to create SVG from buffer, error code %d.", err)); return img; } diff --git a/scene/main/http_request.cpp b/scene/main/http_request.cpp index fa14ad5b3c15..0f8978436977 100644 --- a/scene/main/http_request.cpp +++ b/scene/main/http_request.cpp @@ -50,12 +50,14 @@ Error HTTPRequest::_parse_url(const String &p_url) { String scheme; Error err = p_url.parse_url(scheme, url, port, request_string); - ERR_FAIL_COND_V_MSG(err != OK, err, "Error parsing URL: " + p_url + "."); + ERR_FAIL_COND_V_MSG(err != OK, err, vformat("Error parsing URL: '%s'.", p_url)); + if (scheme == "https://") { use_tls = true; } else if (scheme != "http://") { - ERR_FAIL_V_MSG(ERR_INVALID_PARAMETER, "Invalid URL scheme: " + scheme + "."); + ERR_FAIL_V_MSG(ERR_INVALID_PARAMETER, vformat("Invalid URL scheme: '%s'.", scheme)); } + if (port == 0) { port = use_tls ? 443 : 80; }