Hi, I noticed the TextureArray class does not implement write() and read() and so when I serialize to .j3o and read back it does no longer wrap in the same way as I set it.
I compared with Texture2D and that one does implement these methods:
Yeah if you open a PR I’d be glad to review it and try to have it merged for 3.8
I use texture arrays a lot in my app, but I was not able to get saving/loading to work at all actually, so currently I am regenerating my TextureArrays everytime the app is launched.
Maybe I was doing something wrong, but if I recall correctly, whenever I tried saving a TextureArray to .j3o it was just creating a 1kb size j3o that didn’t appear to actually save the texture array’s image and it just looked like it had saved an empty j3o file.
Aside from the missing wrap mode, does everything else work for you when saving and loading a texture array to j3o?
The code seems to be there to write the image data. If for some reason the texture is given an asset key then it won’t (because it presumes that was loaded as an asset and exists elsewhere).
Another difference with Texture2D is that TextureArray doesn’t override equals/hashCode.
Not sure if those are needed too, I didn’t add them to keep it short.
This must’ve been what I was doing wrong. I remember I was specifically setting an asset key before saving, so I’ll have to go back and try again now that I know, thanks for the tip!
It would probably be good to add these but its okay if you want to keep that PR narrowed to just the read/write issue you’ve encountered. Thanks for submitting a PR
Okey, I added them as a new commit to the PR.
But I found a problem when trying to write a test for it, I don’t think it really works to compare this class.
In the parent class Texture it assumes that the Image was instantiated by the asset manager:
public boolean equals(Object obj) {
...
// NOTE: Since images are generally considered unique assets in jME3,
// using the image's equals() implementation is not necessary here
// (would be too slow)
if (this.image != other.image) {
return false;
}
However in this class the Image is created by the constructor:
public TextureArray(List<Image> images) {
...
Image arrayImage = new Image(format, width, height, null, colorSpace);
...
so the equals() always returns false.
If you think it’s a good idea I can try to re-implement the equals() to not call super.equals() and make the comparison. But I don’t know if it’s a good idea cause it will be forced to compare the byte buffers of the image. Is that desired?
Here’s a simple test:
@Test
public void testEquals() {
List<Image> images = new ArrayList<>();
images.add(createImage());
images.add(createImage());
TextureArray tex3 = new TextureArray(images);
tex3.setWrap(WrapMode.Repeat);
TextureArray tex4 = new TextureArray(images);
tex4.setWrap(WrapMode.Repeat);
assertEquals(tex3, tex4);
}
Yeah this should be okay. Comparing the whole image buffers is likely the only way to check for an accurate equals() result in this situation, since the TextureArray only stores the final image, and it doesn’t store any other information about the orgiinal textures/images used to create the array.
It may end up taking a few ms longer to finish the equals() check, especially for high resolution textureArrays with many textures, but I’d say that having a working (albeit potentially slow) equals() method is better than not having it at all.
If you don’t call super.equals() then you will need to leave a giant 40 story comment in both places about how any time Texture changes you have to remember to also go and change TextureArray.
Can anyone think of a use-case where binary-level TextureArray.equals() is useful?
…I’m beginning to think there was a reason that these weren’t implemented for this class.
I think the side effect is that every time a user tries to batch a subgraph with a couple TextureArrays that things will grind to a halt as it tries to find associated materials. Better to just let it use == for TextureArray, I think.
You make a good point. I’d say that the answer is most likely no.
Texture Arrays are already pretty big and memory intensive, so if someone is creating two identical texture arrays that would return true for an equals() check that compares the full image buffers, then they probably are already taking a bad approach and should just keep one instance of that texture array instead.
So I’d say you are right on this, it is probably best to just let it do a basic == check for the comparison.