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

Incorrect (?) warning about enabling S3TC BPTC texture compression on macOS export #78395

Closed
lostminds opened this issue Jun 18, 2023 · 17 comments · Fixed by #78456
Closed

Incorrect (?) warning about enabling S3TC BPTC texture compression on macOS export #78395

lostminds opened this issue Jun 18, 2023 · 17 comments · Fixed by #78456

Comments

@lostminds
Copy link

Godot version

4.1.beta2

System information

macOS 13.3.1 mobile renderer

Issue description

When setting up an export preset for macOS in a project where the Import S3TC BPTC setting is not enabled you get a warning that this platform requires this to be enabled.

Screenshot 2023-06-18 at 11 37 03

However, if you check the documentation for this project setting, it seems that this is not required at all, and it's actually very unclear (to me) what enabling it will actually change.

Screenshot 2023-06-18 at 12 47 11

As I understand this description the platform will use the format if it requires it even if the setting is disabled (so it doesn't matter). In this case the macOS export warning should be removed. If however this is required the description text also seems to note that just enabling this will do nothing, you also need to re-import all your textures for this to take effect. So just enabling the setting wouldn't be enough?

The setting is for the full project, so if you have a project with exports for both mobile and desktop I guess you need to have it turned off since it's not supported on mobile? So if it is indeed required on some desktop systems perhaps there's need for a mobile version of this setting to have it disabled there? Or just get rid of the setting entirely if this is handled automatically anyway to avoid this confusion?

Steps to reproduce

  • Have a project where the Import S3TC BPTC setting is not enabled in the project settings
  • Set up a macOS export preset
  • Observe warning (and read setting documentation)

Minimal reproduction project

Simple macOS export project

@bruvzg
Copy link
Member

bruvzg commented Jun 18, 2023

It is required for macOS export if you expect it to run on any Mac. ASTC and ETC are supported on M1/M2 (in case of ETC also by some GPUs on Intel Macs) and only when using Vulkan. S3TC is supported everywhere.

@lostminds
Copy link
Author

It is required for macOS export if you expect it to run on any Mac.

I currently have it disabled and it still runs without any problem on the intel-macs I've tried it on (haven't tested on and new M1/M2s). And what about the note that it will use these formats if they're required even if the setting is turned off?

@fire
Copy link
Member

fire commented Jun 19, 2023

@aaronfranke you may be interested

@Calinou
Copy link
Member

Calinou commented Jun 19, 2023

Are you working on a 2D or 3D project? VRAM compression is only used in 3D projects (textures are reimported when they're detected to be used in 3D in the editor). If you disable all VRAM compression methods, it won't have any effect if no textures request VRAM compression on import.

@aaronfranke
Copy link
Member

aaronfranke commented Jun 19, 2023

As I understand this description the platform will use the format if it requires it even if the setting is disabled

The host platform, meaning, the editor will import what it needs for itself. You should explicitly enable what you want to have available to you for the export platforms you want to target.

For example, if you had this setting disabled, and you tried to export for Windows or macOS (S3TC) from the Android editor (ETC2), you would not be able to. That's why Godot is telling you to enable the setting explicitly.

However, there is a bug here. The bug is that the export dialog should not check if the override is enabled, instead it should check if the desired texture format has been imported.

@lostminds
Copy link
Author

lostminds commented Jun 19, 2023

Are you working on a 2D or 3D project?

It's a 3D project

For example, if you had this setting disabled, and you tried to export for Windows or macOS (S3TC) from the Android editor (ETC2), you would not be able to. That's why Godot is telling you to enable the setting explicitly.

Aha, I see. So then it's probably because I'm exporting from macOS (which will generate S3TC regardless of the setting apparently) that this works even though the setting is not enabled.

You should explicitly enable what you want to have available to you for the export platforms you want to target.

So enabling this just makes these compressed formats available for exports to platforms that need them? If this is the case, why would you ever want to turn this off, if it just enables something that's required when it's required?

As I read the explanation it said the textures would be compressed in this format and that it's a format that's only supported on desktop. And since I also want to export to mobile platforms I didn't want to enable it.

@aaronfranke
Copy link
Member

aaronfranke commented Jun 19, 2023

If this is the case, why would you ever want to turn this off

Importing both formats would double the time it takes to import textures, so it's desired to have these settings off by default to speed up import time for users with simple projects especially for ones that don't export (like test projects).

As I read the explanation it said the textures would be compressed in this format and that it's a format that's only supported on desktop. And since I also want to export to mobile platforms I didn't want to enable it.

I understand the confusion but in this case it's a situation of needing both "format that only works on desktop" and "format that only works on mobile". I'm not sure how to best convey this.

@lostminds
Copy link
Author

needing both "format that only works on desktop" and "format that only works on mobile"

I think I understand now. I also now see that there are two settings, "Import S3TC BPTC" and "Import ETC2 ASTC", so it's possible to enable both (or none). So if you enable both, does this mean that the exports will just include and use the proper formats for each platform? So that the proper configuration if you do 3d and export to both desktop and mobile would be to have both enabled? Or do you need to toggle them on/off and re-import the textures?

I did not see both settings first since I filtered the settings on "S3TC" based on the warning to find it, and seeing only this one setting I figured it was the only VRAM compression setting, and enabling it would be incompatible with mobile.

@aaronfranke
Copy link
Member

@lostminds Yes, the exporter will only export what it needs, and it grabs data from what has been imported. So you can enable import of both formats, but only one will be included in the export.

@lostminds
Copy link
Author

Thanks for your explanations and time. I'll close this now since it's not an incorrect warning. And I've opened a proposal instead with some suggestions for changes to the descriptions of these settings to make them easier to understand: godotengine/godot-proposals#7119

@clayjohn
Copy link
Member

clayjohn commented Jun 19, 2023

I am going to reopen as the error message is misleading in this case and needs to be improved. As Aaronfranke says above:

However, there is a bug here. The bug is that the export dialog should not check if the override is enabled, instead it should check if the desired texture format has been imported.

I am also tagging this for 4.1 as it is going to bite a lot of users given the recent change in the import settings.

@clayjohn clayjohn reopened this Jun 19, 2023
@clayjohn clayjohn added the bug label Jun 19, 2023
@clayjohn clayjohn added this to the 4.1 milestone Jun 19, 2023
@aaronfranke
Copy link
Member

Now that I think about it more, I can see two solutions (we could also do both):

  • Make the export dialog check if the desired texture format has been imported instead of if the override is enabled.
  • Make the export dialog have a button to enable the override instead of telling people to go in the project settings.

@clayjohn
Copy link
Member

Now that I think about it more, I can see two solutions (we could also do both):

  • Make the export dialog check if the desired texture format has been imported instead of if the override is enabled.
  • Make the export dialog have a button to enable the override instead of telling people to go in the project settings.

IMO we should definitely do the first, and should probably do the second as well if it is technically feasible.

@akien-mga
Copy link
Member

akien-mga commented Jun 19, 2023

Make the export dialog check if the desired texture format has been imported instead of if the override is enabled.

That would be great, but how would we do it? Would we parse all .import files to check that each texture using VRAM Compressed has been imported with the right format(s)?


Overall, I was never happy that we have to choose ahead of time what formats to import, when where it matters is when you export. So I believe moving more user-friendly logic to the export dialog goes in the right direction, and the import override settings could even be hidden as advanced in the project settings.

For me the ideal workflow would be something like:

  • You start working on your project, it imports what the host needs, doesn't ask questions (current behavior).
  • When you add export presets for new platforms, it lets you know if you're missing the texture format needed for that platform (and only if you actually use VRAM compressed textures - shouldn't nag 2D users about it if they use lossless).
    • You get to choose whether to enable that format at import time, so from there on your import times on the host will be doubled as you need to handle two formats
    • Or you can choose for it to be handled at export time, where it will trigger an import of all missing textures for the target format when you do an export - so you pay the cost of importing all that stuff the first time you export (that's proposal material of course, and needs to be discussed and fleshed out - or rejected - just mentioning it here because I always disliked the way we force imports meant for export time before the user can do any development).

@lostminds
Copy link
Author

I agree with @akien-mga here. The whole thing with the importer also having access to the current editor platform VRAM format is not very obvious. Since the average user will probably not know or understand this, it will also be hard to understand why the same project and export settings will produce a different result when used on a different platform.

Ideally I'd say the importer should be able to re-import these on the fly if required during export so we don't need the importer project setting at all. But if that's not possible or too much work now, I think it's better in the meantime to guide the user to set the project settings correctly (and perhaps re-import textures if needed) so that the project will export correctly on any platform with these settings.

@clayjohn
Copy link
Member

clayjohn commented Jun 19, 2023

Ideally I'd say the importer should be able to re-import these on the fly if required during export so we don't need the importer project setting at all.

We could have a popup that notifies the user that any textures encountered that are set to VRAM compress, that's havent already been compressed will be compressed during export. To avoid that, the user can choose to compress them at import time using the relevant project setting.

That also avoids the issue of having to parse all .import files ahead of time

@aaronfranke
Copy link
Member

aaronfranke commented Jun 20, 2023

Great discussion, I have opened some PRs to fix issues related to this PR: #78455, #78456, #78457. Testing and review is appreciated. The third one depends on the first two.

Note that this doesn't implement Akien's last bullet point, which would be cool, but would also be a lot of work.

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

Successfully merging a pull request may close this issue.

8 participants