More height levels (in trunk since r27010)

[ic111]

Ok, I didn´t have nforenum on the path yet, and using higher makefile magic, it transformed "$(NFORENUM) -s" into "s" for empty NFORENUM. Output is now

Code: Select all

make[1]: Entering directory `/home/wol/openttd/trunk_mhl/objs/extra_grf'
cp /home/wol/openttd/trunk_mhl/media/extra_grf/*.png /home/wol/openttd/trunk_mhl/media/extra_grf/rivers/*.png /home/wol/openttd/trunk_mhl/objs/extra_grf/sprites 2> /dev/null
gcc -nostdinc -I/home/wol/openttd/trunk_mhl/media/extra_grf -C -E - < "/home/wol/openttd/trunk_mhl/media/extra_grf/openttd.nfo" | sed -e '/^#/d' -e '/^$/d' > /home/wol/openttd/trunk_mhl/objs/extra_grf/sprites/openttd.nfo
nforenum -s /home/wol/openttd/trunk_mhl/objs/extra_grf/sprites/openttd.nfo
Linter failure on sprite 4248.
make[1]: *** [/home/wol/openttd/trunk_mhl/bin/baseset/openttd.grf] Fehler 4
make[1]: Leaving directory `/home/wol/openttd/trunk_mhl/objs/extra_grf'
make: *** [all] Fehler 1
wol@dbis-lap13:~/openttd/trunk_mhl$ grfcodec
GRFCodec 6.0.4 r980 - Copyright (C) 2000-2005 by Josef Drexler
[...]

Questions:
(1) Is this ("Linter failure") the nforenum-related error you expected?
(2) Is this the correct grfcodec version (I downloaded it yesterday)?

Trying to manually assemble it manually ended again with errors.

wol@dbis-lap13:~/openttd/trunk_mhl/media/extra_grf$ grfcodec -e openttd.grf .
Encoding in temporary file openttd.new
openttd.grf:1: Warning: Found 9 more sprites than sprite 0 reports.

I will inspect them next, trying to find some error of myself...

[ic111]

The linter error is definitely caused by me, it disappears if I remove my new palette.nfo from openttd.nfo

However, this one

Code: Select all

wol@dbis-lap13:~/openttd/trunk_orig/media/extra_grf$ grfcodec -e openttd.grf .
Encoding in temporary file openttd.new
openttd.grf:1: Warning: Found 9 more sprites than sprite 0 reports.

appears even on clean trunk (i.e. a directory tree where hg pull reports that there aren´t any change sets for me, and no patches are applied).

[Rubidium]

Manually compiling openttd.nfo is not going to work.
a) openttd.nfo includes files (using C preprocessor directives), but grfcodec does not understand this. So you would need to run the C preprocessor 'manually'.
b) the C preprocessor leaves some things grfcodec can't cope with, so you need to clear some stuff, e.g. using sed or awk.
c) the number of sprites in sprite 0 is deliberately wrong, actually all sprite numbers are wrong too; it's mostly ignored since updating that field each and every time is a hassle. Furthermore IIRC the field is not even used by OpenTTD. Anyhow, renum, and later NFOrenum, became a tool to "fix" or calculate these sprite number and the number of sprites.
d) only after steps a, b and c grfcodec will do the right thing, and without warnings for clean OpenTTD.

Luckily OpenTTD will automatically recompile the extra grf if one of the required files in media/extra_grf has changed under the condition that the configure script found both grfcodec and nforenum with a recent enough version. It will even place the files in the right location. As such, it's relatively easy to change these files.

We could even leave out the compiled grf from the source repository and require grfcodec and nforenum, but that's somewhat a nuisance on Windows + MSVC. As a result a normal "make clean" won't remove them, but a "make mrproper" will remove them, and then the following compilation will attempt to build them (assuming grfcodec and nforenum are installed).

If/when you are adding new "features" to the NewGRF format, e.g. a new action 5, then nforenum will warn that it does not know it. This isn't a bug per-se, simply because it's adhering to different specs than you would be. For example, in the GRF source for the extra GUI sprites we deliberately ignore the check for the number of sprites because it will fail each time we add a sprite.

[ic111]

Thanks, this makes things a lot clearer. Then it reduces to the question, by what the linter failure is caused, i.e. what´s wrong with

Code: Select all

-1 * 3 05 18 01
-1 * 257 00 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 
              01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 
              01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 
              01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 
              01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 
              01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 
              01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 
              01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 
              01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 
              01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 
              01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 
              01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 
              01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01 01

(EDIT: Added newlines to repair forum layout)

Or is my problem just that

then nforenum will warn that it does not know it.

in fact results in a fatal message that stops any further processing? I just downloaded the source code of nforenum, but honestly, I didn´t even find out where this message is thrown...

[Rubidium]

There's one minor mistake in the nfo and nforenum needs to be taught a lesson. See the patches.

[Eddi]

Questions:
(1) Is this ("Linter failure") the nforenum-related error you expected?

probably, save for the mistake already mentioned.

you can look at objs/extra_grf/sprites/openttd.nfo to get a more detailed error message.

Trying to manually assemble it manually ended again with errors.

wol@dbis-lap13:~/openttd/trunk_mhl/media/extra_grf$ grfcodec -e openttd.grf .
Encoding in temporary file openttd.new
openttd.grf:1: Warning: Found 9 more sprites than sprite 0 reports.

that's the wrong command, as it misses some steps that are done by the makefile.

use grfcodec in the aforementioned objs/extra_grf directory on the properly processed sprites/openttd.nfo

also, you can ignore nforenum errors by adding a "-" at the beginning of the line in the makefile, but changing nforenum the way Rubidium mentioned is the better alternative.

[ic111]

(post written with respect to Rubidiums post)

Ok, this solves the problem with nforenum, i.e. I have no nforenum related errors left, and assume that make now does the right thing generating the openttd.grf automatically.

However, I still run into the problem that in spritecache.cpp, function GetRawSprite, I get a SpriteCache with id 4249, type ST_NORMAL, although one with type ST_RECOLOUR is expected. I read id 4249 in the generated file objs/extra_grf/sprites/openttd.nfo (files attached below), where my table lands with that id; and placed that id in sprites.h:

Code: Select all

static const PaletteID PALETTE_ALL_BLACK           = 4249;

Is this correct, or do I need some other id, if the latter, where to I get it from?

The corresponding stracktrace to my problem is:

Code: Select all

#0  GetRawSprite (sprite=4249, type=ST_RECOLOUR, allocator=0x0) at /home/wol/openttd/trunk_mhl/src/spritecache.cpp:831
#1  0x083e5e5a in GetNonSprite (sprite=4249, type=ST_RECOLOUR) at /home/wol/openttd/trunk_mhl/src/spritecache.h:47
#2  0x083e7f0d in DrawSpriteViewport (img=4493, pal=4249, x=-128, y=-320, sub=0x0) at /home/wol/openttd/trunk_mhl/src/gfx.cpp:800
#3  0x086c52bb in ViewportDrawTileSprites (tstdv=0x900f2a8 <_vd+40>) at /home/wol/openttd/trunk_mhl/src/viewport.cpp:1638
#4  0x086c5c31 in ViewportDoDraw (vp=0x91035a8, left=0, top=0, right=1244, bottom=1896) at /home/wol/openttd/trunk_mhl/src/viewport.cpp:1833
#5  0x086c6028 in ViewportDrawChk (vp=0x91035a8, left=0, top=0, right=311, bottom=474) at /home/wol/openttd/trunk_mhl/src/viewport.cpp:1898
#6  0x086c5f44 in ViewportDrawChk (vp=0x91035a8, left=0, top=0, right=623, bottom=474) at /home/wol/openttd/trunk_mhl/src/viewport.cpp:1889
#7  0x086c5ee4 in ViewportDrawChk (vp=0x91035a8, left=0, top=0, right=623, bottom=948) at /home/wol/openttd/trunk_mhl/src/viewport.cpp:1885
#8  0x086c610f in ViewportDraw (vp=0x91035a8, left=0, top=0, right=623, bottom=948) at /home/wol/openttd/trunk_mhl/src/viewport.cpp:1916

i.e. it finds out correctly that it must paint the landscape tile with an altered palette (pal = 4249 above), and then tries to load it as sprite, but unfortunately finds a normal sprite. Do I have any chance to inspect at runtime which sprite I actually get? I suspect, anything interesting is located in the void *ptr of SpriteCache, or do I have any chance to locate the input file where the sprite was defined based on the other fields? But how to inspect that void *ptr...

Or is this due to the fact that I´m probably the first one that uses a recolour sprite outside Action 5 / Type 0A, i.e. do I need some extra logic to make it work with my new Type 18?

The problem described here occurs on starting a game, I furthermore still get the message

Code: Select all

dbg: [grf] Palette sprites are missing

on loading the intro game, but I´m not sure wether this is just due to the fact that the intro game doesn´t know anything about my new sprite, or due to the problem above, or due to another problem...

[Eddi]

I read id 4249 in the generated file objs/extra_grf/sprites/openttd.nfo (files attached below), where my table lands with that id; and placed that id in sprites.h:

Code: Select all

static const PaletteID PALETTE_ALL_BLACK           = 4249;

Is this correct, or do I need some other id, if the latter, where to I get it from?

no, these two numbers have no relation to each other whatsoever.

you need to find this line in src/table/sprites.h:

Code: Select all

static const SpriteID SPR_NEWGRFS_BASE = SPR_EMPTY_BOUNDING_BOX + EMPTY_BOUNDING_BOX_SPRITE_COUNT;

this marks the first free sprite ID after all current base set sprites have been processed. you need to use this value as your sprite ID, and bump the newgrf base by one.

e.g.

Code: Select all

static const SpriteID SPR_PALETTE_ALL_BLACK = SPR_EMPTY_BOUNDING_BOX + EMPTY_BOUNDING_BOX_SPRITE_COUNT;
static const SpriteID SPR_NEWGRFS_BASE = SPR_PALETTE_ALL_BLACK + 1; // should probably also define a *_SPRITE_COUNT constant

this SPR_PALETTE_ALL_BLACK you can then use as your palette ID, and this also has to be used in newgrf.cpp to define your action 5 type.

[ic111]

Thank you and sorry for asking so many questions... But nevertheless, I am stuck with that sprite being not loaded. Specifically, I see that message (code in newgrf.cpp, function CheckForMissingSprites):

Code: Select all

			if (!SpriteExists(type->sprite_base + j)) {
				DEBUG(grf, 0, "%s sprites are missing", type->name);   /******* Line 5703 is here *******/
				missing = true;

Thus, SpriteExists returns false. SpriteExists in spritecache.cpp looks as follows:

Code: Select all

bool SpriteExists(SpriteID id)
{
	if (id >= _spritecache_items) return false;

	/* Special case for Sprite ID zero -- its position is also 0... */
	if (id == 0) return true;
	return !(GetSpriteCache(id)->file_pos == 0 && GetSpriteCache(id)->file_slot == 0);
}

The constant in sprites.h now looks as follows; the corresponding openttd.nfo can be seen in my post above:

Code: Select all

static const PaletteID PALETTE_ALL_BLACK           = 6160;//SPR_PALETTE_BASE;  /* Tried both variants */

And gdb (with a breakpoint at line 5703 you see above) tells me:

Code: Select all

Reading symbols from /home/wol/openttd/trunk_mhl/bin/openttd...done.                                                                                                                                                                         
(gdb) break newgrf.cpp:5703
Breakpoint 1 at 0x84a9434: file /home/wol/openttd/trunk_mhl/src/newgrf.cpp, line 5703.                                                                                                                                                       
(gdb) run                                                                                                                                                                                                                                    
Starting program: /home/wol/openttd/trunk_mhl/bin/./openttd                                                                                                                                                                                  
warning: Could not load shared library symbols for linux-gate.so.1.                                                                                                                                                                          
Do you need "set solib-search-path" or "set sysroot"?                                                                                                                                                                                        
[Thread debugging using libthread_db enabled]                                                                                                                                                                                                
Using host libthread_db library "/lib/i386-linux-gnu/i686/cmov/libthread_db.so.1".                                                                                                                                                           
[New Thread 0xb07eeb40 (LWP 9956)]                                                                                                                                                                                                           
[Thread 0xb07eeb40 (LWP 9956) exited]                                                                                                                                                                                                        
[New Thread 0xb07eeb40 (LWP 9957)]                                                                                                                                                                                                           
[New Thread 0x8fdfeb40 (LWP 9958)]                                                                                                                                                                                                           
[Thread 0x8fdfeb40 (LWP 9958) exited]                                                                                                                                                                                                        
[New Thread 0x8fdfeb40 (LWP 9959)]                                                                                                                                                                                                           
[New Thread 0x8f3ffb40 (LWP 9960)]                                                                                                                                                                                                           
[Thread 0x8fdfeb40 (LWP 9959) exited]                                                                                                                                                                                                        
                                                                                                                                                                                                                                             
Breakpoint 1, CheckForMissingSprites () at /home/wol/openttd/trunk_mhl/src/newgrf.cpp:5703
5703                                    DEBUG(grf, 0, "%s sprites are missing", type->name);
(gdb) p type->sprite_base
$1 = 6160
(gdb) p j
$2 = 0
(gdb) p SPR_PALETTE_BASE
$3 = 6160
(gdb) p _spritecache_items
$4 = 9216
(gdb) p _spritecache[6160]
$5 = {ptr = 0x0, file_pos = 0, id = 0, file_slot = 0, lru = 0, type = {m_val = 0 '\000'}, warned = false, container_ver = 0 '\000'}
(gdb) 

So apparently, both file_pos and file_slot are 0. Is there somewhere code I have to adapt for loading such a sprite into the spritecache?

[Eddi]

the most likely cause is that it's not actually using your modified openttd.grf. do you use opengfx by any chance? then opengfx_extra.grf needs the same change.

[ic111]

Arg, probably you are right.

In .openttd/openttd.cfg, graphicsset is not set, but in the game options, OpenGfx is chosen (but that menu doesn´t allow me any other choice either).

Is there any option I can write into openttd.cfg to get back to openttd.gfx?

EDIT: Ah, I see, the name from the obg file; unfortunately OpenTTD says it has not all files for original_dos.

Ok, I think I´ll stop here for today.

Generally, I think I cannot put this into a release of the mhl patch right now anyway --- I cannot sensefully force people to recompile nforenum as I did, or alter OpenGfx etc only to install my patch.

Thus, I would regard this piece of work (when it is finally verified to work) a patch we now know that it can be quickly applied once needed, i.e. something one would put into the patch queue as one of the very last steps of development.

In the meantime, I am open to any other feedback regarding the patch queue...

[Eddi]

it would still be useful to know that it actually works before putting it aside...

also, i think nforenum could be a bit less "the world is going to end!!!!" on certain types of errors, so compiling would actually continue on these things.

[ic111]

it would still be useful to know that it actually works before putting it aside...

Verified, it works. I tested it with the original TTD graphics, as I didn´t want to dig into OpenGfx as well, and also made a test with replacement color 05 instead of 01. Then the outside is some sort of grey, i.e. it actually replaces the color.

Attached are the patch that will finally replace patch 30 in the queue, and in order to have everything at one place, also in an unchanged manner the small patch against nforenum Rubidium posted above.

Patch 999 is meant to be applied on top of version 29 of the mhl patch.

also, i think nforenum could be a bit less "the world is going to end!!!!" on certain types of errors, so compiling would actually continue on these things.

Indeed.

Then I am again in a state where I am not aware about any bugs / tasks I have to fix in the mhl patch, i.e. state "Waiting for input"

[kezhman]

Hi,

I have been following this thread for some time now, the topic is very interesting.

Given the amount of work youguys already did, can you anticipate how much time until we can actually see more height levels implemented in the game?
Would you say weeks, few months or years?

I see its a heck of a change, so I assume its not going to be a simple NewGRF but a new version of OTTD maybe?

I saw a thread the other day about this develepment not being considered to go in the trunk?

thanks for any answer!
your anxious and moral supporter

[ic111]

First of all: This is mostly not under my control. Thus, I can only write here, what would technically be possible:

Technically, this patch is built up quite modular. Some major topics of this patch could, on a technical level, be addressed separately, could even be merged into trunk separately at different points of time:

- Terraforming: The algorithm changes to support new heightlevels, but one could easily use the new algorithm in the old world of fixed 16 heightlevels. I.e., one could test that algorithm, merge it into trunk if it is confirmed to work, and then put the topic "mhl patch" aside again, without any problems.

- Viewport: The same applies here, the new Viewport code can of course work in the old world of fixed 16 heightlevels.

Those two aspects of game already account for major parts of this patch. Several other patches are actually quite small, just making some rather minor adjustments here and there. What one can do with them, which one can actually be applied separately without breaking anything would have to be discussed for each patch separately.

The state of testing is that I already played several long single-player-games with that patch being enabled.

So, IMHO, if desired by the devs, it could be ready for trunk quite fast: Discussions & feedback, patch by patch, test games by others than me with reports wether anything went wrong, especially test games in multiplayer-mode (just because I never did that). That would roughly spoken be the tasklist to process.

With "Discussions" being the most important task on that list, as for hardly any part of that patch, I have ever heard feedback like "it´s ok" - I just notice in my games, that things like terraforming and viewport code perfectly work for me.


The conclusion: I cannot say anything about the amount of time you ask for, just because this IMHO has stopped being a technical question a long time ago (I started this patch towards the end of 2008), it is just a question of what (in the opinion of the devs) should be part of the game, and what not.

[kamnet]

Have you ever formally submitted your patch to the developers for testing and review?

[ic111]

There is an old bugtracker ticket, although started long ago; at some point in time I stopped using it.

However, the state of this patch is no secret, i.e. my expectation is and was always that if there is interest in it, I somehow will hear about it.

And of course, if I am asked to use such a ticket again I will do so; just in the absence of such a message, I don´t see the point in opening a second point of discussions beside this forum thread.

[planetmaker]

Have you ever formally submitted your patch to the developers for testing and review?

Yes, he kinda did. And the latest versions of the queue really look reasonably clean from a brief check. What it really needs is a decent review now. Which needs for a queue like this really some time and motivation and decision (not that there hasn't been quite a bit time since... :S ). And iirc it needs the decision whether we want to add another byte to the map array (but I might err here) which increases all savegames by ~14%

[ic111]

Yes, it would be the transition from 9 bytes per tile to 10 bytes per tile (+ 11% storage for the Tile + TileExtended array). Where, of the 10 bytes, 4 bits would be unused for future additions, as the patch transforms

byte Tile.type_height

into

byte Tile.type [type_height minus the height]
byte Tile.height

(and shifts m6 to TileExtended - I think someone proposed this for byte alignment reasons)

i.e. the second half of type_height is left unused.

[ChillCore]

I had a quick browse through v29.

Besides a few minor things it looks pretty good.

To do:
- A bit of coding style here and there, namely spaces to allign code and comments that use "// comment. " where it should be "/* comment. */". Most of these can be fixed in the patches themselves, without messing up the patches that follow, since they are on 1 line.
- The smallmap elevation colours (green and purple) have a few too many duplicates still between the normal colours and the grey-purple-ish top. I ran out of usable (properly visible) coulours to go upwards and was going to instead work my way down with the geys.never got around to doing this and it slipped my mind.


Would you like me to fix the coding style or would you prefer to do it yourself, it should not take too long to do.
Unfortunately, my time is rather limited still, pesky real life, so I can not make promisses on when I will get to those colours as it is a slow process of changing values, recompiling and gazing at the screen to do this. (since these duplicates are really high up I do not think many peeps will ever see them but still).