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

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

Merged
merged 1 commit into from
Feb 9, 2024
Merged

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

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
12 changes: 6 additions & 6 deletions core/io/image.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2781,7 +2781,7 @@ void Image::_get_clipped_src_and_dest_rects(const Ref<Image> &p_src, const Rect2
}

void Image::blit_rect(const Ref<Image> &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);
Expand Down Expand Up @@ -2823,8 +2823,8 @@ void Image::blit_rect(const Ref<Image> &p_src, const Rect2i &p_src_rect, const P
}

void Image::blit_rect_mask(const Ref<Image> &p_src, const Ref<Image> &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();
Expand Down Expand Up @@ -2873,7 +2873,7 @@ void Image::blit_rect_mask(const Ref<Image> &p_src, const Ref<Image> &p_mask, co
}

void Image::blend_rect(const Ref<Image> &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);
Expand Down Expand Up @@ -2908,8 +2908,8 @@ void Image::blend_rect(const Ref<Image> &p_src, const Rect2i &p_src_rect, const
}

void Image::blend_rect_mask(const Ref<Image> &p_src, const Ref<Image> &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();
Expand Down
2 changes: 1 addition & 1 deletion core/io/image.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,7 @@ class Image : public Resource {
void set_as_black();

void copy_internals_from(const Ref<Image> &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;
Expand Down
2 changes: 1 addition & 1 deletion core/io/image_loader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ void ImageFormatLoaderExtension::_bind_methods() {
}

Error ImageLoader::load_image(String p_file, Ref<Image> p_image, Ref<FileAccess> p_custom, BitField<ImageFormatLoader::LoaderFlags> 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<FileAccess> f = p_custom;
if (f.is_null()) {
Expand Down
180 changes: 110 additions & 70 deletions editor/plugins/asset_library_editor_plugin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<FileAccess> 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<FileAccess> 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> 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<Image> 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> image = 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) {
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<ImageTexture> tex = ImageTexture::create_from_image(image);
Ref<ImageTexture> 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")));
}
}

Expand Down Expand Up @@ -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));
}
Comment on lines +871 to +873
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 •
edited
Loading

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.


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")));
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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")) {
Expand All @@ -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);
}
}
}
Expand Down
5 changes: 3 additions & 2 deletions editor/plugins/asset_library_editor_plugin.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<int, ImageQueue> 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);
Expand Down
2 changes: 1 addition & 1 deletion modules/svg/image_loader_svg.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ Ref<Image> 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<Image>());
ERR_FAIL_COND_V_MSG(err != OK, Ref<Image>(), vformat("ImageLoaderSVG: Failed to create SVG from buffer, error code %d.", err));

return img;
}
Expand Down
6 changes: 4 additions & 2 deletions scene/main/http_request.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
Loading