News:

Use the "Forum Search"
It may help you to find anything in the forum ;).

Improved support for reading and writing image files

Started by ceeac, December 10, 2020, 04:32:51 PM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

ceeac

This patch improves support for reading and writing image files. The patch comprises the following additions and changes:

  • libpng is now required for building Simutrans (not only for makeobj)
  • Reading heightmaps from PNGs is now possible
  • Makeobj now supports reading grayscale PNGs, not only colored ones
  • Makeobj now supports reading BMP and PPM files (if anybody wants that...)
  • Screenshots are now saved as PNGs on all platforms. Platform specific screenshot code has been removed.

prissi

I will probably have a look tomorrow. (I am not sure how one would define transparent colors with 8 bit grayscale makeobj images, but I think that is just a side product of the new loader.)

prissi

Thank you for your work.

Two principal things

I would prefer to have all these file in IO and not add another folder, please. The extra folders just make the include  path longer.(I think I would like to move the files from io/rdwr also to io at some point.) Well, with a patch file, this is just text replacing, so quickly done.

And please, do not indent #if etc. unless they are nested These are at left to make immediately clear these are not part of the code.

Now to the actual code:

On the concept:
I would suggest using two different classify routines, classify_img and classify_file. Images should never be handled by the normal routine and vice versa. That will avoid errors by design. Also rescanning a full savegame folder (can happen occasionally, if one uses different simutrans versions) takes even more time like this.

The comment in line 241 suggests that the last png file is cached. However, your code assumes the last file is cached, whatever format. Maybe overlooked the comment?

Wikipedia gives P3 as magic for PPM files. I think there are several versions around. Maybe go by extension and starting with P instead to classify? (It seems extension is ignored, which is ok for bmp file, which coudl end in rle and dib, but for PNG/png and ppm there should not be other files like this.)

In makeobj, image_writer_t will quite with a fatal error upon wrong size, which make debugging of broken images a little more difficult. Maybe an error message and fail to open would cause the upper level error printing, which indicates which obj to blame.

Assigning a raw_image_t to an existing raw_image_t could cause the memory to be lost. I would rather get an explicit copy command, or just forbid assignments and one has to use pointers. Also copying the memory seems wasteful, and this will happens likely by accident easily with C++. (But maybe I got this wrong, so please correct me, should I be wrong.)

ceeac

Quote from: prissi on December 11, 2020, 02:53:31 PM(I am not sure how one would define transparent colors with 8 bit grayscale makeobj images, but I think that is just a side product of the new loader.)
PNG supports 16 bpp gray + alpha images (8 bits gray + 8 bits alpha) but I have not implemented support for transparent grayscale PNGs, only ones without an alpha channel.

Quote from: prissi on December 13, 2020, 01:38:45 PMI would prefer to have all these file in IO and not add another folder, please. The extra folders just make the include  path longer.
i did it this way so that the files don't have to be moved again when the folder gets bigger. I have not changed it back just yet, but I can change it back before committing the patch.

Quote from: prissi on December 13, 2020, 01:38:45 PMAnd please, do not indent #if etc.
Quote from: prissi on December 13, 2020, 01:38:45 PMI would suggest using two different classify routines, classify_img and classify_file.
Quote from: prissi on December 13, 2020, 01:38:45 PMMaybe an error message and fail to open would cause the upper level error printing
Changed.

Quote from: prissi on December 13, 2020, 01:38:45 PMHowever, your code assumes the last file is cached, whatever format.
This is intentional - image_writer_t::block_load accepts any file name and any supported format, it is just that image_writer_t::write_obj assumes the image has a png extension.

Quote from: prissi on December 13, 2020, 01:38:45 PMMaybe go by extension and starting with P instead to classify?
The ppm reader only supports P6 type files where data is stored raw - P3 type ppm files are stored as ASCII numbers. So IMO it does not make sense to change it for now.

Quote from: prissi on December 13, 2020, 01:38:45 PMAssigning a raw_image_t to an existing raw_image_t could cause the memory to be lost.
No - the new image is passed by value which causes it to be copied, the temporary copy gets swapped with the original image, which causes the original data to be deleted when the temporary copy gets deleted on scope exit. Copy-and-swap.

Quote from: prissi on December 13, 2020, 01:38:45 PMI would rather get an explicit copy command, or just forbid assignments
Hmm, this would be consistent with the classes in tpl/ ... Changed.

Ters

Since typical assignment uses the copy operator, I would expect a copy, not a swap. Swapping is however acceptable for the move operator.

prissi

Here is the patch without the img folder and some changes to actually compile the MSVC project. (And the svn:eol-style attribute)


prissi

No, because this requires other code. It could make it easier.

ceeac

Quote from: prissi on January 25, 2021, 01:10:07 PM
Here is the patch without the img folder and some changes to actually compile the MSVC project. (And the svn:eol-style attribute)
Thanks! I did some more testing and submitted your version and a small fix for compiling on Linux in r9620.

Andarix

this doesn't compile for me under git action

r9629

Linux makeobj
Quote2021-02-12T09:45:33.7218418Z ===> HOSTCXX io/raw_image_bmp.cc
2021-02-12T09:45:34.0143184Z ===> HOSTCXX io/raw_image_png.cc
2021-02-12T09:45:34.1756537Z io/raw_image_png.cc: In member function 'bool raw_image_t::write_png(const char*) const':
2021-02-12T09:45:34.1758407Z io/raw_image_png.cc:298:40: error: invalid conversion from 'const uint8* {aka const unsigned char*}' to 'png_bytep {aka unsigned char*}' [-fpermissive]
2021-02-12T09:45:34.1759429Z      png_write_row(png_ptr, access_pixel(0, y));
2021-02-12T09:45:34.1760010Z                                         ^
2021-02-12T09:45:34.1760674Z In file included from /usr/include/png.h:321:0,
2021-02-12T09:45:34.1761317Z                  from io/raw_image_png.cc:9:
2021-02-12T09:45:34.1762495Z /usr/include/png.h:1646:8: note:   initializing argument 2 of 'void png_write_row(png_structp, png_bytep)'
2021-02-12T09:45:34.1763733Z  extern PNG_EXPORT(void,png_write_row) PNGARG((png_structp png_ptr,
2021-02-12T09:45:34.1765437Z         ^
2021-02-12T09:45:34.1922511Z common.mk:50: recipe for target 'build/default/io/raw_image_png.o' failed
2021-02-12T09:45:34.1925128Z make: *** [build/default/io/raw_image_png.o] Error 1
2021-02-12T09:45:34.1933231Z ##[error]Process completed with exit code 2.
2021-02-12T09:45:34.1976007Z Cleaning up orphan processes

Linux simutrans SDL2
Quote2021-02-12T09:45:49.3913145Z ===> HOSTCXX io/raw_image.cc
2021-02-12T09:45:49.5928168Z ===> HOSTCXX io/raw_image_bmp.cc
2021-02-12T09:45:49.9153800Z ===> HOSTCXX io/raw_image_png.cc
2021-02-12T09:45:50.2225078Z io/raw_image_png.cc: In member function 'bool raw_image_t::write_png(const char*) const':
2021-02-12T09:45:50.2226814Z io/raw_image_png.cc:298:40: error: invalid conversion from 'const uint8* {aka const unsigned char*}' to 'png_bytep {aka unsigned char*}' [-fpermissive]
2021-02-12T09:45:50.2227856Z      png_write_row(png_ptr, access_pixel(0, y));
2021-02-12T09:45:50.2228400Z                                         ^
2021-02-12T09:45:50.2229032Z In file included from /usr/include/png.h:321:0,
2021-02-12T09:45:50.2229656Z                  from io/raw_image_png.cc:9:
2021-02-12T09:45:50.2230767Z /usr/include/png.h:1646:8: note:   initializing argument 2 of 'void png_write_row(png_structp, png_bytep)'
2021-02-12T09:45:50.2232224Z  extern PNG_EXPORT(void,png_write_row) PNGARG((png_structp png_ptr,
2021-02-12T09:45:50.2232833Z         ^
2021-02-12T09:45:50.2394670Z common.mk:50: recipe for target 'build/default/io/raw_image_png.o' failed
2021-02-12T09:45:50.2397584Z make: *** [build/default/io/raw_image_png.o] Error 1
2021-02-12T09:45:50.2408386Z ##[error]Process completed with exit code 2.
2021-02-12T09:45:50.2460164Z Cleaning up orphan processes

Linux simutrans posix
Quote2021-02-12T09:45:33.5163784Z ===> HOSTCXX io/raw_image.cc
2021-02-12T09:45:33.7218418Z ===> HOSTCXX io/raw_image_bmp.cc
2021-02-12T09:45:34.0143184Z ===> HOSTCXX io/raw_image_png.cc
2021-02-12T09:45:34.1756537Z io/raw_image_png.cc: In member function 'bool raw_image_t::write_png(const char*) const':
2021-02-12T09:45:34.1758407Z io/raw_image_png.cc:298:40: error: invalid conversion from 'const uint8* {aka const unsigned char*}' to 'png_bytep {aka unsigned char*}' [-fpermissive]
2021-02-12T09:45:34.1759429Z      png_write_row(png_ptr, access_pixel(0, y));
2021-02-12T09:45:34.1760010Z                                         ^
2021-02-12T09:45:34.1760674Z In file included from /usr/include/png.h:321:0,
2021-02-12T09:45:34.1761317Z                  from io/raw_image_png.cc:9:
2021-02-12T09:45:34.1762495Z /usr/include/png.h:1646:8: note:   initializing argument 2 of 'void png_write_row(png_structp, png_bytep)'
2021-02-12T09:45:34.1763733Z  extern PNG_EXPORT(void,png_write_row) PNGARG((png_structp png_ptr,
2021-02-12T09:45:34.1765437Z         ^
2021-02-12T09:45:34.1922511Z common.mk:50: recipe for target 'build/default/io/raw_image_png.o' failed
2021-02-12T09:45:34.1925128Z make: *** [build/default/io/raw_image_png.o] Error 1
2021-02-12T09:45:34.1933231Z ##[error]Process completed with exit code 2.
2021-02-12T09:45:34.1976007Z Cleaning up orphan processes

Dwachs

Seems it does not like the second parameter to be const. It compiles for me, maybe different version of png.

Replacing

png_write_row(png_ptr, access_pixel(0, y));

by

uint8 pxl = access_pixel(0, y);
png_write_row(png_ptr, pixel);

should do the trick
Parsley, sage, rosemary, and maggikraut.

ceeac

Quote from: Dwachs on February 12, 2021, 10:28:28 AMmaybe different version of png.
Yes, png_write_row takes a png_bytep instead of a png_const_bytep in older versions. The compile error should be fixed in r9630.

Andarix

simutrans compile

makeobj not compile

Linux
Quote2021-02-12T12:37:39.3593335Z ===> CXX ../io/raw_image.cc
2021-02-12T12:37:39.4706871Z ../io/raw_image.cc: In destructor 'raw_image_t::~raw_image_t()':
2021-02-12T12:37:39.4709486Z ../io/raw_image.cc:53:12: error: 'free' was not declared in this scope
2021-02-12T12:37:39.4710998Z    free(data);
2021-02-12T12:37:39.4711947Z             ^
2021-02-12T12:37:39.4748123Z ../uncommon.mk:53: recipe for target '../build/default/io/raw_image-makeobj.o' failed
2021-02-12T12:37:39.4752836Z make: *** [../build/default/io/raw_image-makeobj.o] Error 1
2021-02-12T12:37:39.4759653Z ##[error]Process completed with exit code 2.
2021-02-12T12:37:39.4819044Z Cleaning up orphan processes

prissi

inclusing <stdlib.h> in the header should fix this. see r9632

Andarix

r9632

Quote2021-02-13T23:48:01.4307872Z ===> CXX ../io/classify_file.cc
2021-02-13T23:48:01.5931953Z ===> CXX ../io/raw_image_bmp.cc
2021-02-13T23:48:01.7073524Z ../io/raw_image_bmp.cc: In member function 'bool raw_image_t::read_bmp(const char*)':
2021-02-13T23:48:01.7076070Z ../io/raw_image_bmp.cc:129:26: error: 'abs' was not declared in this scope
2021-02-13T23:48:01.7077388Z   this->width  = abs(width);
2021-02-13T23:48:01.7078032Z                           ^
2021-02-13T23:48:01.7163425Z ../uncommon.mk:53: recipe for target '../build/default/io/raw_image_bmp-makeobj.o' failed
2021-02-13T23:48:01.7164960Z make: *** [../build/default/io/raw_image_bmp-makeobj.o] Error 1
2021-02-13T23:48:01.7172870Z ##[error]Process completed with exit code 2.
2021-02-13T23:48:01.7218671Z Cleaning up orphan processes

MSVC
Quote1>raw_image.cc
1>raw_image_bmp.cc
1>raw_image_png.cc
1>raw_image_ppm.cc
1>log.cc
1>searchfolder.cc
1>Kompilieren...
1>simstring.cc
1>raw_image.obj : error LNK2001: Nicht aufgelöstes externes Symbol ""public: __thiscall file_info_t::file_info_t(void)" (??0file_info_t@@QAE@XZ)".
1>raw_image.obj : error LNK2001: Nicht aufgelöstes externes Symbol ""enum file_classify_status_t __cdecl classify_image_file(char const *,struct file_info_t *)" (?classify_image_file@@YA?AW4file_classify_status_t@@PBDPAUfile_info_t@@@Z)".
1>raw_image_ppm.obj : error LNK2001: Nicht aufgelöstes externes Symbol ""char * __cdecl read_line(char *,int,struct _iobuf *)" (?read_line@@YAPADPADHPAU_iobuf@@@Z)".
1>D:\simutrans_entwicklung\svn\simutrans_nightly\makeobj\..\build\makeobj\Release\Makeobj.exe : fatal error LNK1120: 3 nicht aufgelöste Externe
1>Die Erstellung des Projekts "Makeobj.vcxproj" ist abgeschlossen -- FEHLER.

prissi


Andarix

r9635 Linux

Quote2021-02-14T13:31:25.5640926Z ===> CXX ../io/classify_file.cc
2021-02-14T13:31:25.7540416Z ===> CXX ../io/raw_image_bmp.cc
2021-02-14T13:31:26.0312953Z ===> CXX ../io/raw_image_png.cc
2021-02-14T13:31:26.2939746Z ===> CXX ../io/raw_image_ppm.cc
2021-02-14T13:31:26.4167915Z ../io/raw_image_ppm.cc: In member function 'bool raw_image_t::read_ppm(const char*)':
2021-02-14T13:31:26.4169609Z ../io/raw_image_ppm.cc:47:26: error: 'atoi' was not declared in this scope
2021-02-14T13:31:26.4170315Z    param[index++] = atoi(c);
2021-02-14T13:31:26.4170711Z                           ^
2021-02-14T13:31:26.4242271Z make: *** [../build/default/io/raw_image_ppm-makeobj.o] Error 1
2021-02-14T13:31:26.4244310Z ../uncommon.mk:53: recipe for target '../build/default/io/raw_image_ppm-makeobj.o' failed
2021-02-14T13:31:26.4255828Z ##[error]Process completed with exit code 2.
2021-02-14T13:31:26.4317732Z Cleaning up orphan processes

prissi