iOS: Fix multiple issues with PVRTC import, disable ETC1 #38076

DrMoriarty · 2020-04-21T12:30:25Z

and some other VRAM compression formats.

For iOS we enable pvrtc feature by default for both GLES2/GLES3
Etc1 for iOS doesn't have any sense, so it disabled.
Fixed checks in export editor.
Fixed pvrtc ability detection in GLES2 driver.
Fixed pvrtc encoding procedure.
Compressed stream texture now saves right image dimensions for every compression format.
Also it allows to compressor resize image on it's side to make it more optimized.
So ETC1 format becames smaller then before. Pvrtc format always saves square texture.

Bugsquad edit: closes #28683, closes #28621, closes #28596 and maybe others
For #38040 and #35851, the fix has been moved to #42118.

DrMoriarty · 2020-04-21T12:30:54Z

I'm not sure that it will work for master, so make PR for 3.2 branch.

drivers/gles2/rasterizer_storage_gles2.cpp

Filippopotamus · 2020-09-14T17:53:37Z

Loading textures on iOS using GLES2 is still an issue. Is there a plan to merge this fix?

HEAVYPOLY · 2020-09-14T18:37:31Z

+1 this is a big one for my project.

akien-mga · 2020-09-14T19:06:16Z

I need contributors familiar with iOS to help review it, CC @bruvzg @naithar.

The fixes should also be ported to the master branch.

Filippopotamus · 2020-09-14T23:47:37Z

@akien-mga I have cherry picked it to our 3.2.2 fork and it does seem to fix the GLES2 loading resource issue in iOS

akien-mga · 2020-09-15T07:29:16Z

editor/import/resource_importer_texture.cpp

-	bool resize_to_po2 = false;
-
-	if (p_compress_mode == COMPRESS_VIDEO_RAM && p_force_po2_for_compressed && (p_mipmaps || p_texture_flags & Texture::FLAG_REPEAT)) {
-		resize_to_po2 = true;
-		f->store_16(next_power_of_2(p_image->get_width()));
-		f->store_16(p_image->get_width());
-		f->store_16(next_power_of_2(p_image->get_height()));
-		f->store_16(p_image->get_height());
-	} else {
-		f->store_16(p_image->get_width());
-		f->store_16(0);
-		f->store_16(p_image->get_height());
-		f->store_16(0);
-	}
-	f->store_32(p_texture_flags);


These are the only changes I'm not confident with as they impact all formats, so I want @reduz to review this more closely.

The rest of the changes seem good though (with some obvious bugfixes like in drivers/gles2 or platform/iphone/export/export.cpp), so I'd suggest splitting the texture importer change to a dedicated PR and remove it here, so that the rest can be merged first.

Pretty good changes overall, thanks for your work and sorry for the late review.

DrMoriarty · 2020-09-16T15:29:44Z

@akien-mga #42118

akien-mga · 2020-09-23T09:05:13Z

Rebased to remove the unrelated CHANGELOG conflict, and made the commit log a bit more explicit.

Could you check if the issues referenced as fixed by this PR are still all fixed, or if some of them are fixed by #42118 instead (or only fixed once both are merged)?

akien-mga · 2020-09-23T09:07:00Z

editor/editor_export.cpp

+
+	String driver = ProjectSettings::get_singleton()->get("rendering/quality/driver/driver_name");
+	bool driver_fallback = ProjectSettings::get_singleton()->get("rendering/quality/driver/fallback_to_gles2");
+	bool etc_supported = ProjectSettings::get_singleton()->get("rendering/vram_compression/import_etc");


I guess etc_supported is not relevant for iOS (and now disabled by platform/iphone/export/export.cpp changes), so maybe it can be removed from the checks / error messages?

Yes, I think so

I pushed an amend that removes the etc_supported check.

DrMoriarty · 2020-09-23T09:09:44Z

@akien-mga
issues #38040 and #35851 related to PR #42118
Other related to this PR

Fixes: godotengine#28683, godotengine#28621, godotengine#28596 and maybe others For iOS we enable pvrtc feature by default for both GLES2/GLES3 Etc1 for iOS doesn't have any sense, so it disabled. Fixed checks in export editor. Fixed pvrtc ability detection in GLES2 driver. Fixed pvrtc encoding procedure.

akien-mga

Looks great to me. I can't really assess the modules/pvr/texture_loader_pvr.cpp changes but I trust that you tested them and that they make things work as they should. The rest is 👍

Fixes: godotengine#28683, godotengine#28621, godotengine#28596 and maybe others For iOS we enable pvrtc feature by default for all backends Etc1 for iOS doesn't have any sense, so it disabled. Fixed checks in export editor. Fixed pvrtc encoding procedure. Edit by Akien: Forward-ported from godotengine#38076, this may not make sense as is for Godot 4.0, but it's important that we have the latest code in sync with 3.2 for when more rendering backends and proper iOS support are added back. Co-authored-by: Rémi Verschelde <rverschelde@gmail.com>

akien-mga · 2020-09-23T09:29:01Z

I forward-ported the relevant changes for the master branch with #42262 (untested, and some of it might not make sense for Vulkan, hence the TODOs).

reduz · 2020-09-27T18:15:45Z

As most users dont go iOS out of the box, I think disabling PVRTC by default is likely the best idea, and only enable it if you are planning to export to iOS.

DrMoriarty · 2020-09-27T19:03:57Z

@reduz this PR don't change default enabled compressed textures formats. So PVRTC will be disabled until user enabled it.

akien-mga · 2020-09-28T08:17:18Z

Yes @reduz seems to have misinterpreted the code, this doesn't change the fact that PVRTC is off by default (but it should be toggled on if you want to target iOS with GLES2, just like ETC1 needs to be toggled on manually if you want to target Android with GLES2).

akien-mga · 2020-09-28T08:17:26Z

Thanks!

Fixes: godotengine#28683, godotengine#28621, godotengine#28596 and maybe others For iOS we enable pvrtc feature by default for all backends Etc1 for iOS doesn't have any sense, so it disabled. Fixed checks in export editor. Fixed pvrtc encoding procedure. Edit by Akien: Forward-ported from godotengine#38076, this may not make sense as is for Godot 4.0, but it's important that we have the latest code in sync with 3.2 for when more rendering backends and proper iOS support are added back. Co-authored-by: Rémi Verschelde <rverschelde@gmail.com>

DrMoriarty requested a review from reduz as a code owner April 21, 2020 12:30

Calinou added bug enhancement platform:ios topic:porting labels Apr 21, 2020

akien-mga reviewed Apr 21, 2020

View reviewed changes

drivers/gles2/rasterizer_storage_gles2.cpp Outdated Show resolved Hide resolved

akien-mga added this to the 3.2 milestone Apr 21, 2020

DrMoriarty force-pushed the fix_pvrtc branch from b5074a9 to dab386d Compare April 21, 2020 13:53

DrMoriarty force-pushed the fix_pvrtc branch 2 times, most recently from 7f41407 to 02eda97 Compare May 21, 2020 09:00

Calinou mentioned this pull request Sep 14, 2020

Vram compressed textures not working on iOS export with GLES2 #42040

Closed

akien-mga reviewed Sep 15, 2020

View reviewed changes

DrMoriarty force-pushed the fix_pvrtc branch from 02eda97 to 70570c1 Compare September 16, 2020 15:14

akien-mga mentioned this pull request Sep 23, 2020

Fix import logic for VRAM compressed texture formats #42118

Open

akien-mga force-pushed the fix_pvrtc branch from 70570c1 to 90b7674 Compare September 23, 2020 09:03

akien-mga changed the title ~~Ultimate fix for PVRTC~~ iOS: Fix multiple issues with PVRTC import, disable ETC1 Sep 23, 2020

akien-mga reviewed Sep 23, 2020

View reviewed changes

akien-mga force-pushed the fix_pvrtc branch from 90b7674 to f388ea0 Compare September 23, 2020 09:14

akien-mga approved these changes Sep 23, 2020

View reviewed changes

akien-mga added the topic:import label Sep 23, 2020

akien-mga mentioned this pull request Sep 23, 2020

iOS: Fix multiple issues with PVRTC import, disable ETC1 #42262

Merged

akien-mga merged commit 8ca9680 into godotengine:3.2 Sep 28, 2020

DrMoriarty deleted the fix_pvrtc branch September 28, 2020 08:30

Calinou mentioned this pull request Dec 15, 2020

iOS uses more RAM than other platforms when using large AtlasTextures #36105

Open

akien-mga modified the milestones: 3.2, 3.3 Apr 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iOS: Fix multiple issues with PVRTC import, disable ETC1 #38076

iOS: Fix multiple issues with PVRTC import, disable ETC1 #38076

DrMoriarty commented Apr 21, 2020 •

edited by akien-mga

Loading

DrMoriarty commented Apr 21, 2020

Filippopotamus commented Sep 14, 2020

HEAVYPOLY commented Sep 14, 2020

akien-mga commented Sep 14, 2020

Filippopotamus commented Sep 14, 2020

akien-mga Sep 15, 2020 •

edited

Loading

DrMoriarty commented Sep 16, 2020

akien-mga commented Sep 23, 2020

akien-mga Sep 23, 2020

DrMoriarty Sep 23, 2020

akien-mga Sep 23, 2020

DrMoriarty commented Sep 23, 2020

akien-mga left a comment

akien-mga commented Sep 23, 2020

reduz commented Sep 27, 2020

DrMoriarty commented Sep 27, 2020

akien-mga commented Sep 28, 2020

akien-mga commented Sep 28, 2020

iOS: Fix multiple issues with PVRTC import, disable ETC1 #38076

iOS: Fix multiple issues with PVRTC import, disable ETC1 #38076

Conversation

DrMoriarty commented Apr 21, 2020 • edited by akien-mga Loading

DrMoriarty commented Apr 21, 2020

Filippopotamus commented Sep 14, 2020

HEAVYPOLY commented Sep 14, 2020

akien-mga commented Sep 14, 2020

Filippopotamus commented Sep 14, 2020

akien-mga Sep 15, 2020 • edited Loading

Choose a reason for hiding this comment

DrMoriarty commented Sep 16, 2020

akien-mga commented Sep 23, 2020

akien-mga Sep 23, 2020

Choose a reason for hiding this comment

DrMoriarty Sep 23, 2020

Choose a reason for hiding this comment

akien-mga Sep 23, 2020

Choose a reason for hiding this comment

DrMoriarty commented Sep 23, 2020

akien-mga left a comment

Choose a reason for hiding this comment

akien-mga commented Sep 23, 2020

reduz commented Sep 27, 2020

DrMoriarty commented Sep 27, 2020

akien-mga commented Sep 28, 2020

akien-mga commented Sep 28, 2020

DrMoriarty commented Apr 21, 2020 •

edited by akien-mga

Loading

akien-mga Sep 15, 2020 •

edited

Loading