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

Add support for ImageTexture3D serialization #82055

Conversation

jsjtxietian
Copy link
Contributor

@jsjtxietian jsjtxietian commented Sep 21, 2023

Fixes #80866

In the issue comment, @clayjohn posted a partially working diff to support ImageTexture3D serilization. I took it and modifed the code to make it work correctly (at least in the MRP provided).

If anyone needs this function, feel free to take it and help test this code. I would appreciate any feedback or alternative suggestions.

scene/resources/image_texture.cpp Outdated Show resolved Hide resolved
scene/resources/image_texture.cpp Outdated Show resolved Hide resolved
@AThousandShips AThousandShips added this to the 4.2 milestone Sep 21, 2023
@jsjtxietian jsjtxietian force-pushed the add-support-for-texture3d-serilization branch from 45c81ab to 5d998a9 Compare September 21, 2023 15:04
@fire fire changed the title Add support for ImageTexture3D serilization Add support for ImageTexture3D serialization Sep 21, 2023
@fire fire requested review from a team September 22, 2023 00:03
@bitsawer
Copy link
Member

Those errors might come when the binary or text resource loader tries to get the property default value (ClassDB::class_get_default_property_value()). Adding a simple test to _get_images() might be enough as we are lazily allocating texture RID's and returning an empty array is fine. So it would become like this:

TypedArray<Image> ImageTexture3D::_get_images() const {
	TypedArray<Image> images;
	if (texture.is_valid()) {
		Vector<Ref<Image>> raw_images = get_data();
		ERR_FAIL_COND_V(raw_images.is_empty(), TypedArray<Image>());

		for (int i = 0; i < raw_images.size(); i++) {
			images.push_back(raw_images[i]);
		}
	}
	return images;
}

ImageTextureLayered might possibly need a similar check if used in a same manner as the ImageTexture3D in the issue code snippet, but that would need some more testing and is a bit out of scope of this PR.

Kind of speculating here, so needs testing for confirmation.

@jsjtxietian jsjtxietian force-pushed the add-support-for-texture3d-serilization branch from 5d998a9 to d9d2bb3 Compare September 22, 2023 08:25
@jsjtxietian
Copy link
Contributor Author

Thanks! It solved my problem.

Copy link
Member

@bitsawer bitsawer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally on top of current master (9e455f4), seems to work and fixes the linked issue and serialization.

@clayjohn might still want to take a final look as it is mostly his suggested code.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me. This isn't really my area of expertise, but I also tested locally and it appears to work fine.

@akien-mga akien-mga merged commit e02e38d into godotengine:master Oct 24, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

ImageTexture3D serialization bug
5 participants