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

Print a warning when importing a repeating NPOT texture in a GLES2 project #48817

Merged

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented May 18, 2021

Repeating NPOT textures are not guaranteed to be displayed correctly in GLES2, since the specification does not mandate support for it. The warning is displayed for both repeating and mirrored repeating textures.

The warning is also displayed in GLES3 projects that are configured to allow falling back to GLES2.

This closes #48807.

Testing project: test_npot.zip

Questions

  • Should there be a project setting to disable this warning? It's only displayed when initially importing a texture or reimporting it, so I don't think it's that intrusive.
  • Should the warning be extended to non-repeating textures?
  • Could the wording be improved?

@Calinou Calinou requested a review from a team as a code owner May 18, 2021 17:37
@Calinou Calinou requested a review from lawnjelly May 18, 2021 17:38
@Calinou Calinou added this to the 3.4 milestone May 18, 2021
@lawnjelly
Copy link
Member

lawnjelly commented May 18, 2021

Could possibly mention HTML5 as well as GLES2. It isn't necessary for non-wrapping textures as the limitation doesn't apply there. I can't remember the exact details of why it doesn't detect npot support on HTML5, it seems to run the detection for the extension but maybe it doesn't report correctly on that platform.

Copy link
Member

@lawnjelly lawnjelly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what the CI failure is about, but yes this should be a big improvement as we continue to get issues posted about npot textures.

I don't think this should be that intrusive if it only happens on import. I suspect most users once they have seen this warning a couple of times, they will get the idea, so we may not need a runtime warning (when you start a project in GLES2 with npot wrapping textures).

@akien-mga
Copy link
Member

 editor/import/resource_importer_texture.cpp: In member function 'virtual Error ResourceImporterTexture::import(const String&, const String&, const Map<StringName, Variant>&, List<String>*, List<String>*, Variant*)':
editor/import/resource_importer_texture.cpp:399:41: error: comparison of integer expressions of different signedness: 'int' and 'unsigned int' [-Werror=sign-compare]
  399 |   if (!min_gles3 && (image->get_width() != closest_power_of_2(image->get_width()) || image->get_height() != closest_power_of_2(image->get_height()))) {
      |                      ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
editor/import/resource_importer_texture.cpp:399:106: error: comparison of integer expressions of different signedness: 'int' and 'unsigned int' [-Werror=sign-compare]
  399 |   if (!min_gles3 && (image->get_width() != closest_power_of_2(image->get_width()) || image->get_height() != closest_power_of_2(image->get_height()))) {
      |                                                                                      ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

But maybe you could use Image.is_size_po2() instead of reinventing the wheel :P

@Calinou Calinou force-pushed the import-texture-npot-gles2-warning branch from 4fd6e85 to 0581be7 Compare May 20, 2021 17:49
…oject

Repeating NPOT textures are not guaranteed to be displayed correctly
in GLES2, since the specification does not mandate support for it.

The warning is also displayed in GLES3 projects that are configured
to allow falling back to GLES2.
@Calinou Calinou force-pushed the import-texture-npot-gles2-warning branch from 0581be7 to 20f7928 Compare May 20, 2021 18:38
@Calinou
Copy link
Member Author

Calinou commented May 20, 2021

I updated the PR to use Image.is_size_po2() and mention HTML5 in the warning message.

@akien-mga akien-mga merged commit 463073a into godotengine:3.x May 20, 2021
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 3.3.2.

@Calinou Calinou deleted the import-texture-npot-gles2-warning branch August 3, 2021 15:59
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.

3 participants