Page 3 of 5
Re: resurrection of diagonal level and clear patch
Posted: 31 Mar 2009 00:39
by Eddi
err... if you don't mind the question... WTF does this have to do with "java"?
Re: resurrection of diagonal level and clear patch
Posted: 31 Mar 2009 06:23
by Terkhen
Eddi wrote: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.
Re: resurrection of diagonal level and clear patch
Posted: 31 Mar 2009 14:19
by fonso
Eddi wrote: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).
Re: resurrection of diagonal level and clear patch
Posted: 31 Mar 2009 15:08
by Eddi
so basically the difference is that java-style uses methods (next, hasNext) and c++-style uses overloaded operators (==, ++ and *)?
Re: resurrection of diagonal level and clear patch
Posted: 31 Mar 2009 21:00
by fonso
Eddi wrote: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.
Re: resurrection of diagonal level and clear patch
Posted: 10 Apr 2009 08:20
by 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
Re: resurrection of diagonal level and clear patch
Posted: 16 Apr 2009 22:02
by 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]
Re: resurrection of diagonal level and clear patch
Posted: 21 Apr 2009 19:42
by fonso
Updated. The devs have chosen c++ style to be their favourite, so the java style patch is obsolete now. See first post.
Re: resurrection of diagonal level and clear patch
Posted: 16 Jun 2009 22:02
by fonso
Updated to current trunk. See first post.
Re: resurrection of diagonal level and clear patch
Posted: 17 Jun 2009 00:31
by micomico
fonso wrote:Updated to current trunk. See first post.
Enough said.
Re: resurrection of diagonal level and clear patch
Posted: 17 Jun 2009 19:18
by 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!
Re: resurrection of diagonal level and clear patch
Posted: 14 Sep 2009 18:26
by 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.
Re: resurrection of diagonal level and clear patch
Posted: 21 Nov 2009 11:10
by 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?
Re: resurrection of diagonal level and clear patch
Posted: 21 Nov 2009 11:51
by 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.
Re: resurrection of diagonal level and clear patch
Posted: 21 Nov 2009 12:01
by 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.
Re: resurrection of diagonal level and clear patch
Posted: 21 Nov 2009 12:51
by 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.
Re: resurrection of diagonal level and clear patch
Posted: 06 Dec 2009 00:29
by fonso
The problems with docks and the magic bulldozer have been fixed. You can find an updated version at the usual places.
Re: resurrection of diagonal level and clear patch
Posted: 06 Dec 2009 10:04
by Terkhen
Everything is working correctly now. Thanks!
Re: resurrection of diagonal level and clear patch
Posted: 20 Dec 2009 17:35
by petert
Is this where I should post binaries for the diaglvl patch? Or should I post them at all?
Re: resurrection of diagonal level and clear patch
Posted: 20 Dec 2009 18:53
by 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.