Fix ImageTextureLayered serialization issues

[Ithamar]

This is a master branch version of PR #70212 , fixing many of the issues with serialisation for ImageTextureLayered subclasses.

NOTE: this commit was very much inspired by commit V-Sekai@2ba3561, authored by @SaracenOne that I was made aware of by @Calinou in issue #54202

It needed some more rework due to code of 3.x and 4.x diverging in relevant areas.

[Kwu564]

Does this also fix serialization for texture3Ds? I've applied these changes on top of master and I'm finding that I'm unable to save them. Texture2DArray also now has a create_placeholder() in place of create(); I'm guessing there is some work in progress going on for texture arrays.

[Ithamar]

This PR fixes issues related to (Image)TextureLayered and its subclasses, and Texture3D is a subclass of Texture directly, so I doubt this will have much impact.

I'm waiting for someone to review this patch, if the approach is ok, I'm more than willing to update the PR to latest master.

[JohanAR]

Perhaps I misunderstand what this is supposed to do, but it does not appear to solve my problem.

In my project I have a ShaderMaterial with a Texture2DArray parameter. Using a @tool script I can run a function which loads a bunch of images, creates a Texture2DArray from them, and assigns them as a shader parameter. This works perfectly fine in the editor, but if I play the scene or close it and open the scene again, then all the textures are gone.

If I run the same function in the scene's _ready function it works in-game as well, but it takes a lot of time so I'd much rather save the Texture2DArray and load it instead.

[Ithamar]

Perhaps I misunderstand what this is supposed to do, but it does not appear to solve my problem.

In my project I have a ShaderMaterial with a Texture2DArray parameter. Using a @tool script I can run a function which loads a bunch of images, creates a Texture2DArray from them, and assigns them as a shader parameter. This works perfectly fine in the editor, but if I play the scene or close it and open the scene again, then all the textures are gone.

If I run the same function in the scene's _ready function it works in-game as well, but it takes a lot of time so I'd much rather save the Texture2DArray and load it instead.

Okay, could you try saving the Texture2DArray resource and loading it again? You could also check the .tres file saved to see if there's any data in there, as before this change it was basically empty (no image data saved).

If the .tres file looks okay, it is probably a different bug with shader parameter persistence I suspect.

[JohanAR]

I cherry picked your "In-progress fix for TextureLayered in 4.0" commit and rebuilt Godot.

The I run a function (from a tool script) which boils down to:

	var img_tex2d_array := Texture2DArray.new()
	img_tex2d_array.create_from_images(img_array)
	heightmap_material.set_shader_parameter("textures", img_tex2d_array)

This works perfectly well in the editor. Now I also saved the resource from the editor, but it only created an empty file:

[gd_resource type="Texture2DArray" format=3 uid="uid://hrywckxde6tw"]

[resource]

Same thing with a Texture3D that I also build from code. I will try using your MRP from the original issue and see if that works better.

[JohanAR]

Tried the example snippet from #66558 and it still does not work. layered.tres is still empty

Has anyone tried this rebased on latest master? If it works for most other people, perhaps it's platform related? I use Linux

[Ithamar]

I haven't tried this on any recent release, as soon as I have some time available I'll rebase and test.

[JohanAR]

I'll have a quick look at the code to see if I can figure out why it isn't working for me.

Also just noticed that Texture3D is not a subclass of TextureLayered, so I assume that would need a similar fix. However I did try with Texture2DArray first, which I believe ought to be covered by this.

I'm on dev chat and Godot discord with same username if there's something you want me to test btw

[JohanAR]

Sooo, I realised that this PR is from your feat-imagetexlay-ser branch, while I had cherry-picked the commit from your fix-texturelayered-40 branch.. Got the right commit now, and with a trivial merge conflict it's now working for Texture2DArray. 👍

Do you want to include similar fixes for ImageTexture2D and Texture3D in the same commit btw?

[Ithamar]

Sooo, I realised that this PR is from your feat-imagetexlay-ser branch, while I had cherry-picked the commit from your fix-texturelayered-40 branch.. Got the right commit now, and with a trivial merge conflict it's now working for Texture2DArray. 👍

Ah yeah, I should really clean up my branches in there, it took a couple of different attempts to get this working ;)

Do you want to include similar fixes for ImageTexture2D and Texture3D in the same commit btw?

Are you sure ImageTexture2D is broken too? I haven't explicitly tested but I would expect a lot things to break if that's broken too.

Texture3D could likely be fixed in the same way, I just haven't had a use case for it. If it is basically the same issue I'd vote for including it, seeing how long it is taking this thing to merge. Not having others waiting this long for a Texture3D fix would be enough reason imho ;)

[JohanAR]

I added the following to your example:

	var t := ImageTexture.new()
	t.create_from_image(i)
	ResourceSaver.save(t, "res://texture.tres")

And the resulting resource file was empty. I'm guessing that all resources created by the editor use different methods, and creating+saving resources from gdscript might be more of a niche use case, but I haven't looked into it.

It seems like Texture3D has a slightly different API than ImageTextureLayered, and while it is still created from an array of Images, it does not have an images property. Hopefully it's not difficult to add that, but perhaps one of the core devs need to approve it.

If it's not that interesting to you then I can try to make a PR for serializing Texture3D, since I need that too.

[Ithamar]

I added the following to your example:

	var t := ImageTexture.new()
	t.create_from_image(i)
	ResourceSaver.save(t, "res://texture.tres")

And the resulting resource file was empty. I'm guessing that all resources created by the editor use different methods, and creating+saving resources from gdscript might be more of a niche use case, but I haven't looked into it.

Actually, the native code goes through the same path as the GDScript one. The problem with the above code is that ImageTexture.create_from_image is a static method, returning the new ImageTexture object, so the above code should be

	var t := ImageTexture.create_from_image(i)
	ResourceSaver.save(t, "res://texture.tres")

and that should give you a proper tres file.

It seems like Texture3D has a slightly different API than ImageTextureLayered, and while it is still created from an array of Images, it does not have an images property. Hopefully it's not difficult to add that, but perhaps one of the core devs need to approve it.

If it's not that interesting to you then I can try to make a PR for serializing Texture3D, since I need that too.

I'm hoping to have some time over the weekend, rebasing this PR and adding the Texture3D support here too. If that doesn't happen, you might be better off creating a new PR for that ;)

[JohanAR]

The problem with the above code is that ImageTexture.create_from_image is a static method

Ahh, that's confusing that it's different from Texture2DArray.. Would be nice if it was possible for Godot to give a warning when a statement has no effect, but I guess that would require a lot of work.

I'm hoping to have some time over the weekend, rebasing this PR and adding the Texture3D support here too.

Looking forward to it! It's getting annoying to regenerate my textures all the time :)

[clayjohn]

Ahh, that's confusing that it's different from Texture2DArray.. Would be nice if it was possible for Godot to give a warning when a statement has no effect, but I guess that would require a lot of work.

Heh, we added a warning actually but we have it disabled by default as it was annoying too many users

[clayjohn]

Tested locally and it appears to work! The changes make sense and are quite minimal, so I think this should be good to merge for 4.2

That being said, I think its best not to cherrypick this for 4.1.x

[clayjohn]

This PR will just need a quick rebase before it is ready to merge as the texture.cpp file was split up. The change in that file just needs to be moved to image_texture.cpp

[Ithamar]

This PR will just need a quick rebase before it is ready to merge as the texture.cpp file was split up. The change in that file just needs to be moved to image_texture.cpp

Thanks for the heads-up, I've just rebased the PR so it should be good to merge now. I had almost given up this would be merged, happy to see it will most likely still be picked up!

[YuriSizov]

Thanks! And congrats on your first merged Godot pull request!