OpenGL: Implement 3D Texture support

[alula]

Bugsquad edit:

• Fixes OpenGL: TextureArrays and Texture3D samplers not implemented yet #71029

[akien-mga]

Should fix #71029 I suppose?

[alula]

This is for 4.x compatibility renderer, so 3.x milestone sounds like a mistake...

[clayjohn]

I am getting the following error just opening up the project manager with this PR:

ERROR: Image has included mipmaps
   at: texture_3d_initialize (drivers/gles3/storage/texture_storage.cpp:816)
ERROR: Image has included mipmaps
   at: texture_3d_initialize (drivers/gles3/storage/texture_storage.cpp:816)

I traced it back to our default texture generation which reuses images that already have mipmaps to create the default images. The following diff resolves the errors:

diff --git a/drivers/gles3/storage/texture_storage.cpp b/drivers/gles3/storage/texture_storage.cpp
index 268043f99f..ef16a52b3a 100644
--- a/drivers/gles3/storage/texture_storage.cpp
+++ b/drivers/gles3/storage/texture_storage.cpp
@@ -77,19 +77,24 @@ TextureStorage::TextureStorage() {
 			default_gl_textures[DEFAULT_GL_TEXTURE_2D_ARRAY_WHITE] = texture_allocate();
 			texture_2d_layered_initialize(default_gl_textures[DEFAULT_GL_TEXTURE_2D_ARRAY_WHITE], images, RS::TEXTURE_LAYERED_2D_ARRAY);
 
-			for (int i = 0; i < 3; i++) {
+			for (int i = 0; i < 5; i++) {
 				images.push_back(image);
 			}
 
-			default_gl_textures[DEFAULT_GL_TEXTURE_3D_WHITE] = texture_allocate();
-			texture_3d_initialize(default_gl_textures[DEFAULT_GL_TEXTURE_3D_WHITE], image->get_format(), 4, 4, 4, false, images);
+			default_gl_textures[DEFAULT_GL_TEXTURE_CUBEMAP_WHITE] = texture_allocate();
+			texture_2d_layered_initialize(default_gl_textures[DEFAULT_GL_TEXTURE_CUBEMAP_WHITE], images, RS::TEXTURE_LAYERED_CUBEMAP);
+		}
+
+		{
+			Ref<Image> image = Image::create_empty(4, 4, false, Image::FORMAT_RGBA8);
+			image->fill(Color(1, 1, 1, 1));
 
-			for (int i = 0; i < 2; i++) {
+			Vector<Ref<Image>> images;
+			for (int i = 0; i < 4; i++) {
 				images.push_back(image);
 			}
-
-			default_gl_textures[DEFAULT_GL_TEXTURE_CUBEMAP_WHITE] = texture_allocate();
-			texture_2d_layered_initialize(default_gl_textures[DEFAULT_GL_TEXTURE_CUBEMAP_WHITE], images, RS::TEXTURE_LAYERED_CUBEMAP);
+			default_gl_textures[DEFAULT_GL_TEXTURE_3D_WHITE] = texture_allocate();
+			texture_3d_initialize(default_gl_textures[DEFAULT_GL_TEXTURE_3D_WHITE], image->get_format(), 4, 4, 4, false, images);
 		}
 
 		{ // black
@@ -101,19 +106,23 @@ TextureStorage::TextureStorage() {
 			texture_2d_initialize(default_gl_textures[DEFAULT_GL_TEXTURE_BLACK], image);
 
 			Vector<Ref<Image>> images;
-
-			for (int i = 0; i < 4; i++) {
+			for (int i = 0; i < 6; i++) {
 				images.push_back(image);
 			}
+			default_gl_textures[DEFAULT_GL_TEXTURE_CUBEMAP_BLACK] = texture_allocate();
+			texture_2d_layered_initialize(default_gl_textures[DEFAULT_GL_TEXTURE_CUBEMAP_BLACK], images, RS::TEXTURE_LAYERED_CUBEMAP);
+		}
 
-			default_gl_textures[DEFAULT_GL_TEXTURE_3D_BLACK] = texture_allocate();
-			texture_3d_initialize(default_gl_textures[DEFAULT_GL_TEXTURE_3D_BLACK], image->get_format(), 4, 4, 4, false, images);
+		{
+			Ref<Image> image = Image::create_empty(4, 4, false, Image::FORMAT_RGBA8);
+			image->fill(Color(0, 0, 0, 1));
 
-			for (int i = 0; i < 2; i++) {
+			Vector<Ref<Image>> images;
+			for (int i = 0; i < 4; i++) {
 				images.push_back(image);
 			}
-			default_gl_textures[DEFAULT_GL_TEXTURE_CUBEMAP_BLACK] = texture_allocate();
-			texture_2d_layered_initialize(default_gl_textures[DEFAULT_GL_TEXTURE_CUBEMAP_BLACK], images, RS::TEXTURE_LAYERED_CUBEMAP);
+			default_gl_textures[DEFAULT_GL_TEXTURE_3D_BLACK] = texture_allocate();
+			texture_3d_initialize(default_gl_textures[DEFAULT_GL_TEXTURE_3D_BLACK], image->get_format(), 4, 4, 4, false, images);
 		}
 
 		{ // transparent black
@@ -813,6 +822,7 @@ void TextureStorage::texture_3d_initialize(RID p_texture, Image::Format p_format

[alula]

done

[clayjohn]

This looks mostly good. I left a comment on the validation code as it will need some changes.

Another thing to consider is implementing texture_3d_get() without it, NoiseTexture3Ds crash on creation. Please let me know if you would like some help with implementing texture_3d_get() as it is a little more complex

[alula]

NoiseTexture3Ds seem to work fine now and implementing texture_2d_layer_get() and texture_3d_get() is straightforward on OpenGL due to glGetTexImage() availability but a bit tricky on GLES.

For me it looks like it would need a variant of the copy.glsl shader with sampler3D, what would be your preferred way to implement that?

[clayjohn]

I would do something similar to how it was done in 3.x

godot/drivers/gles3/rasterizer_storage_gles3.cpp

Line 1039 in 4461f38

if (texture->type == VS::TEXTURE_TYPE_2D_ARRAY || texture->type == VS::TEXTURE_TYPE_3D) {

[alula]

That should be everything I guess?

[alula]

I've applied the code review changes in separate commit, but it's not showing up here for me for some reason?

[alula]

GitHub moment

[alula]

rebased

[alula]

Fixed an issue I found while testing my project on Web target, any updates on this PR by the way?

[QbieShay]

Hey 👋

This has been postponed to 4.3 at the time of 4.2, are you still available to update this PR @alula ?

Please let us know so we can organise and align on the implementation ^^

[alula]

Oh yes, I'm still around, I'll update it soon!

[clayjohn]

Hi! This has become a blocker for other work that needs to get finished on the compatibility backend. So I'm going to make the necessary changes and push directly to this PR

[alula]

@clayjohn alright, I've been busy with irl stuff and went on trip after my last comment and totally forgot about finishing this PR, oops... 😅

[clayjohn]

@clayjohn alright, I've been busy with irl stuff and went on trip after my last comment and totally forgot about finishing this PR, oops... 😅

No worries!

[alula]

rebased

[clayjohn]

Hey, thanks for rebasing and updating based on my last comment.

Right after commenting earlier I realized we could get by a little longer without 3D textures. So thank you for taking the initiative back and finishing this PR.

It looks great to me overall. The final step before merging it is to "squash" all the commits into one commit. We describe how to that here: https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html#the-interactive-rebase

We like to have only one commit per PR in order to keep our git history clean and easy to navigate. Please let me know if you run into any trouble or want me to finish it off for you. I feel bad saying I would take over and then leaving it to you to finish again.

[alula]

done

Hey, thanks for rebasing and updating based on my last comment.

Right after commenting earlier I realized we could get by a little longer without 3D textures. So thank you for taking the initiative back and finishing this PR.

It looks great to me overall. The final step before merging it is to "squash" all the commits into one commit. We describe how to that here: https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html#the-interactive-rebase

We like to have only one commit per PR in order to keep our git history clean and easy to navigate. Please let me know if you run into any trouble or want me to finish it off for you. I feel bad saying I would take over and then leaving it to you to finish again.

[akien-mga]

Thanks!