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

ImageTextureLayered does not persist its state #66558

Closed
Ithamar opened this issue Sep 28, 2022 · 15 comments
Closed

ImageTextureLayered does not persist its state #66558

Ithamar opened this issue Sep 28, 2022 · 15 comments

Comments

@Ithamar
Copy link

Ithamar commented Sep 28, 2022

Godot version

4.0b1dev (commit c2f6664)

System information

Windows 10, Vulkan

Issue description

ImageTextureLayered and its subclasses do not save their "content" when asked by using ResourceSaver.save. No persistable properties are defined, so none are stored.

Steps to reproduce

The following code:

var i := Image.create(1, 1, false, Image.FORMAT_RGB8)
i.set_pixel(0,0, Color(1,0,0))
ResourceSaver.save(i, "res://image.tres")
var l := Texture2DArray.new()
l.create_from_images([ i ])
ResourceSaver.save(l, "res://layered.tres")

Results in layered.tres looking empty like this:

[gd_resource type="Texture2DArray" format=3]

[resource]

while image.tres shows the proper image was created (and saved):

[gd_resource type="Image" format=3]

[resource]
data = {
  "data": PackedByteArray(255, 0, 0),
  "format": "RGB8",
  "height": 1,
  "mipmaps": false,
  "width": 1
}

Minimal reproduction project

See code above.

@Ithamar
Copy link
Author

Ithamar commented Sep 28, 2022

I'd love to create a PR for this, but then I first have to understand how this is supposed to work. Since the whole TextureLayered code was completely reworked since 4.x, I have to idea what the (re)designer of this code had in mind for persistence. Any suggestions/hints would be greatly appreciated.

@Calinou
Copy link
Member

Calinou commented Sep 29, 2022

4.0

Which alpha/beta version are you using? 4.0 isn't released in stable form yet.

@Ithamar
Copy link
Author

Ithamar commented Sep 29, 2022

4.0

Which alpha/beta version are you using? 4.0 isn't released in stable form yet.

Yeah sorry for not specifying that, it is a master branch from a day or so ago, commit c2f6664

@Calinou
Copy link
Member

Calinou commented Sep 29, 2022

Related to #66192 and #54202 (likely due to the same cause).

@Calinou Calinou added this to the 4.0 milestone Sep 29, 2022
@clayjohn
Copy link
Member

Duplicate of #63025

As explained there, this:

var l := Texture2DArray.new()
l.create_from_images([ i ])

Should be:

var l = Texture2DArray.create_from_images([ i ])

@clayjohn clayjohn removed this from the 4.0 milestone Dec 13, 2022
@lewiji
Copy link
Contributor

lewiji commented Dec 13, 2022

Duplicate of #63025

As explained there, this:

var l := Texture2DArray.new()
l.create_from_images([ i ])

Should be:

var l = Texture2DArray.create_from_images([ i ])

Have discussed this on rocket chat, but adding a comment here for clarity - I don't believe the static create_from_images function has been implemented for the layered texture types yet (though I think they probably should be for consistency with changes in other Image-related classes).

In my tests, creating e.g. a Texture2DArray using similar code to the OP (i.e. non-static, modifying in-place l.create_from_images([i])) works fine in-memory - assigning the resulting Texture2DArray object to a shader uniform works as intended and can be sampled. It's when it's saved via ResourceSaver.save() that it appears to be saving an empty resource instead of serialising the layers.

@clayjohn clayjohn reopened this Dec 13, 2022
@clayjohn clayjohn added this to the 4.x milestone Dec 13, 2022
@Ithamar
Copy link
Author

Ithamar commented Dec 17, 2022

So, is there any suggested docs I should look at if I want to create an MR for this? My gut feeling says TextureLayered should simply have the layer images persisted, but I'm not sure that's an acceptable solution. Any guidance from Godot devs on this would be greatly appreciated.

This bug is really blocking me from seriously using Godot :(

@lewiji
Copy link
Contributor

lewiji commented Dec 17, 2022

Don't know if this is useful, but here's a valid TextureArray resource generated in Godot 3.5.1. It doesn't import/convert into Godot 4, and running the same code in Godot 4 produces that mostly empty resource.

It's just 3 128x128 textures from OpenGameArt. As you can see it's the raw image data stored as subresources, followed by some metadata about the TextureArray itself. Interestingly, the layers property of that metadata is set to [null, null, null]. I don't know if that's intentional or not, but I can still sample the layers of the generated resource in a shader correctly. Makes me wonder if that could be related to the Godot 4 issue where Godot 3 is handling those nulls in some way but Godot 4 finds them invalid? Pure speculation though.

stone_wall_texarray.zip

Truncated for brevity:

[gd_resource type="TextureArray" load_steps=4 format=2]

[sub_resource type="Image" id=1]
data = {
"data": PoolByteArray( 87, 81, 75, [... lots more values]),
"format": "RGBA8",
"height": 128,
"mipmaps": false,
"width": 128
}

[sub_resource type="Image" id=2]
data = {
"data": PoolByteArray( 119, 120, 109, [...]),
"format": "RGBA8",
"height": 128,
"mipmaps": false,
"width": 128
}

[sub_resource type="Image" id=3]
data = {
"data": PoolByteArray( 94, 95, 89, [...]),
"format": "RGBA8",
"height": 128,
"mipmaps": false,
"width": 128
}

[resource]
flags = 9
data = {
"depth": 3,
"flags": 9,
"format": 5,
"height": 128,
"layers": [ null, null, null ],
"width": 128
}

I was trying to figure out in Godot's source where in the ResourceSaver/ResourceFormatSaver system these get generated but I've had no luck so far.

@Ithamar
Copy link
Author

Ithamar commented Dec 17, 2022

The problem you describe above is 3.x specific, see my PR #70212 for a fix. master ("4.x") currently has no serialisation of Texture2DArray implemented at all :(

I was trying to figure out in Godot's source where in the ResourceSaver/ResourceFormatSaver system these get generated but I've had no luck so far.

I agree it isn't very obvious, and it is lacking documentation (serialisation in general). If you look at the C++ classes mapping to the GDScript classes you can find where it defines the "public" GDScript attributes which are serialised automagically.

@Ithamar
Copy link
Author

Ithamar commented Dec 17, 2022

Okay, I think I have a fix for master too, seems to work here locally, but I want to wait for the results of PR #70212 first, as they share the core changes, and I want to first be sure those are okay to commit.

Anyway, hopefully this will finally be the end of all the TextureLayered woes with Godot, and I can go back to hacking on my game instead of Godot 😄

@lewiji
Copy link
Contributor

lewiji commented Dec 17, 2022

The problem you describe above is 3.x specific, see my PR #70212 for a fix. master ("4.x") currently has no serialisation of Texture2DArray implemented at all :(

Ah, I didnt realise it was actually a problen in 3.x - just that porting my game from 3.x to 4 indeed means I'm blocked by the broken serialization, I just posted the 3.x as an example in case it gave someone some insight into why it wasn't working in 4. Sounds like you nailed it though, I will keep an eye out for the 4 PR as I can finally continue my porting :)

@Ithamar
Copy link
Author

Ithamar commented Dec 17, 2022

Sounds like you nailed it though, I will keep an eye out for the 4 PR as I can finally continue my porting :)

If you feel experimental enough, the 4.x commit that fixes it for me is here: Game3DEE@68bb829, feel free to grab it, but this is only tested with the above script, so YMMV.

so, with the above commit, the layered.tres now looks like:

[gd_resource type="Texture2DArray" load_steps=2 format=3]

[sub_resource type="Image" id="Image_6yw58"]
data = {
"data": PackedByteArray(255, 0, 0),
"format": "RGB8",
"height": 1,
"mipmaps": false,
"width": 1
}

[resource]
_images = [SubResource("Image_6yw58")]

and has the correct (red square) preview in the Inspector.

@lewiji
Copy link
Contributor

lewiji commented Dec 19, 2022

@Ithamar I just compiled master with your patch and can confirm, it fixed my problem with Texture2DArray saving - worked perfectly! Thanks!

@Ithamar
Copy link
Author

Ithamar commented Dec 19, 2022

@Ithamar I just compiled master with your patch and can confirm, it fixed my problem with Texture2DArray saving - worked perfectly! Thanks!

Nice! I'll clean it up and post a PR for it as well tonight, so we can get all the Texture2DArray stuff back working again 🎊

@clayjohn
Copy link
Member

Fixed by #71394

@clayjohn clayjohn modified the milestones: 4.x, 4.2 Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants