enki-community / enki

A fast 2-D robot simulator. To contact us, please open an issue.
Other
36 stars 34 forks source link

groundTexture image format has inverted R and B channels #74

Closed titos-carrasco closed 2 years ago

titos-carrasco commented 2 years ago

I'm testing enki viewer using the enkiplayground app and the image below (Equinoxe Infinity Album - Jean Michel Jarre) as background

image

But the image does not shows properly image

The next patch (Playground.cpp) fix the problem

@@ -332,11 +332,14 @@ int main(int argc, char *argv[])

    // Create the world and the viewer
    bool igt(app.arguments().size() > 1);
    QImage gt;
    if (igt)
-       gt = QGLWidget::convertToGLFormat(QImage(app.arguments().last()));
+   {
+       QImage img(app.arguments().last());
+       if( img.format() ) gt = img.mirrored( false, true );
+   }
    igt = !gt.isNull();
    #if QT_VERSION >= QT_VERSION_CHECK(4,7,0)
    const uint32_t *bits = (const uint32_t*)gt.constBits();
    #else
    uint32_t *bits = (uint32_t*)gt.bits();

image

You can use jpg, gif, svg, etc image formats (RGB - needs the qt image format filters) - a 200cm x 200cm and 18dpi image works fine with low memory consumption

BUT:

Saludos

titos-carrasco commented 2 years ago

Some test and NO need to change nothinge else (only mirror the background)

image

stephanemagnenat commented 2 years ago

I am not sure to understand, is the problem that the background image has the red and blue channels inverted?

stephanemagnenat commented 2 years ago

It seems that indeed, that line should use GL_RGBA instead of GL_BGRA according to QGLWidget's convertToGLFormat documentation.

It seems that I introduced a change in that commit and somewhat forgot about the texture being loaded from file. Probably the easiest is to fix the data on load after the call to convertToGLFormat, otherwise it is an API breakage of Enki and I'm sure it will break downstream users.

titos-carrasco commented 2 years ago

Current background image - blue: 0,0,1 - cyan: 0,1,1 image

With "QGLWidget::convertToGLFormat()" when preparing groundTexture and robot over "cyan", gound sensors (World::getGroundColor) shows inverted values for channels image

Without "QGLWidget::convertToGLFormat()" when preparing groundTexture (just mirroring the image) and robot over cyan, gound sensors (World::getGroundColor) shows correct values image

In this last case no need to change fromARGB or GL_BGRA ¿why?

stephanemagnenat commented 2 years ago

As its documentation says, QGLWidget::convertToGLFormat() converts to GL_RGBA but the viewer, and the robot's sensors, expect the equivalent of GL_BGRA. So the hack I propose is to still call convertToGLFormat(), to have a known but wrong order, than pass through the array to invert R and B.

titos-carrasco commented 2 years ago

Many images in QImage are stored using a 32-bit ARGB format (QImage::Format_ARGB32 - 0xAARRGGBB). In little endian means BRGA****

This is why in Viewer.cpp (line 873 - initializeGL ) "groundTexture" is read as GL_UNSIGNED_BYTE GL_BGRA in glTexImage2D() - because read byte 0 first, then byte 1 and so on

In PhysicalEngine.cpp (line 783) getGroundColor() reads ground texture data as uint32_t and this is why returns "Color::fromARGB" (QImage::Format_ARGB32 format)

So, no needs to change anything just:

  1. don't use QGLWidget::convertToGLFormat() when preparing the background image (just mirror it)
  2. ese QImage::Format_ARGB32 image format for any background
  3. reduce dpi for lower memory consumption

Saludos

titos-carrasco commented 2 years ago

Just don't use convertToGLFormat() in:

Saludos

stephanemagnenat commented 2 years ago

The reason there is the call to convertToGLFormat in the first place is that the loaded image can have any format, so we need to convert to the one we will use. Maybe another conversation is bether indeed, as long as we have guarantees on not having any padding between lines.

titos-carrasco commented 2 years ago

In that case, and considering that the GL format used in convertToGLFormat is GL_RGBA:

Saludos

stephanemagnenat commented 2 years ago

I do not want to change the format of the ground data stored within Enki itself, as this would break the API and the projects using Enki. So we should convert to the current expected format on load, with 32 bpp hopefully there should be no padding, so calling QImage::convertToFormat with QImage::Format_ARGB32 instead of using convertToGLFormat should work.

stephanemagnenat commented 2 years ago

Fixed by bb486bf.

titos-carrasco commented 2 years ago

Works fine :-)