Skip to content

Remove the default pixel format, fix rendering of grayscale PNGs #2573

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

Open
wants to merge 4 commits into
base: wip-v3
Choose a base branch
from

Conversation

j-jorge
Copy link
Contributor

@j-jorge j-jorge commented Jun 19, 2025

This is a following of #2230 and #2234. The goal is to display gray+alpha PNGs as gray+alpha sprites, instead of the bogus greenish rendering we have currently.

First commit

Add AXMOL_AUTOTEST_CAPTURE_DIR for cpp-tests. In auto test mode, cpp-tests will save a capture of the scene for each test, in the directory whose path is given by environment variable AXMOL_AUTOTEST_CAPTURE_DIR, if such a path is provided.

This is useful to create an overall view of cpp-tests and check if the introduced changes have an unexpected effect. It would be better if there was no randomness in the tests.

Second commit

Check the value of AXMOL_START_AUTOTEST, ignore it if zero. The mere existence of the variable was enough to enable autotest mode. I've changed the behavior to prevent autotest if the variable is set to zero.

Third commit

Remove the default pixel format from Texture2D (i.e. Texture2D::g_defaultAlphaPixelFormat). The textures have the format of the input file unless the user asks for a specific format, in which case the texture will be converted on the fly.

Fourth commit

Handle gray+alpha PNG images as gray+alpha sprites. This also remove the conversion from R8 and RG8 to RGBA8 when AX_GLES_PROFILE is not defined (e.g. a Linux build). A shader has been added for these formats and is automatically selected by ax::Sprite.

Notes

  • A discussion from Fix gray alpha png #2234 suggested to use lowp precision in the shaders, but also pointed that all shaders use highp and should be updated too. I decided to keep highp to stay consistent with existing code.
  • There are still pixel format conversions if AX_USE_METAL is defined, including the bogus RG8 -> RGBA8 that produce green textures for gray+alpha png. I would need confirmation that it can safely be removed.
  • There was some tests for the pixel format conversion. I've disabled them and will remove them if we agree that on-the-fly conversion is not required (the devs could use assets in the expected format instead).
  • Building cpp-tests takes forever, it was a pain to work on it :(

Before / after

Captures on the dev branch on the left, and the result of this PR on the right.

The main issue that caused all these changes: the top-left and bottom-left sprites are now displayed as expected.
3_Texture2D-TexturePNG

Some variations in PVR tests, including a fix for greenish rendering.
3_Texture2D-TexturePVRA8v3
3_Texture2D-TexturePVRAI88
3_Texture2D-TexturePVRAI88v3
3_Texture2D-TexturePVRI8
3_Texture2D-TexturePVRI8v3
3_Texture2D-TexturePVRRGB565
3_Texture2D-TexturePVRRGB565v3
3_Texture2D-TexturePVRRGBA5551
3_Texture2D-TexturePVRRGBA5551v3
3_Texture2D-TexturePVRA8

The pixel format conversions still have some ugly green rendering but R8 and RG8 seem okay.
3:Texture2D-TextureConvertL8
3:Texture2D-TextureConvertLA8
3:Texture2D-TextureConvertRGB888
3:Texture2D-TextureConvertRGBA8888
3:Texture2D-TexturePixelFormat

For these ones I don't know if it is expected.
25_Interval-IntervalTest
37_Node__Node-StressTest2
41_Node__Physics-PhysicsDemoBug3988
41_Node__Physics-PhysicsDemoJoints
68:SpriteFrameCache-SpriteFrameCachePixelFormatTest

@j-jorge j-jorge mentioned this pull request Jun 19, 2025
@aismann
Copy link
Contributor

aismann commented Jun 20, 2025

  • Building cpp-tests takes forever, it was a pain to work on it :(

@j-jorge
What was the most pain building a cpp-test?
I plan to add source code generation for cpp-tests on Live::Editor.

@j-jorge j-jorge marked this pull request as draft June 20, 2025 17:09
@j-jorge
Copy link
Contributor Author

j-jorge commented Jun 20, 2025

@aismann It takes forever to compile the thousands targets. I think there are too many things in the headers, thus too many header dependencies, and too many functions end up in the object files. For example, both constructors of PhysicsMaterial are present in 2475 object files. It is one example of the things that are propagated everywhere: its header is directly included in axmol.h and indirectly included in Node.h. Anything that touches this file causes a recompilation of everything.

If you do source code generation then try to create small headers and include as few as possible to reduce the time of incremental rebuilds. Stay away from axmol.h :) And if you can, try to enable unity builds.

@j-jorge
Copy link
Contributor Author

j-jorge commented Jun 20, 2025

Turning the PR to draft to fix the CI issues and resolve the texture conversion.

@j-jorge j-jorge force-pushed the rm-default-texture-format branch from 5c46dc8 to 6fee890 Compare June 20, 2025 21:19
@j-jorge j-jorge marked this pull request as ready for review June 21, 2025 05:19
@halx99 halx99 added enhancement New feature or request breaks compatibility labels Jun 21, 2025
@halx99 halx99 added this to the next milestone Jun 21, 2025
@halx99
Copy link
Collaborator

halx99 commented Jun 21, 2025

According to https://semver.org/#semantic-versioning-200, this PR should merge to branch wip-v3

@halx99 halx99 modified the milestones: next, 3.0.0 Jun 21, 2025
j-jorge added 4 commits June 21, 2025 18:08
In auto test mode, cpp-tests will save a capture of the scene in the
directory whose path is given by environment variable
AXMOL_AUTOTEST_CAPTURE_DIR, if such a path is provided.
@j-jorge j-jorge force-pushed the rm-default-texture-format branch from f11d70b to 786e13a Compare June 21, 2025 16:10
@j-jorge j-jorge changed the base branch from dev to wip-v3 June 21, 2025 16:10
@j-jorge
Copy link
Contributor Author

j-jorge commented Jun 21, 2025

@halx99 The branch is now rebased on wip-v3.

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