jMonkeyEngine / jmonkeyengine

A complete 3-D game development suite written in Java.
http://jmonkeyengine.org
BSD 3-Clause "New" or "Revised" License
3.84k stars 1.13k forks source link

Image class wrongly set mips flags inside the constructor #1883

Closed Ali-RS closed 1 year ago

Ali-RS commented 1 year ago

These two constructors on Image class behave differently!

https://github.com/jMonkeyEngine/jmonkeyengine/blob/f92a73f91ce1a2852645cd1bb5a4c6b597faf7af/jme3-core/src/main/java/com/jme3/texture/Image.java#L749-L762

https://github.com/jMonkeyEngine/jmonkeyengine/blob/f92a73f91ce1a2852645cd1bb5a4c6b597faf7af/jme3-core/src/main/java/com/jme3/texture/Image.java#L805-L816

Look at needGeneratedMips and mipsWereGenerated flags, if mipMapSizes is null the first one won't modify the flags but the second one will do (I belive it is wrongly setting mipsWereGenerated = true)

Forum post: https://hub.jmonkeyengine.org/t/issue-in-image-class-bug/46297

zzuegg commented 1 year ago

I wonder if needGeneratedMips should be set to true in the second constructor. I would kind of expect that if i only pass the wanted mip sizes they would be generated.

zzuegg commented 1 year ago

If i follow the code in GLRenderer correctly, mipsWhereGenerated is needed to set to true if the miplevels are generated on the gpu, (they can be trusted to be correct) if not, it makes some checks.

Following that, it should actually set mipsWereGenerated to false in the first constructor(Since they are passed manually), but needGeneratedMips to true in the second one (Since we actually want mips to be generated)

Ali-RS commented 1 year ago

Looked a bit deeper into this

The first constructor which takes ArrayList<ByteBuffer> data seems to be used for texture array and the second constructor which takes ByteBuffer data is for regular images.

if we pass an int[] mipMapSizes it means we have generated mips. All mips data must be packed inside the ByteBuffer data.

This is how mips are generated and stored for an image:

https://github.com/jMonkeyEngine/jmonkeyengine/blob/ac8c1012d2ded0812d91a393f935c9f0f7d0c192/jme3-core/src/main/java/com/jme3/util/MipMapGenerator.java#L96-L131

GLRenderer uses TextureUtil to upload a texture to GPU, there if an image has mips packed inside it it will be extracted and uploaded to GPU:

https://github.com/jMonkeyEngine/jmonkeyengine/blob/930809e749e3e7841e0f874bdd6909c514703a10/jme3-core/src/main/java/com/jme3/renderer/opengl/TextureUtil.java#L245-L301

If we do not pass int[] mipMapSizes it means there is no mips packed inside image data.

I wonder if needGeneratedMips should be set to true in the second constructor. I would kind of expect that if i only pass the wanted mip sizes they would be generated.

So this is not true.