Export: Modify template without rcedit #75950

pkowal1982 · 2023-04-11T18:39:23Z

Using rcedit is little problematic so I've created a POC which shows how it can be dropped.
PR contains only crucial code, if it's decided to go this way I'll fix loose ends.
For now code drops old resources section and replaces it with brand new created from scratch.
Questions which I can think of:

How to test this solution properly?
Do we need to copy anything from original resources?
Is there a minimum set of information which user should provide?
All the editor code regarding rcedit should be probably removed in another dedicated PR?

Production edit: This closes Compressed godot.ico brakes Godot Icon tool #73497
and closes Provide an easier way to change the executable icon for projects exported to Windows godot-proposals#1097.

Calinou · 2023-04-11T22:50:15Z

All the editor code regarding rcedit should be probably removed in another dedicated PR?

I think the rcedit code should be removed in the same PR, as we don't want to support both approaches at the same time. While we try to preserve compatibility across 4.x, this mainly applies to the API, not the export system.

(Also, it should be possible to create compatibility handlers to automatically update old entries' names in the Windows export presets to the new names.)

akien-mga · 2023-04-12T06:35:35Z

All the editor code regarding rcedit should be probably removed in another dedicated PR?

I think the rcedit code should be removed in the same PR, as we don't want to support both approaches at the same time. While we try to preserve compatibility across 4.x, this mainly applies to the API, not the export system.

(Also, it should be possible to create compatibility handlers to automatically update old entries' names in the Windows export presets to the new names.)

I think it's fine to first assess whether this approach is good, before doing the extra work to remove the old. If it's not clear whether this PR wouldn't be merged (it hasn't been reviewed yet), there's no point doing removing the code it's meant to replace.

fire · 2023-07-17T03:09:26Z

I am interested in getting this to work since I export from Linux for Windows.

Maran23 · 2023-12-21T22:04:23Z

I also think this is a good idea since it has multiple advantages:

No rcedit needed on Windows
Project can be exported on Linux or MacOS without using Wine (for this particular feature)

Note: This PR needs a rebase. I would be happy to test this as I always used rcedit in the past.

pkowal1982 · 2023-12-22T13:29:41Z

Ok, rebased it so you can test it.

Also fixed a bug introduced here which prevented exporting console template.
Looks like it never worked, probably not too popular feature.

YuriSizov · 2023-12-22T13:37:38Z

Also fixed a bug introduced here which prevented exporting console template.
Looks like it never worked, probably not too popular feature.

This is incorrect. The official export templates (and the editor executable) are named with the underscore:

The name with a period is what is produced by our buildsystem by default, but the actual distribution is renamed. So this code was correct and should not be modified.

pkowal1982 · 2023-12-22T13:57:06Z

Ok, the ones built on github contained in artifact windows-template.zip are named:
godot.windows.template_release.x86_64.console.exe

Would be nice to change the github workflow probably to be consistent.

YuriSizov · 2023-12-22T14:07:27Z

@pkowal1982 That's not related to the CI. As I mentioned, that's how the name is generated by the buildsystem. It uses a number of suffixes to differentiate between build options, but those get removed for the official distribution. It may be worth discussing the naming scheme of the official distribution, but in the meantime we should not break the system in favor of CI artifacts which are not supposed to be used in development anyway.

pkowal1982 · 2023-12-22T18:46:51Z

Ok, reverted _console -> .console change. Though I think it would be nice
to change the CI workflow in another PR to produce _console named templates.
Now editor built from master does not work smoothly with templates built from the same branch.

Maran23

Right now, I can not really test this as I always get Wrong number of icon images..
With the old implementation, I just got a bunch of warnings (that I do not provide icons for all formats) but the modify process worked (and the icon worked in the explorer, not when I opened the properties though).

Maran23 · 2024-01-07T16:06:51Z

Tested now with an icon containing all 6 required image formats.
Looks good. I can confirm that the icon and all the metadata is set correctly. Nice!
Results below (Note: My Windows is set to German)

Godot 4.2.1	Godot 4.3.dev (this PR)

I would still recommend that if the icon does not contain all 6 image formats, we just print a warning as before, and continue with the modification.

Maran23 · 2024-09-04T20:10:00Z

platform/windows/export/template_modifier.cpp

+		return;
+	}
+	if (p_icon_file->get_16() != IMAGE_COUNT) {
+		print_error("Wrong number of icon images.");


Is it possible like with rcedit to also do that operation with just one icon image? Would be nice for backward compatibility.

We have application/icon_interpolation option in export settings, which is going to be unused if icon resizing is not implemented. This needs to be resolved.

fire · 2024-09-04T21:44:11Z

@KoBeWi What are you thoughts about modifying the template without rcedit?

KoBeWi · 2024-09-04T21:47:06Z

This basically reimplements what rcedit does and makes it built-in, no? If it works the same as before then why not I guess.
Custom code also gives us more control.

Maran23 · 2024-09-04T21:56:35Z

And it works on other platforms as well. Currently you need to use Wine on Linux to run rcedit to modify the .exe. Which is annoying for cross platform creation of the game binaries.

fire

design proposal review: I think it's a great idea
technical review: unreviewed
style review: @AThousandShips Can you take a look?
test plan: untested

AThousandShips · 2024-09-05T08:25:05Z

platform/windows/export/template_modifier.h

+		String key;
+
+		Vector<uint8_t> save() const;
+		Vector<uint8_t> &add_length(Vector<uint8_t> &bytes) const;


Suggested change

Vector<uint8_t> &add_length(Vector<uint8_t> &bytes) const;

Vector<uint8_t> &add_length(Vector<uint8_t> &r_bytes) const;

AThousandShips · 2024-09-05T08:25:22Z

platform/windows/export/template_modifier.h

+
+		Vector<uint8_t> save() const;
+		StringStructure();
+		StringStructure(String p_key, String p_value);


Suggested change

StringStructure(String p_key, String p_value);

StringStructure(const String &p_key, const String &p_value);

AThousandShips · 2024-09-05T08:27:16Z

platform/windows/export/template_modifier.cpp

+
+	uint32_t offset = sizeof(RESOURCE_DIRECTORY_TABLES) + data_entries.size() * ResourceDataEntry::SIZE;
+
+	for (Vector<uint8_t> data_entry : data_entries) {


Suggested change

for (Vector<uint8_t> data_entry : data_entries) {

for (const Vector<uint8_t> &data_entry : data_entries) {

AThousandShips · 2024-09-05T08:27:40Z

platform/windows/export/template_modifier.cpp

+	memcpy(resources.ptrw(), RESOURCE_DIRECTORY_TABLES, sizeof(RESOURCE_DIRECTORY_TABLES));
+
+	Vector<Vector<uint8_t>> data_entries;
+	for (Vector<uint8_t> image : p_group_icon.images) {


Suggested change

for (Vector<uint8_t> image : p_group_icon.images) {

for (const Vector<uint8_t> &image : p_group_icon.images) {

AThousandShips · 2024-09-05T08:27:57Z

platform/windows/export/template_modifier.cpp

+		}
+	}
+
+	for (Vector<uint8_t> data_entry : data_entries) {


Suggested change

for (Vector<uint8_t> data_entry : data_entries) {

for (const Vector<uint8_t> &data_entry : data_entries) {

AThousandShips · 2024-09-05T08:33:54Z

platform/windows/export/template_modifier.cpp

+		print_error("Wrong icon file type.");
+		return;


Suggested change

print_error("Wrong icon file type.");

return;

ERR_FAIL_MSG("Wrong icon file type.");

AThousandShips · 2024-09-05T08:34:03Z

platform/windows/export/template_modifier.cpp

+		print_error("Wrong number of icon images.");
+		return;


Suggested change

print_error("Wrong number of icon images.");

return;

ERR_FAIL_MSG("Wrong number of icon images.");

AThousandShips · 2024-09-05T08:34:38Z

platform/windows/export/template_modifier.cpp

+	Error error;
+
+	Ref<FileAccess> file = FileAccess::open(path, FileAccess::READ, &error);
+	ERR_FAIL_COND_V(error, ERR_CANT_OPEN);


Suggested change

ERR_FAIL_COND_V(error, ERR_CANT_OPEN);

ERR_FAIL_COND_V(error != OK, ERR_CANT_OPEN);

Be consistent about error check here (you use != OK below)

AThousandShips · 2024-09-05T08:34:46Z

platform/windows/export/template_modifier.cpp

+
+	String truncated_path = path + ".truncated";
+	Ref<FileAccess> truncated = FileAccess::open(truncated_path, FileAccess::WRITE, &error);
+	ERR_FAIL_COND_V(error, ERR_CANT_CREATE);


Suggested change

ERR_FAIL_COND_V(error, ERR_CANT_CREATE);

ERR_FAIL_COND_V(error != OK, ERR_CANT_CREATE);

AThousandShips · 2024-09-05T08:36:19Z

platform/windows/export/template_modifier.cpp

+
+Vector<uint8_t> TemplateModifier::StringTable::save() const {
+	Vector<uint8_t> bytes = Structure::save();
+	for (StringStructure string : strings) {


Suggested change

for (StringStructure string : strings) {

for (const StringStructure &string : strings) {

KoBeWi · 2024-09-05T14:11:42Z

Tested on Windows. Using svg icon prints "Wrong icon file type.", using ico generated with GIMP iconify extension, which I'm always using, results in "Wrong number of icon images.". The executables end up without any icon. Both cases worked perfectly before the change.

pkowal1982 requested a review from a team as a code owner April 11, 2023 18:39

YuriSizov added enhancement platform:windows topic:porting labels Apr 11, 2023

YuriSizov added this to the 4.x milestone Apr 11, 2023

pkowal1982 mentioned this pull request Apr 11, 2023

Compressed godot.ico brakes Godot Icon tool #73497

Open

This was referenced Apr 28, 2023

Change godot.ico to uncompressed/fixed size #57850

Merged

Export from Linux: replace causes error "Could not find icons in executable. Wrong template?" pkowal1982/godoticon#2

Closed

This comment was marked as off-topic.

Sign in to view

Calinou added the topic:export label Jun 15, 2023

pkowal1982 force-pushed the icon branch from 60797d7 to fba8311 Compare December 22, 2023 11:55

YuriSizov requested a review from a team December 22, 2023 13:38

pkowal1982 force-pushed the icon branch from fba8311 to 1407c79 Compare December 22, 2023 17:49

Maran23 reviewed Jan 7, 2024

View reviewed changes

AThousandShips changed the title ~~Export: Modify template without rcedit, fix #73497~~ Export: Modify template without rcedit Jan 7, 2024

pkowal1982 force-pushed the icon branch 3 times, most recently from ec94441 to 2886ff8 Compare January 25, 2024 10:52

pkowal1982 mentioned this pull request Feb 6, 2024

Nonexistent function 'add_child' in base 'EditorInterface'. pkowal1982/godoticonplugin#5

Closed

pkowal1982 mentioned this pull request Feb 19, 2024

Getting Compilation Errors when installed pkowal1982/godoticonplugin#6

Closed

wlwl2 mentioned this pull request Mar 21, 2024

Update changing_application_icon_for_windows.rst godotengine/godot-docs#8563

Closed

Modify template without rcedit

0430e99

pkowal1982 force-pushed the icon branch from 2886ff8 to 0430e99 Compare May 11, 2024 18:50

Maran23 reviewed Sep 4, 2024

View reviewed changes

fire reviewed Sep 4, 2024

View reviewed changes

AThousandShips reviewed Sep 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Export: Modify template without rcedit #75950

Export: Modify template without rcedit #75950

pkowal1982 commented Apr 11, 2023 •

edited by Calinou

Loading

Calinou commented Apr 11, 2023 •

edited

Loading

akien-mga commented Apr 12, 2023

This comment was marked as off-topic.

fire commented Jul 17, 2023

Maran23 commented Dec 21, 2023

pkowal1982 commented Dec 22, 2023

YuriSizov commented Dec 22, 2023

pkowal1982 commented Dec 22, 2023

YuriSizov commented Dec 22, 2023

pkowal1982 commented Dec 22, 2023

Maran23 left a comment •

edited

Loading

Maran23 commented Jan 7, 2024

Maran23 Sep 4, 2024

KoBeWi Sep 5, 2024

fire commented Sep 4, 2024

KoBeWi commented Sep 4, 2024 •

edited

Loading

Maran23 commented Sep 4, 2024 •

edited

Loading

fire left a comment •

edited

Loading

AThousandShips Sep 5, 2024

AThousandShips Sep 5, 2024

AThousandShips Sep 5, 2024

AThousandShips Sep 5, 2024

AThousandShips Sep 5, 2024

AThousandShips Sep 5, 2024

AThousandShips Sep 5, 2024

AThousandShips Sep 5, 2024

AThousandShips Sep 5, 2024

AThousandShips Sep 5, 2024

KoBeWi commented Sep 5, 2024

	Vector<uint8_t> &add_length(Vector<uint8_t> &bytes) const;
	Vector<uint8_t> &add_length(Vector<uint8_t> &r_bytes) const;

	StringStructure(String p_key, String p_value);
	StringStructure(const String &p_key, const String &p_value);


		uint32_t offset = sizeof(RESOURCE_DIRECTORY_TABLES) + data_entries.size() * ResourceDataEntry::SIZE;

		for (Vector<uint8_t> data_entry : data_entries) {

	for (Vector<uint8_t> data_entry : data_entries) {
	for (const Vector<uint8_t> &data_entry : data_entries) {

	for (Vector<uint8_t> image : p_group_icon.images) {
	for (const Vector<uint8_t> &image : p_group_icon.images) {

	print_error("Wrong icon file type.");
	return;
	ERR_FAIL_MSG("Wrong icon file type.");

	print_error("Wrong number of icon images.");
	return;
	ERR_FAIL_MSG("Wrong number of icon images.");

	ERR_FAIL_COND_V(error, ERR_CANT_OPEN);
	ERR_FAIL_COND_V(error != OK, ERR_CANT_OPEN);

	ERR_FAIL_COND_V(error, ERR_CANT_CREATE);
	ERR_FAIL_COND_V(error != OK, ERR_CANT_CREATE);

	for (StringStructure string : strings) {
	for (const StringStructure &string : strings) {

Export: Modify template without rcedit #75950

Are you sure you want to change the base?

Export: Modify template without rcedit #75950

Conversation

pkowal1982 commented Apr 11, 2023 • edited by Calinou Loading

Calinou commented Apr 11, 2023 • edited Loading

akien-mga commented Apr 12, 2023

This comment was marked as off-topic.

fire commented Jul 17, 2023

Maran23 commented Dec 21, 2023

pkowal1982 commented Dec 22, 2023

YuriSizov commented Dec 22, 2023

pkowal1982 commented Dec 22, 2023

YuriSizov commented Dec 22, 2023

pkowal1982 commented Dec 22, 2023

Maran23 left a comment • edited Loading

Choose a reason for hiding this comment

Maran23 commented Jan 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fire commented Sep 4, 2024

KoBeWi commented Sep 4, 2024 • edited Loading

Maran23 commented Sep 4, 2024 • edited Loading

fire left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KoBeWi commented Sep 5, 2024

pkowal1982 commented Apr 11, 2023 •

edited by Calinou

Loading

Calinou commented Apr 11, 2023 •

edited

Loading

Maran23 left a comment •

edited

Loading

KoBeWi commented Sep 4, 2024 •

edited

Loading

Maran23 commented Sep 4, 2024 •

edited

Loading

fire left a comment •

edited

Loading