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

ImageTexture3D serialization bug #80866

Closed
Mohamed-Kr opened this issue Aug 21, 2023 · 3 comments · Fixed by #82055
Closed

ImageTexture3D serialization bug #80866

Mohamed-Kr opened this issue Aug 21, 2023 · 3 comments · Fixed by #82055
Milestone

Comments

@Mohamed-Kr
Copy link

Mohamed-Kr commented Aug 21, 2023

Godot version

4.1 stable and 4.2.dev3

System information

Godot v4.1.stable - Windows 10.0.22621 - Vulkan (Mobile) - integrated Intel(R) Iris(R) Plus Graphics (Intel Corporation; 27.20.100.9621) - Intel(R) Core(TM) i7-1065G7 CPU @ 1.30GHz (8 Threads)

Issue description

When I create a ImageTexture3D, populate it and then save it I obtain a 1x1x1 Lum8 pink texture instead of the one I created (I think it is properly created beacause it behaves well when I use it in a custom shader).

This happens whether

  • it is running as tool or not
  • I try to save in a res or in a tres file
  • I put flags in the ResourceSaver.save function or not

and it don't give errors.

And when It is in a shader uniform in editor, working properly - If I save the scene and reopen it, there's pink where the texture is involved and the 1x1x1 in the uniform field.

Steps to reproduce

  • Create a project and a scene
  • Give the root the script below
  • Close the scene and reopen it to make sure that @tool takes effect
  • Try running the scene or checking the save boolean field in the inspector
  • Check if the ImageTexture3D got saved properly in the FileSystem Dock
@tool
extends Node

@export var save := false :
	set(v) :
		ResourceSaver.save(_get_grad_tex(), "res://tex3D.res")
		ResourceSaver.save(_get_grad_tex(), "res://tex3D.tres")

func _ready() -> void :
	if Engine.is_editor_hint() : return
	ResourceSaver.save(_get_grad_tex(), "res://tex3D.tres")
	ResourceSaver.save(_get_grad_tex(), "res://tex3D.res")

func _get_grad_tex() -> ImageTexture3D:
	var res := ImageTexture3D.new()
	var imgs : Array[Image] = [] 
	for i in 10 :
		var img := Image.create(10, 10, false, Image.FORMAT_RGB8)
		imgs.push_back(img)
	for x in 10 :
		for z in 10 :
			for y in 10 :
				imgs[z].set_pixel(x, y, Color(x/10., y/10., z/10.))
	res.create(Image.FORMAT_RGB8, 10, 10, 10, false, imgs )
	return res

Minimal reproduction project

N/A

@clayjohn
Copy link
Member

clayjohn commented Aug 22, 2023

Looks like we need to port #71394 to ImageTexture3D as well.

Here is a partially working diff:

diff --git a/scene/resources/image_texture.cpp b/scene/resources/image_texture.cpp
index ecf70d96ac..a4ff009a80 100644
--- a/scene/resources/image_texture.cpp
+++ b/scene/resources/image_texture.cpp
@@ -462,9 +462,51 @@ void ImageTexture3D::set_path(const String &p_path, bool p_take_over) {
        Resource::set_path(p_path, p_take_over);
 }
 
+TypedArray<Image> ImageTexture3D::_get_images() const {
+       Vector<Ref<Image>> raw_images = get_data();
+       ERR_FAIL_COND_V(raw_images.is_empty(), TypedArray<Image>());
+       TypedArray<Image> images;
+       for (int i = 0; i < get_depth(); i++) {
+               images.push_back(raw_images[i]);
+       }
+       return images;
+}
+
+void ImageTexture3D::_set_images(const TypedArray<Image> &p_images) {
+       int new_layers = p_images.size();
+       ERR_FAIL_COND(new_layers == 0);
+       Ref<Image> img_base = p_images[0];
+       ERR_FAIL_COND(img_base.is_null());
+
+       Image::Format new_format = img_base->get_format();
+       int new_width = img_base->get_width();
+       int new_height = img_base->get_height();
+       int new_depth = 0;
+       bool new_mipmaps = false;
+
+       for (int i = 1; i < p_images.size(); i++) {
+               Ref<Image> img = p_images[i];
+               ERR_FAIL_COND(img.is_null());
+               ERR_FAIL_COND_MSG(img->get_format() != new_format, "All images must share the same format");
+               if (img->get_width() != new_width || img->get_height() != new_height) {
+                       new_mipmaps = true;
+                       if (new_depth == 0) {
+                               new_depth = i;
+                       }
+               }
+       }
+
+       ERR_FAIL_COND(_create(new_format, new_width, new_height, new_depth, new_mipmaps, p_images) != OK);
+}
+
void ImageTexture3D::_bind_methods() {
        ClassDB::bind_method(D_METHOD("create", "format", "width", "height", "depth", "use_mipmaps", "data"), &ImageTexture3D::_create);
        ClassDB::bind_method(D_METHOD("update", "data"), &ImageTexture3D::_update);
+
+       ClassDB::bind_method(D_METHOD("_get_images"), &ImageTexture3D::_get_images);
+       ClassDB::bind_method(D_METHOD("_set_images", "images"), &ImageTexture3D::_set_images);
+
+       ADD_PROPERTY(PropertyInfo(Variant::ARRAY, "_images", PROPERTY_HINT_ARRAY_TYPE, "Image", PROPERTY_USAGE_INTERNAL | PROPERTY_USAGE_STORAGE | PROPERTY_USAGE_RESOURCE_NOT_PERSISTENT), "_s
et_images", "_get_images");
 }
 
 ImageTexture3D::ImageTexture3D() {
diff --git a/scene/resources/image_texture.h b/scene/resources/image_texture.h
index 9d9c296a45..3b756fb593 100644
--- a/scene/resources/image_texture.h
+++ b/scene/resources/image_texture.h
@@ -137,6 +137,9 @@ class ImageTexture3D : public Texture3D {
        int depth = 1;
        bool mipmaps = false;
 
+       TypedArray<Image> _get_images() const;
+       void _set_images(const TypedArray<Image> &p_images);
+
 protected:
        static void _bind_methods();

@jsjtxietian
Copy link
Contributor

jsjtxietian commented Sep 2, 2023

@clayjohn The code here posted here don't work on the mrp because this code regarding mimmaps in set_image:

if (img->get_width() != new_width || img->get_height() != new_height) {
	new_mipmaps = true;
	if (new_depth == 0) {
		new_depth = i;
	}
}

When the texture3D doesn't have a mipmap, new_depth should be p_images.size(). get_images should take mipmaps into account too.

After fixing this, the code works well like the image below

image

I'm happy to continue digging if you allow me to continue based on your work :)

@jsjtxietian
Copy link
Contributor

jsjtxietian commented Sep 9, 2023

The code still triger two errors in editor console because of some resource init code will read the texture array's info before it actually deserilize it.

These two errors come from:
ResourceLoaderText::load() 's Variant get_value = resource->get(assign, &is_get_valid) will eventually call texture3d's _get_images and get_data, but the texture3d is not inited with its data yet ;

after it, the _set_image will be called at resource->set(assign, value) and it will be inited correctly.

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

Successfully merging a pull request may close this issue.

5 participants