resurrection of diagonal level and clear patch

[Eddi]

err... if you don't mind the question... WTF does this have to do with "java"?

[Terkhen]

err... if you don't mind the question... WTF does this have to do with "java"?

fonso did two versions of the patch, one with c++ style iterators and other with java style iterators, to let the developers decide which one is the best for OpenTTD.

[fonso]

err... if you don't mind the question... WTF does this have to do with "java"?

I assume you have done some programming. In the Java libraries that come with the JDK you have iterators over containers. They work like this:

Code: Select all

Iterator i = container.iterator();
while(i.hasNext()) {
    Object o = i.next();
    //do something with o
}

In the C++ STL you also have iterators over containers. But those are different:

Code: Select all

for(Containerclass::iterator i = container.begin(); i != container.end(); ++i) {
    object o = *i;
    // do something with o
}

Now some people like one better, some the other. I modeled the tile loop as iteration over a virtual container that would contain all or a specific selection of tiles on the map in a certain order (diagonal or orthogonal). The iterators come in one of the two versions explained above. Now the developers are supposed to choose what they like better (or give me a third alternative).

[Eddi]

so basically the difference is that java-style uses methods (next, hasNext) and c++-style uses overloaded operators (==, ++ and *)?

[fonso]

so basically the difference is that java-style uses methods (next, hasNext) and c++-style uses overloaded operators (==, ++ and *)?

Mostly. However, as always, with the C++ style iterators you can do some crazy stuff and easily shoot foot. For example, guess what is wrong here:

Code: Select all

for (Container::iterator i = c.begin(); i != c.end; ++i) {
    // do things with i
    if (condition) {
        c.erase(i++);
    } 
}

However, as I don't implement the funnier things like erase or swap, you don't get the pleasure to shoot your foot really badly. So yes, in our case there's not much difference, except for the operator/method thing.

[planetmaker]

I get numerous compile warnings - though they all point to the same problem:

Code: Select all

/Users/ingo/ottd/clientpatches/src/yapf/../map_func.h:358: warning: 'class OrthIterator' has virtual functions but non-virtual destructor
/Users/ingo/ottd/clientpatches/src/yapf/../map_func.h:428: warning: 'class DiagIterator' has virtual functions but non-virtual destructor
/Users/ingo/ottd/clientpatches/src/yapf/../map_func.h:341: warning: 'class TileIterator' has virtual functions but non-virtual destructor

[Terkhen]

I found a bug in both versions of the patch: any non-diagonal terraform including the southwest edge of the map caused an assertion (tile < mapSize()). Since fonso is spending a lot of time already with Cargo Distribution, I tried to find the error myself and finally solved it. I changed just one line in map_func.h file, at OrthIterator & operator++() (C++ style) and TileIndex next() (java style) functions:

Code: Select all

 - if(TileX(current) > x_max) {
 + if ((TileX(current) > x_max) || (TileX(current) == 0)) {
		current += TileDiffXY(0, 1) - (w);
			if (TileY(current) > y_max) {
				current = INVALID_TILE;
			}
		}

[REMOVED]

[fonso]

Updated. The devs have chosen c++ style to be their favourite, so the java style patch is obsolete now. See first post.

[fonso]

Updated to current trunk. See first post.

[micomico]

Updated to current trunk. See first post.

Enough said.

[Terkhen]

I have never understood why this patch has so little comments being as useful as it is. That's why I'm going to say it again: you are doing a great work, and not only with this patch. Thank you!

[Terkhen]

I have found a bug with the current version of the patch. If you try to build a dock (and only a dock) pressing CTRL and there is a demolished station nearby, the game will crash:

Code: Select all

/src/station_cmd.cpp:519: void GetAcceptanceAroundTiles(uint*, TileIndex, int, int, int): Assertion `w > 0' failed.

I detected it in current trunk with an updated version (I can provide it if needed) but I tested in r16581 with the patch found at the first post of this thread and the bug is present too.

Edit: I got this bug report in the TPPP thread. I reproduced the bug in r16581 using the patch posted in this thread: after activating the magic bulldozer you can't demolish industries and unmovable objects.

[Terkhen]

I have been looking at the dock bug. The problem seems to be that the DraggingDiagonal() function returns true when building docks with ctrl pressed (not always, sometimes the assert does not happen); probably because _thd.select_proc is not being set to the correct DDSP_ value somewhere, but I don't know where and how it should be set. I hacked a temporal solution adding the following condition to DraggingDiagonal():

Code: Select all

static inline bool DraggingDiagonal()
{
	return _ctrl_pressed && _thd.select_method == VPM_X_AND_Y &&
			_cursor.sprite != SPR_CURSOR_DOCK && //Hack
			(_thd.select_proc == DDSP_DEMOLISH_AREA || _thd.select_proc == DDSP_LEVEL_AREA ||
			_thd.select_proc == DDSP_RAISE_AND_LEVEL_AREA || _thd.select_proc == DDSP_LOWER_AND_LEVEL_AREA);
}

I ran more checks on the magic bulldozer bug: you can demolish unmovables and industries, but only when dragging. Using it as a single tile command fails.

Any ideas on how to solve these issues?

[fonso]

Well, I know I should give this patch more attention ... however, my time is limited and Cargodist is more important. But I'll have a look at your patch and see what I can do.

[Gremnon]

I've been experimenting with the patch recently myself actually, and I haven't experienced any odd behaviour yet. I do keep forgetting it's there though, but it's damn useful when you're trying to carve a valley for diagonal tracks.
I don't know how I manged without this patch before.

[Terkhen]

My patch changes only the line with the //Hack comment. I use the cursor sprite to force DraggingDiagonal to return false in the dock case.

The code causing the assert is at UpdateTile() in viewport.cpp:

Code: Select all

//In the case causing the assert, either x2 == x1, y2 == y1 or both.
_thd.new_size.x = x2 - x1;
_thd.new_size.y = y2 - y1;
if (!_thd.new_diagonal) {
	//These lines would make size greater than zero, 
	//but they don't get executed because DraggingDiagonal returned true.
	_thd.new_size.x += TILE_SIZE;
	_thd.new_size.y += TILE_SIZE;
}
//As a result, _thd.size.(x | y) is zero. This will cause the described assert later.

When DraggingDiagonal is called, _thd.select_proc is set to DDSP_DEMOLISH_AREA. Is there any way to make DraggingDiagonal return false when the user is not dragging?. I think that would prevent the assert in a cleaner way.

[fonso]

The problems with docks and the magic bulldozer have been fixed. You can find an updated version at the usual places.

[Terkhen]

Everything is working correctly now. Thanks!

[petert]

Is this where I should post binaries for the diaglvl patch? Or should I post them at all?

[Gremnon]

IIRC, diaglvl isn't meant to be used by itself, it's meant to be applied on top of tieliter (Or whatever it's actually called).
I'd imagine no one would object much if you posted binaries for it here though.