GLRenderer.modifyTexture on GLES fix

[joliver82]

As I commented in forum thread https://hub.jmonkeyengine.org/t/niftygui-some-controls-have-bad-rendering-in-android-having-batch-enabled/42641 all calls to GLRenderer.modifyTexture having different formats of source and destination texture done in GLES context fails with a GL_INVALID_EXCEPTION because glTexSubImage2D on GLES doesn't support it.

This PR adds a method in Image class to convert among formats, currently just rgb8 to rgba8 and back which I think are the most common cases for this and modifies GLRenderer.modifyTexture to call it if required

[stephengold]

Looks satisfactory to me, but I'd like to hear from other who know more.

[riccardobl]

Mhhh, we can't do that.
modifyTexture should not change the source image.
And it shouldn't replace/destroy the internal data buffer, this is not something safe to do, since we do not know what the buffer is and where it comes from or if it is safe to destroy.

I think the method should only throw an exception if the formats do not match, and then let the developer handle conversions depending on the situation.

[pspeed42]

Agreed. I was going to say the same thing.

We don't know what the application is doing with this image. They might be updating the buffer, for example. Only the application knows so only the application can determine when/if it is safe to convert.

[pspeed42]

Also, I could swear we already image conversion implemented somewhere... and directly on Image is not a good place for it. It should be external and take one Image and produce another.

[joliver82]

I agree also, thinking about it I realized that it can be risky to change the image this way as it could be used by other textures for example.

There's an exception on this, in the case of niftygui at least using xmls you don't have code control to convert the image before it's added to the atlas, in fact worst still, nifty has the hardcoded

thePlainImage = (BatchRenderImage) createImage("de/lessvoid/nifty/render/batch/nifty.png", true);

So if we just throw an exception if formats don't match every nifty android user will receive it and no app using batched nifty will work on android at all. Instead I would output a warning log or something similar instead. Do you agree?

I'll check the JmeBatchRenderBackend just in case I could create a new image with the appropiate format in loadImage or addImageToAtlas methods (which in the end will call GLRenderer.modifyTexture). Fixing this for nifty but allowing the developers to do what they want with the images.

Also, @pspeed42 I didn't find any image conversion method, do you remember where it is? I'll look for it and if not found I'll move mine from Image class to other place or maybe keep the new convert method but return a new image keeping the current as it is now 🤔

EDIT:

For the case of nifty batch, it's safe to add the gles check and conversion just in public Image loadImage(final String filename) from any other format to rgba8

because public Image loadImage(final ByteBuffer imageData,.... and the texture atlas image use RGBA8 format

And for the case of any other texSubImage call, we leave the developers make what they want

[pspeed42]

Which loadImage()? For the general loadImage(), you also wouldn't want to autoconvert because apps might be expecting a particular format.

Or is this in the JME-nifty adapter? (Which is where any nifty-specific changes should be.)

[pspeed42]

And as for existing conversions, I may have been thinking about the ImageCodec stuff that was written for ImageRaster. I thought there was a utility method to use it to copy one image format to another but I can't find it in 5 seconds of glancing around.

[joliver82]

Which loadImage()? For the general loadImage(), you also wouldn't want to autoconvert because apps might be expecting a particular format.

Or is this in the JME-nifty adapter? (Which is where any nifty-specific changes should be.)

Yes, I was talking about the loadImage in JmeBatchRenderBackend class in jme3-nifty package

[joliver82]

And as for existing conversions, I may have been thinking about the ImageCodec stuff that was written for ImageRaster. I thought there was a utility method to use it to copy one image format to another but I can't find it in 5 seconds of glancing around.

I didn't find any conversion method and I spent much more time looking around... :(

[Ali-RS]

This may help: https://github.com/NemesisMate/util-jme3/blob/master/util-jme3-base/src/main/java/com/nx/util/jme3/base/ImageUtil.java

[riccardobl]

You should be able to use ImageRaster for that

[joliver82]

I finally reverted the previous change, implemented the conversion using ImageRaster directly in JmeBatchRenderBackend when needed and added a warning to the modifyTexture methods

[riccardobl]

This looks good, nothing obviously wrong. I would wait for someone else who knows nifty to approve. But for me it's OK.