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.

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

Remove s3tc feature tag from arm64 macOS exports #98263

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

allenwp
Copy link
Contributor

@allenwp allenwp commented Oct 17, 2024

The s3tc feature tag is conditionally added as a preset feature for macOS, depending on the architecture.

These conditions appear to be accidentally overridden by platform features, which always include this feature.

This PR's change seems correct based on the commit notes of 28f51ba, which added the architecture-related conditions. When writing the documentation of #98251 I noticed that macOS was the only platform that included a texture format feature tag, so this alerted me to believe that something was out-of-place. All other texture format feature tags in Godot are added as "preset features", not "platform" features.

This PR does not actually resolve any issues that I am aware of, so this is a hypothetical fix/improvement. Testing of exports on an arm64 macOS device is needed...

@allenwp allenwp requested a review from a team as a code owner October 17, 2024 13:27
@AThousandShips AThousandShips added this to the 4.4 milestone Oct 17, 2024
@allenwp allenwp force-pushed the remove-s3tc-from-arm64-macos branch from 0265dec to d524c0d Compare December 6, 2024 17:09
@Calinou
Copy link
Member

Calinou commented Dec 6, 2024

For reference, the Metal feature set tables documents what kinds of VRAM-compressed formats are supported on page 3:

image

(Apple7 is M1, Apple8 is M2, Apple9 is M3 and M4.)

@Repiteo Repiteo modified the milestones: 4.4, 4.5 Feb 24, 2025
@bruvzg
Copy link
Member

bruvzg commented Mar 4, 2025

While it is out of place and should be in the preset features, it's likely needed for arm64 if compatibility renderer is used. Neither ETC2 nor ASTC are supported by macOS OpenGL implementation. Metal and Vulkan should support both.

@allenwp allenwp force-pushed the remove-s3tc-from-arm64-macos branch from d524c0d to 8f25257 Compare March 4, 2025 20:48
@allenwp
Copy link
Contributor Author

allenwp commented Mar 4, 2025

While it is out of place and should be in the preset features, it's likely needed for arm64 if compatibility renderer is used. Neither ETC2 nor ASTC are supported by macOS OpenGL implementation. Metal and Vulkan should support both.

Aha! Thanks for these details. So what happens when compress/high_quality == true on arm64 Mac OS? As it stands, I would have expected ASTC to be used, since the BPTC feature tag has not been included since 28f51ba ...

@adamscott I remember you were asking what to test with this PR: it looks like this is the testing that's needed. Low priority, mind you.

@bruvzg
Copy link
Member

bruvzg commented Mar 4, 2025

So what happens when compress/high_quality == true on arm64 Mac OS?

I'll test it tomorrow, but it's probably never exporting anything except S3TC and BPTC:

	if (architecture == "universal" || architecture == "x86_64") {
		r_features->push_back("s3tc");
		r_features->push_back("bptc");
	} else if (architecture == "arm64") {
		r_features->push_back("etc2");
		r_features->push_back("astc");
	} else {
		ERR_PRINT("Invalid architecture");
	}

macOS export is always using universal architecture.

@bruvzg
Copy link
Member

bruvzg commented Mar 5, 2025

I'll test it tomorrow, but it's probably never exporting anything except S3TC and BPTC

It is the case:

  • when using compatibility and selecting "high quality" it always fails (BPTC is not supported by OpenGL on macOS).
  • when using forward+/mobile it's always using S3TC and BPTC (I guess it's reasonable to only use one type of textures supported by both x86_64 and arm64).

Removing "s3tc" from platform flags have no effect, since it's set in preset flags. So this PR should be OK.

@allenwp
Copy link
Contributor Author

allenwp commented Mar 5, 2025

Removing "s3tc" from platform flags have no effect, since it's set in preset flags. So this PR should be OK.

If this is the case, should this PR also include a change something like this?

current code:

	if (architecture == "universal" || architecture == "x86_64") {
		r_features->push_back("s3tc");
		r_features->push_back("bptc");
	} else if (architecture == "arm64") {
		r_features->push_back("etc2");
		r_features->push_back("astc");
	} else {
		ERR_PRINT("Invalid architecture");
	}

loosely suggested change:

	if (architecture == "universal" || architecture == "x86_64" || architecture == "arm64") {
		r_features->push_back("s3tc");
		r_features->push_back("bptc");
	} else {
		ERR_PRINT("Invalid architecture");
	}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants