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

VRAM Compressed Texture2DArray formats do not work in Compatibility renderer #91365

Open
luciusponto opened this issue Apr 30, 2024 · 11 comments · May be fixed by #91853
Open

VRAM Compressed Texture2DArray formats do not work in Compatibility renderer #91365

luciusponto opened this issue Apr 30, 2024 · 11 comments · May be fixed by #91853

Comments

@luciusponto
Copy link

Tested versions

  • Reproducible in 4.2.2, 4.3.dev5

System information

Godot v4.3.dev5 - Windows 10.0.19045 - GLES3 (Compatibility) - NVIDIA GeForce GTX 1060 6GB (NVIDIA; 31.0.15.3713) - Intel(R) Core(TM) i5-3470 CPU @ 3.20GHz (4 Threads)

Issue description

In the compatibility renderer, textures imported as Texture2DArray seem only work if imported as: lossless, lossy, VRAM uncompressed.
However, if imported as VRAM Compressed or Basis Universal, they display as black both in the inspector as well as when used in scene materials.

This issue does no occur with the Forward+ or Mobile renderers.

image

Steps to reproduce

Open repro project, check main scene

Minimal reproduction project (MRP)

CompatTextArrayTest.zip

@clayjohn
Copy link
Member

clayjohn commented Apr 30, 2024

Running the MRP prints the following error:

ERROR: GL ERROR: Source: OpenGL	Type: Error	ID: 1	Severity: High	Message: GL_INVALID_ENUM in glCompressedTexImage2D(target=GL_TEXTURE_2D_ARRAY)
   at: _gl_debug_print (drivers/gles3/rasterizer_gles3.cpp:181)
   

This would be good for someone to investigate with a little bit of OpenGL knowledge. We should be using glCompressedTexImage3D() for TextureArrays

@CuptainTeamo
Copy link

I am investigating this issue now, but I am new to the community.

The problem I am facing now is that I couldn't get the IDE print this Error message,

ERROR: GL ERROR: Source: OpenGL	Type: Error	ID: 1	Severity: High	Message: GL_INVALID_ENUM in glCompressedTexImage2D(target=GL_TEXTURE_2D_ARRAY)
   at: _gl_debug_print (drivers/gles3/rasterizer_gles3.cpp:181)

Would anyone mind helping me on how to enter this section of code?
Already set the project properly in Visual Studio, and breakpoint works in the main.cpp, but the breakpoints do not work in the rasterizer_gles3.cpp section.

@luciusponto
Copy link
Author

@CuptainTeamo Hi. I'm also somewhat new to Godot. I didn't know about the message until I read clayjohn's comment above. I found out just now that if I go into Project Settings/General/Debug/Settings and check verbose_stdout, then the message shows when I run the project.

@CuptainTeamo
Copy link

CuptainTeamo commented May 3, 2024

Running the MRP prints the following error:

ERROR: GL ERROR: Source: OpenGL	Type: Error	ID: 1	Severity: High	Message: GL_INVALID_ENUM in glCompressedTexImage2D(target=GL_TEXTURE_2D_ARRAY)
   at: _gl_debug_print (drivers/gles3/rasterizer_gles3.cpp:181)
   

This would be good for someone to investigate with a little bit of OpenGL knowledge. We should be using glCompressedTexImage3D() for TextureArrays

@clayjohn @luciusponto
The problem I found is in the texture_storage.cpp, around line 1505
image

glCompressedTexImage2D(blit_target, i, internal_format, bw, bh, 0, size, &read[ofs]);

This line of code should be modified similarly with the code below

if (p_initialize) {
    glTexImage3D(GL_TEXTURE_2D_ARRAY, i, internal_format, w, h, texture->layers, 0, format, type, nullptr);
}
glTexSubImage3D(GL_TEXTURE_2D_ARRAY, i, 0, 0, p_layer, w, h, 1, format, type, &read[ofs]);

Because the glCompressedTexImage3D can work with the GL_TEXTURE_2D_ARRAY, but glCompressedTexImage2D cannot work with that.

This is the code I modified
image
The Issue I face now is that when calling the function, it will give me an error that the Image size is invalid.

@semensanyok
Copy link
Contributor

semensanyok commented May 3, 2024

prepared draft MR to share progress. need more time to investigate. current solution breaks forward renderer. #91508

nevermind, just had to refresh image to reload image correctly in other profile.

so fix works for all profiles. However, question:

  • shouldnt it automatically reload images on profile switch??? there is a visual hint to reload image, but maybe autoreload would be a nice feature?
  • unit test would be a boon?

@AThousandShips AThousandShips modified the milestones: 4.x, 4.3 May 3, 2024
@CuptainTeamo
Copy link

@semensanyok Hi, does this fix work for you? I tried to follow your changes to modify this

case FORMAT_BPTC_RGBA:
	return 4; //btpc bc6h

Does it fix everything for you? Not working well for me, I am trying to figure out which part I get wrong.

@semensanyok
Copy link
Contributor

semensanyok commented May 3, 2024

@semensanyok Hi, does this fix work for you? I tried to follow your changes to modify this

case FORMAT_BPTC_RGBA:
	return 4; //btpc bc6h

Does it fix everything for you? Not working well for me, I am trying to figure out which part I get wrong.

there is also diff in drivers/gles3/storage/texture_storage.cpp in my PR (#91508), did you miss it by any chance? its not like your's

answering your question - yes it does, and switching layer in shader works for all profiles, loading all 4 layers for 4 textures correctly.

nope looks like its not fixed, cached version, keep looking. forward is broken currently by this draft

P.S. its wrong, 1 is correct value, forget this diff

@CuptainTeamo
Copy link

@semensanyok

After modifying the code as you showed in your branch. I encounter this error when opening the demo project.
image

Do you have the same issue?

And the basis universal format is fixed, but the VRAM compressed is becoming to white.
image

@semensanyok
Copy link
Contributor

semensanyok commented May 9, 2024

@CuptainTeamo forget my current PR, its completely wrong. I'm preparing new solution. I will push it ASAP.

@semensanyok
Copy link
Contributor

@CuptainTeamo @clayjohn fixed, review #91853 please

@luciusponto
Copy link
Author

luciusponto commented May 12, 2024

Hi. I tested the changes in the PR above with the min repro project and it seems to work well now, both in Compatibility and Forward+.

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