GRFCodec feature discussion thread

[planetmaker]

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.

Thanks for this link to the documentation, that made the difference clear.

Remembering that the realsprite params are xyyxxy is not the worst part of NFO.

Well, changing it to xyxyxy does make sense to me... if coordinates and offsets for real sprites are coded in (x/y) order it doesn't really come naturally to code x- and y-extend in (y/x) order; having other places of similar inconsistency is IMO not the best argument to keep it.

I don't know of sprites with heights around 255 pixels. Maybe TTRS?

Apple uses since Mac OS 9 als \n as line feed.

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.

No strong feelings, but go straight for it. It keeps things simpler, at least within this container, even though not cross-container.

[eis_os]

OK, If you want to change the container format, here are my wishes:

Convert the info byte to word size, to be really future proof
- (spec) add bit to tell O)TTD(Patch to draw a certain colour semitransparent (reduces station sprite complexity, you need one sprite less while sorting)

Add way to store several sprites into one (spritestreams) So you can add sprites for each zoomlevel (optimized sprite) Maybe the OTTD folks want to store 32bit pngs too?

[peter1138]

A new sprite container with the ability to store multiple zoom levels and different bit depths with the same sprite ID would be really really cool, although I'm not sure we actually need it...

[planetmaker]

He, I agree, different zoom levels and bit depths... awesome idea!

Need is relative, but it surely would be used. And it would make use of 32bpp and 8bpp sets WAY easier from a end-user perspective. Concerning zoom levels it'd need some thought so that it possibly allows for consistent definition of additional zoom levels which are not (yet) defined.

EDIT: after a short discussion with petert1138:
My personal preference for the zoom-level definition would be a description of the related sprite size in terms of the width in pixels (i.e.. 64 for normal zoom level, 32 for zoomed out and 16 for zoomed out twice). This would allow possibly the implementation of fractional zoom levels with sprite width which are not a power of two or possibly the representation of this sprite on the overview or minimap when the whole sprite is condensed to width=1. Not sure whether it should be a byte value (sufficiently I guess) or even word.

[eis_os]

Some thing like this in.nfo:

+ <word: type> <sprite size description> filename.pcx
+ <word: type> <sprite size description> filename.png
+ <word: type> <sprite size description> filename.png
+ <word: type> <sprite size description> filename.png


<sprite size description> aka width height (if position and width is zero, use the dimension of the file)


Internal:
<size of header word> <type word> <offset dword> <type word> <offset dword> <type word> <offset dword> 00 00

Types:
01 00 = regular sprite
01 02 = regular sprite x2 zoom in
01 04 = regular sprite x4
01 FE = regular sprite x -2 zoom out

02 00 = regular sprite (32bpp)
02 02 = regular sprite x2 zoom in (32bpp)
02 04 = regular sprite x4 (32bpp)
02 FE = regular sprite x -2 zoom out (32bpp)

00 00 = end marker

For low memory a grf could be stripped, aka removing certain types of sprites, a bit depth...
The internal sprite stream header can be loaded quite easy and needs a simple loop construct to get all sprites.

[peter1138]

I would envisage that grfcodec would abstract the binary format, so that the NFO would say something like "8bpp 1x", and grfcodec would encode that as 01 00.

If, as per planetmaker's suggestion, pixel-based size is specified in the binary format, then "8bpp 1x" could resolve to 01 40 00, and "32bpp 256" would be 02 00 10.

[planetmaker]

I'd propose the slightly larger (1byte per definition albeit more flexible definition). Following eis_os scheme:

<type> <tile size> <sprite size description> filename

with
<type> = 01 // 8bpp
<type> = 02 // 32bpp

<tile size> = 64 // normal zoom level
<tile size> = 32 // zoomed out
<tile size> = 16 // zoomed out maximum
<tile size> = 256 // zoomed twice
<tile size> = 96 // freaky fractional zoom level

The rest as eis_os describes. Possibly with 8bpp automatically detecting whether the sprite is supplied as pcx or png, allowing either or.

EDIT: petern was faster

[DaleStan]

- Convert the info byte to word size, to be really future proof

I'm not convinced being that future-proof is worthwhile. But it's a possibility.

- (spec) add bit to tell O)TTD(Patch to draw a certain colour semitransparent

I don't believe that needs a container update. Just grab an always-clear bit from the info byte and document that it now means something when set, like what I did when I defined bit 6. (The obvious color to use is 255 (FFFFFF); it has a visually indistinguishable replacement at FCFCFC and it's a lousy color to actually use on the screen because it doesn't play well with fade out.)

Add way to store several sprites into one (spritestreams) So you can add sprites for each zoomlevel (optimized sprite)

Not happening. I'm not convinced this is a container issue, and I'm not thrilled by how easy this will make it to put the wrong zoomed-out sprites in the stream, but my main objection is that I have no clue how to make Patch support it.
You're welcome to do it yourself, though. Bump the container version to 3 -- or the GRF version to 8 -- and have fun.

Current change summary:
- Add header
- Sprite size to dword
- (probably) realsprite height to word
- (maybe) info byte to word

[planetmaker]

Add way to store several sprites into one (spritestreams) So you can add sprites for each zoomlevel (optimized sprite)

Not happening. I'm not convinced this is a container issue, and I'm not thrilled by how easy this will make it to put the wrong zoomed-out sprites in the stream, but my main objection is that I have no clue how to make Patch support it.
You're welcome to do it yourself

Fair enough and quite understandable as it means quite some changes. Despite that, why not provision for this kind of change despite. And meanwhile require sprites to have the default type and tile size, i.e. only allowing sprites of the type

01 40 00 <sprite size description> filename.pcx // default type 01 = 8bpp, default zoom level = 64px per tile

and declaring all other definitions as an error? Also I don't see how this definition makes it easy to supply wrong zoom-level sprites. That wouldn't require any further changes for now, but would avoid the need for another bump of the container specs for this end (which is not something to do too lightly IMO)

[eis_os]

- Convert the info byte to word size, to be really future proof

I'm not convinced being that future-proof is worthwhile. But it's a possibility.

Current change summary:
- Add header
- Sprite size to dword
- (probably) realsprite height to word
- (maybe) info byte to word

OK

[DaleStan]

why not provision for this kind of change despite. And meanwhile require sprites to have the default type and tile size, i.e. only allowing sprites of the type
01 40 00 <sprite size description> filename.pcx // default type 01 = 8bpp, default zoom level = 64px per tile

Because 1) the container doesn't contain file names, 2) guessing what we'll want next month almost invariably comes out wrong, and 3) I am provisioning for changes. Change that 82 in the header to an 83, and anything that appears later can be changed too.

Also I don't see how this definition makes it easy to supply wrong zoom-level sprites.

With the current system, if I supply the wrong sprite, the same wrong sprite appears in all zoom levels. Easy to test, easy to spot, easy to fix. If I get to specify multiple sprites, then I can end up having an engine with an engine sprite in zoom levels 0 and 1, but a wagon sprite at level 2. Much head-scratching (at best) will ensue when a full-size half-full coal hopper appears where you were expecting a quarter-size engine.

[eis_os]

Some clarification for the grf specs:

Size:
Size can be either compressed or uncompressed size depending on info bit 1
(Size as in TTD memory)

bit: 1 Decompress when drawing.
Sprites will be hold in compressed state in TTD memory (not decompressed when loaded),
drawing will be slower. The size is compressed size if set, otherwise it should be the decompressed size.
If it is unset, you *must* decompress it to find out how large it is in the file.

As pseudo sprites have bit1 set, TTD won't try to decompress them when loading.

-update-

Using tile compression together with bit1 works only if the actual tile compressed version after normal decompression will be width*height bytes, otherwise TTD(Patch) will crash badly. So you can't combine both

[Rubidium]

Please make UPX conditional so people compiling GRFCodec don't need to modify the Makefile. Especially for platforms where UPX doesn't work like x86_64 BSD or the other platforms that OpenTTD compiles on like alpha, ia64 and sparc. See the attached patch for an implementation of this "feature".

Also GRFCodec's compilation looks ugly on systems that don't use a bash-alike shell, i.e. a shell where "-e" is a valid parameter for echo instead of something to echo. For me it looks like compilation looks like: "-e \t[LD] grfcodec". If I remove the -e from the echo/Makefile and update the assignment of _I the Makefile still seems to work for me and when using bash as default shell it works exactly the same. A patch removing the "-e" is attached.
Another solution would be "SHELL = /bin/bash" in the Makefile, but a problem with that is that on Msys/MinGW there is no /bin/bash, only a /bin/sh which is bash. In other words, doing that breaks it there unless when guarding that by a "is mingw/msys" check.

Furthermore "make remake" and "make release" are racy with -jN, N > 1; sometimes it only removes the files, sometimes it works fine and sometimes it fails compilation somewhere in the middle. A patch to fix that is attached too.

[Rubidium]

On some SUSE systems GRFCodec seems to fail to properly compress, see http://obs.openttdcoop.org/specs/opentt ... ory_64_log for the full log. I am not sure what causes those problems, but it gave me a reason to run valgrind over GRFCodec.

It showed a couple of "conditional jump or move depends on uninitialised value" and "mismatched free / delete / delete[]" warnings as can be seen in the attachment. I was compiling ogfxi_logos.grf (from OpenGFX) with that test (valgrind grfcodec -e ogfxi_logos.grf). The attached patch fixes the warnings/errors from valgrind.

[Ammler]

I can confirm that the issue is something with grfcodec, building grf works with flag -u

The patch from Rubi didn't solve the issue.

[planetmaker]

There seems to happen some unwanted things, if I compile OpenGFX with the "-c" flag. See http://dev.openttdcoop.org/issues/903 and the attached screenshot. I'm not quite sure why the alignment of namely the track sprites (both some road, monorail and maglev) seems to fail. I tested with r413 (with -c) / r414 (without -c) of OpenGFX (anonymous checkout via hg clone http://mz.openttdcoop.org/hg/opengfx )

[ChillCore]

Just a wild guess planetmaker but would you mind trying this in table/sprites.h

Code: Select all

- static const uint16 OPENTTD_SPRITE_COUNT = 153;
+ static const uint16 OPENTTD_SPRITE_COUNT = 154;

I think that the debug button was not yet added there and that could be the cause.
I had to change it for the moreheightlevels patch to avoid glitches. I added 20 sprites instead of 19 and my glitches are gone.
Reading your "missing sprites?" message and seeing the picture ... It is the first thing that comes to mind.

As I said just a wild guess but worth the try...

[planetmaker]

Just a wild guess planetmaker but would you mind trying this in table/sprites.h

I fail to see how changing OpenTTD might have something to do with grfcodec's output; especially as
a) it works with the original base set
b) it works when grfocdec is used without the -c option but the same NFO input
c) OpenTTD has no problems if the grf supplies TOO MANY gui sprites
d) I usually - also in this case - test with recent OpenTTD, approx. yesterday's nightly

[ChillCore]

a) Because the original graphics do not add sprites by themselves and OpenTTD does?
b) I have not much experience creating grfs. I used grf wizard for the flat_blacktiles grf. What does the -c option do?
c) No, but the grf has problems if it thinks that openttd only has 153 sprites and in fact there are 154. As long as you do not define sprites after them all is well. If I do add some like with the flat_blacktiles.grf, I have to tell it that there are 154 sprites and only then I can add the other nineteen as intended if I do not do it like that I get glitches.
d) Me too, I check out my source from svn and try to keep it somewhat up to date ...

I have just finished compiling while I was typing.
I will now test and revert OpenTTD sprite count to 153 sprites and take a screenshot if I still have the glitches. I will edit my post in a few minutes ...

EDIT: a) OpenGFX -> OpenTTD

Please see attachment.

Code: Select all

static const uint16 OPENTTD_SPRITE_COUNT = 172;
...
static const SpriteID SPR_FLAT_BLACKTILES               = SPR_OPENTTD_BASE + 153;

OpenTTD and the grf are fighting over sprite 154.

if I do this:

Code: Select all

static const uint16 OPENTTD_SPRITE_COUNT = 173;
...
static const SpriteID SPR_FLAT_BLACKTILES               = SPR_OPENTTD_BASE + 154;

my glitches are gone.

[Rubidium]

The bug report planetmaker posted has a misleading title. It has absolutely nothing to do with missing sprites, just sprites with wrong alignment. Furthermore the sprite ChillCore is talking about is thousands of sprites after the sprites planetmaker is concerned about. Even then, OpenGFX/OpenTTD handles the extra sprite correctly (as per specs). If OpenTTD would be missing sprites it would show the question mark (in OpenGFX it's the i in a circle), except when you replace them with black/empty sprites but then it would show those and not misaligned sprites.

Conclusion: what ChillCore is talking about has nothing to do with the bug in GRFCodec w.r.t. '-c' that planetmaker reported.

The issue ChillCore is facing is that he wants to add GUI sprites and the official OpenTTD developers want to add them sometimes too which creates a conflict much like the saveload version conflict. In this case the 'bug' sprite is already added to the OpenGFX graphics because I am working on something that needs that sprite (the actual code already exists for one case, just needs to be generalised) and after a short conversation with OpenGFX we decided to add the sprite already to the NewGRF specs and OpenGFX so it could be added in the release instead of making a new release for one sprite just a few weeks later. Technically the issue ChillCore has is that his OpenTTD (with trunk's action5 size) does not conform to the specification.