Possible bugs detected

[Rubonnek]

Godot version

master branch at 5ee9831

System information

Arch Linux

Issue description

During the code review in #84445 we stumbled upon three code sections of the engine that have not been patched and may require closer inspection.

---

1. In platform/macos/export/export_plugin.cpp

There's an attempt to copy an Image but instead its reference count is just increased:

godot/platform/macos/export/export_plugin.cpp

Lines 588 to 591 in 5ee9831

	for (uint64_t i = 0; i < (sizeof(icon_infos) / sizeof(icon_infos[0])); ++i) {
	Ref<Image> copy = p_icon; // does this make sense? doesn't this just increase the reference count instead of making a copy? Do we even need a copy?
	copy->convert(Image::FORMAT_RGBA8);
	copy->resize(icon_infos[i].size, icon_infos[i].size, (Image::Interpolation)(p_preset->get("application/icon_interpolation").operator int()));

Original comment by Riteo:

• Add const references detected by clang-tidy #84445 (comment)

---

2. In editor/import/resource_importer_scene.cpp

There's an attempt to duplicate the options but the duplicate seems unnecessary:

godot/editor/import/resource_importer_scene.cpp

Lines 2372 to 2374 in 5ee9831

	HashMap<StringName, Variant> options_dupe = p_options;
	Node *scene = importer->import_scene(p_source_file, EditorSceneFormatImporter::IMPORT_ANIMATION | EditorSceneFormatImporter::IMPORT_GENERATE_TANGENT_ARRAYS | EditorSceneFormatImporter::IMPORT_FORCE_DISABLE_MESH_COMPRESSION, options_dupe, nullptr, &err);

Original comment by AThousandShips

• Add const references detected by clang-tidy #84445 (comment)

---

3. In core/io/resource_saver.cpp

There's an attempt to copy a Resource but instead its reference count is just increased:

godot/core/io/resource_saver.cpp

Line 123 in 5ee9831

Ref<Resource> rwcopy = p_resource;

Original comment by AThousandShips

• Add const references detected by clang-tidy #84445 (comment)

Steps to reproduce

N/A - This issue is related to a possible engine bug or code clean up.

Minimal reproduction project

N/A - This issue is related to a possible engine bug or code clean up.

[Rubonnek]

I won't be able to work on these anytime soon. They are up for grabs if anyone wants to look into these.

[jsjtxietian]

I can help look into these and test whether it causes potential bugs.

[jsjtxietian]

1

There's an attempt to copy an Image but instead its reference count is just increased:

After some investigation, my conclusion is at least it's not a obvious bug.

Here we init a icon and pass it to `_make_icon:

godot/platform/macos/export/export_plugin.cpp

Lines 1640 to 1645 in 5ee9831

	Ref<Image> icon;
	icon.instantiate();
	err = ImageLoader::load_image(icon_path, icon);
	if (err == OK && !icon->is_empty()) {
	_make_icon(p_preset, icon, data);
	}

And in the loop we do all the operation (convert & resize) on the p_icon passed in and use it as a image to be serialized per loop. So it looks like there is no bug here because at each loop we always do all the necessay ops, but in this way we do change p_icon's data all the time, and the change will be carried to next loop, so it might not look the same compared to if we do all the op on the origin p_icon.

godot/platform/macos/export/export_plugin.cpp

Lines 588 to 591 in 5ee9831

	for (uint64_t i = 0; i < (sizeof(icon_infos) / sizeof(icon_infos[0])); ++i) {
	Ref<Image> copy = p_icon; // does this make sense? doesn't this just increase the reference count instead of making a copy? Do we even need a copy?
	copy->convert(Image::FORMAT_RGBA8);
	copy->resize(icon_infos[i].size, icon_infos[i].size, (Image::Interpolation)(p_preset->get("application/icon_interpolation").operator int()));

But I don't have a mac, other contributors are welcomed to investiage based on my finding.

2

There's an attempt to duplicate the options but the duplicate is seems unnecessary:

I believe it is unnecessary as we just read options_dupe's content in all import_scene function, and here is the same call with out a copy right below it:

godot/editor/import/resource_importer_scene.cpp

Line 2440 in 5ee9831

Node *scene = importer->import_scene(src_path, import_flags, p_options, &missing_deps, &err);

3

There's an attempt to copy a Resource but instead its reference count is just increased:

I did some inverstigation and found the code is here when godot is open sourced. IMO it's safe to remove it as it doesnt make sense (The ref count is 2 and after the assign is 3, no need to increase the ref count to protect the value)

Here is the origin code (all the way back to 2014) :

RES rwcopy = p_resource;
if (p_flags&FLAG_CHANGE_PATH)
	rwcopy->set_path(local_path);

err = saver[i]->save(p_path,p_resource,p_flags);

if (err == OK ) {

#ifdef TOOLS_ENABLED

	((Resource*)p_resource.ptr())->set_edited(false);
	if (timestamp_on_save) {
		uint64_t mt = FileAccess::get_modified_time(p_path);

		((Resource*)p_resource.ptr())->set_last_modified_time(mt);
	}
#endif

	if (p_flags&FLAG_CHANGE_PATH)
		rwcopy->set_path(old_path);

[bruvzg]

So it looks like there is no bug here because at each loop we always do all the necessay ops, but in this way we do change p_icon's data all the time, and the change will be carried to next loop, so it might not look the same compared to if we do all the op on the origin p_icon.

It looks like a bug. This should be Ref<Image> copy = p_icon->duplicate();, it's subsequently resizing it one step down each iteration, so it's not broken as is, but downscaling from the source image should be better for image quality (and probably the intended way for it to work).

[jsjtxietian]

but downscaling from the source image should be better for image quality (and probably the intended way for it to work).

Yes, a bug that may not be easily observed.

[akien-mga]

Which of the reported issues are still valid? Might be worth changing it to a task list so the fixed ones can be checked.

[jsjtxietian]

I believe all of the three are tracked and already fixed.