GRFCodec feature discussion thread

[DaleStan]

edorfaus, please tell me that 2155 works for you? I did something substantially more complicated, but the nasm problems are gone. (Probably to be replaced with perl problems ...)

[edorfaus]

grfdiff and grfmerge are ancient and minimally supported, but it is nice to have them compile. However, it seems like everyone is complaining that nasm-doesn't-like-my-this, nasm-doesn't-like-my-that, nasm-doesn't-like-my-other-thing. I'm rather tempted to write a Perl script to turn it grfmrgc.bin into C and solve the problem once and for all.

edorfaus, please tell me that 2155 works for you? I did something substantially more complicated, but the nasm problems are gone. (Probably to be replaced with perl problems ...)

OK. I checked out a clean copy (2156, but close enough), and grfdiff compiles out of the box now.

There is one minor issue, which I probably should have mentioned in the last post, but it's not that important really... I don't have upx installed, don't really care about having the apps compressed, and therefore only compile the apps with make all instead of release. However, grfdiff tries to strip/upx grfmrgc.bin during compile - which fails with "make: upx: Command not found".
The reason this isn't really a problem (for me anyway), is that I just run make all a second time, make then skips that step when it resumes where it left off, and completes successfully.

1. If grfcodec is encoding pure white pixels then, 9 times out of 10, you typoed the real-sprite. I could make it more intelligent and only trigger on white pixels on the edge of the sprite, and/or only on full lines of white pixels, but ...
2. Pure white pixels do not fade out when TTD fades to black, leaving mostly-black with a few bright white dots

There is a visually identical #FCFCFC in the palette that I recommend you use instead.

Aaah, I see. Makes sense now. Especially the second reason is good to warn against, as it might give a pointer for someone who doesn't know that about the fading, but early warnings for typos are good too (I assume people would check their sprites in-game and notice white lines, but that takes longer). And yeah, I agree it's not really worth the bother to only check edges/full lines, even if the warning was only for the typos.

Thank you for the help, and for making such useful tools! :)

[DaleStan]

(I assume people would check their sprites in-game and notice white lines, but that takes longer).

Unfortunately, this doesn't always happen. Some time ago, even one of TTDPatch's base graphics (nsignals[w].grf) had a white line on one of the signals.

Another way to make it shut up (assuming I did it right) is -q.

[planetmaker]

1. If grfcodec is encoding pure white pixels then, 9 times out of 10, you typoed the real-sprite. I could make it more intelligent and only trigger on white pixels on the edge of the sprite, and/or only on full lines of white pixels, but ...
2. Pure white pixels do not fade out when TTD fades to black, leaving mostly-black with a few bright white dots

There is a visually identical #FCFCFC in the palette that I recommend you use instead.

Fixing these warnings in the root, e.g. in the sprite certainly is better than to silence warnings later. But can you help my understanding: when do sprites "fade to black" so that this becomes an issue? (Maybe I just don't understand properly though what you mean with those words, e.g. language issue)

[DaleStan]

TTD and TTDPatch fade to black when displaying the high score table. They also fade to black when exiting if in full-screen mode.

Attached is a sample, halfway through the fade. The pure white is blatantly obvious.

[Ammler]

about the pure white warnings, might it be possible to pipe those warnings through the 2nd output stderr or how you call that. We run "grfcodec ... 1>log 2>errors", if we now try to remove all pure white in the sprites itself, it would be nice to have those warnings more verbose.

Thanks and greets
Ammler

[Rubidium]

I would like to propose two features for GRFCodec:
1) show filename and sprite number on warnings/errors. This makes it easier to actually find the wrong sprite when you're compiling multiple NewGRFs by means of a script and grep on "Warning" or "Error" string. Attached is a patch that implements this and unifies the error output to go to stderr.

grfcodec_make_warnings_more_useful.diff

(6.1 KiB) Downloaded 170 times

2) do not show progress reports and the likes to stdout when stdout is not a console. This makes logs of compile runs more useful because it will not be for 99% filled with "Sprite N Done: N% ...\n\n" / "Loading <file>" and thus easier to spot warnings/errors that might occur. Attached is a patch that implements this.

grfcodec_no_progress_on_non_interactive_output.diff

(3.24 KiB) Downloaded 202 times

[planetmaker]

I get two warnings when compiling on OSX 10.6 with gcc 4.2 and a (non-critical) error:

Code: Select all

info.cc: In constructor ‘inforeader::inforeader(char*)’:
info.cc:88: warning: format not a string literal and no format arguments
-e [CC] error.o
-e [CC] getopt.o
-e [CC] path.o
-e [CPP] readinfo.o
-e [CPP] file.o
-e [CPP] grfcodec.o
-e [LD] grfcodec
-e [CPP] grfdiff.o
-e [CC] grfmerge.os
grfmerge.c: In function ‘int yesno(const char*)’:
grfmerge.c:127: warning: format not a string literal and no format arguments
-e [CC] getopt.os
-e [LD] grfmrgc.bin
-e [STRIP/UPX] grfmrgc.bin
make: *** [grfmrgc.bin] Error 1

The error is due to the fact that there is principally no upx available (I checked, I have the latest version available on the upx website, from 27 April 2008) which can actually pack the resulting binary format and upx thus fails on that.

[Ammler]

I ran svn r2212 on build.opensuse.org through a lot of different linux distros.

The only patch I used the first time was to disable upx:

remove-upx.diff

(522 Bytes) Downloaded 175 times

This already succeeded on almost every version:
CentOS_5 x86_64 i586
Fedora_10 x86_64 i586
Mandriva_2009 x86_64 i586
Mandriva_2009.1 x86_64 i586
openSUSE_10.3 x86_64 i586
openSUSE_11.0 x86_64 i586
openSUSE_11.1 x86_64 i586
RHEL_4 x86_64 i586
RHEL_5 x86_64 i586
SLES_9 x86_64 i586
SLE_10 x86_64 i586
SLE_11 x86_64 i586


It failed on Fedora 11 and openSUSE Factory,
openSUSE_Factory x86_64 i586
Fedora_11 x86_64 i586

Code: Select all

[CC] path.o
getopt.c: In function 'int _getopt_internal(int, char* const*, const char*, const option*, int*, int)':
getopt.c:523: error: invalid conversion from 'const char*' to 'char*'
make: *** [getopt.o] Error 1
make: *** Waiting for unfinished jobs....
make: *** wait: No child processes. Stop.
error: Bad exit status from /var/tmp/rpm-tmp.1Tdb1a (%build)

But I found a patch from the fedora packager:
http://cvs.fedoraproject.org/viewvc/rpm ... iew=markup
with this it was a success on all, also the older...

The rpms are available here:
http://download.opensuse.org/repositori ... r:/openttd

Greets
Ammler

[Rubidium]

Feature request/bug report/feature implementation:

GRFcodec does not support *reading* pcx files higher/wider than 32768 pixels [0]. Writing them goes fine, at least till 65536 (I assume) pixels because the pcx format doesn't support anything bigger.

Anyhow, decoding the world airline set [1] creates a pcx of about 34700 pixels high which means that it cannot be encoded properly. I can open the created pcx in GIMP, so it seems to be valid.

The specifications I could find [2,3,4] did not speak about the signed/unsignedness of the width/height/DPI values only that they are word sized, but I think we can assume them to be unsigned. For this case a patch is attached; it's nothing more than a quick fix that works for me. If it cannot be assumed it is unsigned it would be wise to either fail decoding or decode into multiple pcx files (most future proof solution I think).

[0] http://www.tt-forums.net/viewtopic.php?p=822154#p822154
[1] http://mz.openttdcoop.org/bundles/world ... v-r582.zip
[2] http://www.qzx.com/pc-gpe/pcx.txt
[3] http://www.fileformat.info/format/pcx/corion.htm
[4] http://www.fileformat.info/format/pcx/egff.htm

[planetmaker]

With svn r2245 and OSX 10.6 (gcc 4.2.1) I get a range of compile warnings:

Code: Select all

pcxfile.cc: In member function ‘void pcxfile::newfile(int)’:
pcxfile.cc:80: warning: comparison is always false due to limited range of data type
-e [CPP] sprites.o
-e [CPP] pcxsprit.o
-e [CPP] info.o
info.cc: In constructor ‘inforeader::inforeader(char*)’:
info.cc:88: warning: cannot pass objects of non-POD type ‘class Sprite::unparseable’ through ‘...’; call will abort at runtime
info.cc:88: warning: format ‘%s’ expects type ‘char*’, but argument 2 has type ‘int’
-e [CC] error.o
-e [CC] getopt.o
-e [CC] path.o
-e [CPP] readinfo.o
-e [CPP] file.o
-e [CPP] grfcodec.o
-e [LD] grfcodec
-e [CPP] grfdiff.o
-e [CC] grfmerge.os
grfmerge.c: In function ‘int yesno(const char*)’:
grfmerge.c:127: warning: format not a string literal and no format arguments
-e [CC] getopt.os
-e [LD] grfmrgc.bin
-e [STRIP/UPX] grfmrgc.bin
-e [PERL] grfmrg.c
-e [CC] grfmrg.o
-e [LD] grfdiff
-e [CC] grfmerge.o
grfmerge.c: In function ‘int yesno(const char*)’:
grfmerge.c:127: warning: format not a string literal and no format arguments

Also, version.h is not generated automatically by make, once I run mrproper. If I ask make to generate version.h explicitly, the output above is the result and grfcodec successfully built.

[planetmaker]

Kinda follow-up on above posting:

I've a problem making grfcodec r2274 from a clean checkout (after make mrproper && svn up):

Code: Select all

-e [CPP] info.o
info.cc: In constructor ‘inforeader::inforeader(char*)’:
info.cc:88: warning: format not a string literal and no format arguments
...
-e [CPP] grfcodec.o
grfcodec.cc: In function ‘int main(int, char**)’:
grfcodec.cc:747: error: expected `)' before ‘GRFCODECVER’
make: *** [grfcodec.o] Error 1

The attached diff fixes the make error with version.h by adding an explicit rule that grfcodec.o depends on it.

It remain the above warning as well as another one in two other places:

Code: Select all

-e [CPP] info.o
info.cc: In constructor ‘inforeader::inforeader(char*)’:
info.cc:88: warning: format not a string literal and no format arguments
...
-e [CC] grfmerge.o
grfmerge.c: In function ‘int yesno(const char*)’:
grfmerge.c:127: warning: format not a string literal and no format arguments

Build system: MacOSX 10.6.2 (64bit, intel with gcc 4.2.1)

[planetmaker]

Sorry for double posting. But a completely different issue (r2274):

grfcodec -d fails to produce a decent pcx file on my mac while it works on my linux system. Find attached the de-compiled pcx files of the very same newgrf

The headers of the pcx files written do differ:
linux, 32bit grfcodec:

Code: Select all

0a 05 01 08 00 00 00 00  1f 03 7f 01 48 00 48 00 
00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 
00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 
00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 
00 01 20 03 01 00 1f 03  7f 01 00 00 00 00 00 00 
00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 
00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 
00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 

vs. mac, 64bit grfcodec:

Code: Select all

00 00 00 00 00 00 00 00  ff ff 7f 01 00 00 00 00 
00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 
00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 
00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 
00 00 00 00 00 00 00 00  7f 01 00 00 00 00 00 00 
00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 
00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 
00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00

The rest of the files is identical.

[DaleStan]

It has been ... suggested ... that I need to stop idly kicking this around, and actually do something about it, so here goes:

GRF version 2:
The current GRF container has served us well, but has the notable limitation of only supporting sprites smaller than 64 KiB.
I therefore propose two changes to the current GRF container (herein "GRF version 1" or similar) to generate a version 2 container that has that limitation raised to 4 GiB:

1. The size element in each sprite definition, currently a word, shall be changed to an extended word.
   Following from the theory of an extended byte, an extended word encodes values 0 through 0xFFFE in two bytes, and 0xFFFF through 0xFFFFFFFF in six bytes: FF FF xx xx xx xx. Version 1 containers containing sprites that are exactly 65535 bytes long must therefore have each such FF FF size field changed to FF FF FF FF 00 00.
2. To distinguish such version 2 GRFs from version 1 GRFs, all version 2 GRFs will start with the six bytes 00 00 47 52 46 02. (47 52 46 is "GRF")
   The first two bytes are so selected so that current readers will see a 0-byte sprite and determine that they have reached EOF. The remaining four are to identify this as a GRF v2 container, and not any other file that happens to start with two null bytes.

Comments? Thoughts? Concerns? Typos?

(Throughout the above, "version" refers to the container format, not to <version> byte in action 8 nor the "Info version" line in the NFO header.)

[peter1138]

For OpenTTD: http://fuzzle.org/~petern/ottd/containerv2.diff

[planetmaker]

Probably I'm missing some crucial point: wouldn't it rather make sense to move to grf version 8 in order to incorporate this? Or is there a special reason to call this change version two and add the indication of a change of the specs in another place than the grf version?

And when it comes to changing the specs anyway: what about going for some more changes to specs at the same time:
What about a mechanism for newgrfs to allow versioning for compatible newgrfs, so that for a given GRFID the most recent compatible NewGRF could be loaded? Also frosch made long ago some proposals for changes: http://devs.openttd.org/~frosch/texts/grf_version8.txt and http://devs.openttd.org/~frosch/texts/StaticGRFInfo.txt , maybe also http://devs.openttd.org/~frosch/texts/vartranstable.txt

There was once a grf version 8 discussion thread, but I fail to dig it from the depths of this forum.

[peter1138]

Because the version information in Action 8 is within the container, and it's the container format itself that would be changed.

[eis_os]

Together with the 4GB change I wish to note that TTDPatch may change certain GRF specific stuff to use internally grf local sprites. If we change the GRF Container format, we may as well change some TTDPatch cruft in the action321 syntax like the need to use extended bytes. And break sprite ids and sprite drawing flags. (Some internal code to remove TTDs SpriteCache is already in my local version trying to change TTDPatch Sprite Drawing Code)

Reference:
Progress and questions about the action format raised by switching to grf local sprites

[frosch]

I guess the discussion about the container format and the pseudo sprite format should not be mixed. Changing the container format is a small well defined task, while anything bigger will likely result in nothing. Also changing the format of action123 is not related to the container format, and could be archieved by changing the grf version in action8.

So, about the container format:
Does the format of real sprites belong to this? Then it might be useful to extent the height of realsprites to a word.
(And if that can be done, maybe positions of <width> and <height> could be swapped as well for general saneness. Though this is not important, as this could be done at grfcodec level independent of the container format.)

[DaleStan]

I guess the discussion about the container format and the pseudo sprite format should not be mixed. Changing the container format is a small well defined task, while anything bigger will likely result in nothing. Also changing the format of action123 is not related to the container format, and could be archieved by changing the grf version in action8.

Indeed, indeed, indeed.
Planetmaker, Oskar, others: Please make sure you are keeping the concepts of "Container version" and "Content version" separate. I am only changing the container -- the format documented here. If you want to discuss things documented elsewhere, please come back next week.

Rubidium suggested that, since I'm adding a brand new header, I might as well do something more like the PNG header. So, I'm changing the proposed header:
00 00 47 52 46 0D 82 0D 0A 1A 0A
The first five bytes are the same. Then:
0D: Mac line-feed, to detect line-ending conversions. (OT: Does Mac still use \r as its line-feed character, or is that merely historical?)
82: Double purpose: version byte and detection of 7-bit transmission links.
0D 0A 1A 0A: As in PNG header.

This is probably over-engineered, but it's 11 bytes against a container that rarely holds under 11 KB.

Does the format of real sprites belong to this? Then it might be useful to extent the height of realsprites to a word.

Arguably, yes. And conceivable. Do we have images that are pushing 255 pixels high? (This comes to 20-30 TTD stories, I think?)

(And if that can be done, maybe positions of <width> and <height> could be swapped as well for general saneness.

This seems gratuitous to me.

Though this is not important, as this could be done at grfcodec level independent of the container format.)

This is the way I'd do it, if I did it. I'm not enamored of the idea, though. Remembering that the realsprite params are xyyxxy is not the worst part of NFO.

Also, I'm thinking about going straight for dword sizes (and word heights), rather than extended-word (and extended-byte). If anyone has any strong feelings either way, speak quickly, or I may end up just flipping a coin.