MAP REWRITE patch 0.9

[mkxx]

I know, but in GetRailType(...) you still use GET_M2(...) or _m[tile].tt.rail.type

If we replace all _m[tile].m2 occurences with GET_m2(...), we can then alter data structure (and GET_M2) with no pain. (Or with less pain than without GET_M2)

[ValHallA|SW]

It is unnecessary. BEFORE the switch to the new map array, GetRailType(...) should be implemented instead of GET_M2(...).

GET_M2(...) adds ABSOLUTELY nothing, as you can change it to GetRailType with the same effort, giving much more readable code. Give me one good reason to use GET_M2 instead of GetRailType?

[mkxx]

ValHallA|SW: 'Cause you are not changing only GetRailType but you change A LOT (100...200) occurences of _m[tile].m2 -> you think that better work is to change all of them manually? (Yes, we need to change them manually. But until we change them manually and we do not have GET_M2(), we will not be able to run ottd 'cause of incompatible data structures)

[ValHallA|SW]

But until we change them manually and we do not have GET_M2(), we will not be able to run ottd 'cause of incompatible data structures)

Why not? While the change is happening ottd will still use the old data structure and both GetRailType() and _m[tile].m2 will work. Before the new map array is introduced, all changes will have to be made.

Your step is a pretty useless one imnsho, and would only be useable if the new map array would arrive really soon.


and it won't

[egladil]

Yes, it is better to change them all manually. There are many reasons for this. The most obvious one is that it would make the code much more easy to read.

-edit-
Removed since i didn't see ValHallA|SW post until after i pressed submit.

[Graphite]

Right... regardless of the implementation, at some point _m[tile].tt.rail.type will have to be referenced. From what I can tell the differences between mkxx's suggestion and the others are the following:

mkxx:
- uses two functions containing huge switch-statements that select the correct return-value based on a parameter, thus centralising any access to _m[tile].tt.rail.type and any other map-implementation specific data.
- if the internal representation is changed, the respective portions of the switch-statements are changed to fit the new representation.

others:
- use lots of simple functions with descriptive names that centralise any access to _m[tile].tt.rail.type and any other map-implementation specific data.
- if the internal representation is changed, the respective functions are changed to fit the new representation.

See the pattern, mkxx? Both centralise any reference to the internal representation of the map-array into a single location. Yours are in two big functions, theirs are in lots of tiny ones. The main difference between the two is that their version uses functions with descriptive names, so wherever they are used, you can immediately see what they're to access. This is NOT the case with your solution.

From a practical point of view, the other solution is faster too as the overhead for the function call is the same, but in your case it has to process this huge switch-statement as well.

[Korenn]

Yes ) You are near
[..code..]
You do not have to touch whole sourcecode, couse everywhere you have only GET_M2(tile). Or you can then replace GET_M2(tile) with proper name of desired variable ("_m[tile].tt.rail.type" for example)

(New data structure is not correct, it was only an example)

but we WANT to touch the whole sourcecode, because it's a whole lot of chaotic crappy spaghetti at the moment. You're missing a large part of the point of the map rewrite.
Yes a new map array needs to be introduced, BUT at the same time, the code accessing the map array needs to be made more readable.

The net result of your code is that only the bits accessing the arrays directly (so _m) are changed, but the bit shifting and other nonsense in there is not changed at all. Which makes your code more or less a waste of time.

[edit: typo]

[LordOfThePigs]

The point is, this has been discussed at the last developper meeting, and the approach chosen has been unanimously accepted by all coders. And I guess OTTD coders are pretty experienced.

On a final note, I'd like to mention that the code is *already* in the process of beeing rewritten as decided. There is still a lot of work to do, but to the code still works and compiles. The snippet I posted on the first page is an actual bit of code that is waiting to be commited into SVN.

Beleive me. This matter has been discussed thouroughly and the way the devs have chosen is clearly the best.

And Korenn is perfectly right!

@graphite: The point isn't there. What we want isn't only to abstract the access to the map array from it's internal structure, but also the whole code. Right now, huge portions of OTTD's code are totally linked to internal structure of the map array (through weird bit-masking and bit-shifting and all sorts of other crappy tricks). Whole loops depend on that kind of nastly stuff to run correctly. All that code HAS to be rewritten, one day or another, and mkxx way doesn't solve the problem.

[Brianetta]

see Brianetta's post on first page.
We need temporary RAW getter and setter for introduce new map array structure.

I never once said temporary. I think direct map access should be avoided if at all possible. Encapsulation is a Good Thing - I was taught that at university, and I still think that reasoning is sound.

One day, in the distant future, the map might be rewritten again. This would be much less painful if we don't reverse the good encapsulation work.

[Darkvater]

Code: Select all

Get_M2(tile) {
   switch(GetTileType(tile)) {
      case MP_GROUND: return _m[tile].tt.ground.foo | (_m[tile].tt.ground.bar<<4); break;
      case MP_WATER: return _m[tile].tt.water.barfoo; break;
      ...
   }
}

You do not have to touch whole sourcecode, couse everywhere you have only GET_M2(tile). Or you can then replace GET_M2(tile) with proper name of desired variable ("_m[tile].tt.rail.type" for example)

Something like that will kill performance, so it is not going to happen.

The question is whether we want Get/Set/Is-sers for everything or something like tile.track_info[1].reserved is enough. Yes, getsetters are nicer and you can go wild with assertions (most of them useless). Once the format is fixed the second, direct access is enough (you can freely increase 'reserved' size without changing anything else).
However if the format changes, getset is a lot easier to maintain. But do we really want ~200-300 accessor functions (tile.h from map/ branch, with Get/Set/Is for every member of the Tile struct)? It is a valid question.

[mkxx]

to Darkwater: this was thought like temporary solution. When devs will change OTTD data structure, it will take lot of time to rewrite all accessors like IsTileType, GetTileTrack, SetGroundType, GetFooBar, SetBar, ...
for use new data structure (that means: not "GB(_m[t].m3,0,3)", but _m[t].tt_bar.foo). Until the code will be compeltely rewritten to be compatible with new data structure, nobody will be able to compile OTTD source.
- For this state of map_rewrite, we should replace all direct accesses to map array (= occurences of _m[t].xx in accessors) with one global accessor (like GetVarInOldMapStyle(t,MAP_M3)).
(GetVarInOldMapStyle returns variable, which looks like old m3 variable. But that variable is build from new data structure vars (like "return _m[t].bar << 3 || _m[t].foo;")).
Then programmers need to replace occurences of "GetVarInOldMapStyle" uses with direct accesses to new data structure ("GetVarIn...(t,MAP_M3)" => "_m[t].foo.bar.some_value").

- I understand that rewrite all accesses to this global wrapper - as I said before - will take A LOT of time. But I have written simple application, which does this action for us automatically. (That means: replacing all occurences of "_m[t].xx" to something like "Get/SetVarInOldMapStyle(...)" will take maximally 10minutes.) Then we need only to maintain functions Get/SetVarInOldMapStyle, which sets/gets variable in same syntax like in old map array. Advantage of this global wrapper is when ottd-team will change data structure, devs need to rewrite only one function (that wrapper) and OTTD runs normally (little bit slower, 'cause of wrapper). But this is not final state. Then we need to manually rewrite all occurences of Get/SetVarInOldMapStyle with direct access (without wrapper) to new data structure. After source code rewrite - when new data structure is used (and no m1,m2,... variable is demanded), the wrapper Get/SetVarInOldMapStyle should be removed.

Sounds little bit crazy, but it saves lot of time and work.

[DaleStan]

When devs will change OTTD data structure, it will take lot of time to rewrite all accessors like IsTileType, GetTileTrack, SetGroundType, GetFooBar, SetBar, ...
Until the code will be compeltely rewritten to be compatible with new data structure, nobody will be able to compile OTTD source.

WRONG!![0] The accessors will change *as* the data structure changes. That's why they exist. To hide the actual structure from the program. If you have to hide the data structure from the accessors, then they aren't doing their job properly, and the program might as well use the sub-accessors instead.

[0] Be thankful there's no [blink] tag; I'd be using it if it existed. Anything to make mkxx see this word.

(GetVarInOldMapStyle returns variable, which looks like old m3 variable. But that variable is build from new data structure vars

How does this make it unnecessary to change the accesses in-sync with the data-structure changes? Sure, the accesses are only in one function, but they still have to be changed in-sync.
And how do you eliminate the massive, performance killing switch statement? (The only way you're going to satisfy me on this one is to post (pseudo)code for GetVarInOldMapStyle.)

like "return _m[t].bar << 3 || _m[t].foo;"

That code will never work. Go away. And don't come back until you can explain what's wrong, and how to fix it.

[richk67]

Go away. And don't come back until you can explain what's wrong, and how to fix it.

Way, way, way out of order.

[DaleStan]

Just to clarify, I'm not talking about the variable widths there.
It is perfectly valid C, and I know what it does, I just fail to comprehend how it could be useful. And I stand by my statement, at least until someone can explain how that construct is useful.

[ebik]

Just to clarify, I'm not talking about the variable widths there.
It is perfectly valid C, and I know what it does, I just fail to comprehend how it could be useful. And I stand by my statement, at least until someone can explain how that construct is useful.

The only usefullness of what mkxx proposed is to introduce new data structures now, instead of waiting after all the code is purified. (I don't know how it will do with loadgame (i didn't see the code yet...), but other things should go well). So developers waiting for new map array can start their work sooner !, while the others will be hunting and killing the temporary getters and setters.

[DaleStan]

I was not talking about mkxx's concept either. (Well, I was other places, but not there.)

I was talking about the "return _m[t].bar << 3 || _m[t].foo;" statement.

It is valid, but it has no logical meaning.

[ebik]

Sorry, I just did wrong quotation.

And double sorry that I did not see New Map Rewrite effort thread before.

[Korenn]

I was not talking about mkxx's concept either. (Well, I was other places, but not there.)

I was talking about the "return _m[t].bar << 3 || _m[t].foo;" statement.

It is valid, but it has no logical meaning.

if you really want to nickpick like that, yes it does have logical meaning. shifting by 3 bits can mean you shift out 3 bits.
so then it would be equivalent to

return _m[t].bar & 0x1F || _m[t].foo

if _m[t].bar is an 8 bit variable.
Still horrible style, but it definitely has logical meaning. So go away and come back when you've learned some manners.

[prissi]

Just to point to the oviously not really clear thing (although it should burn in your eyes; even more since crappy compilers accept this without warnings):

return _m[t].bar << 3 || _m[t].foo;

is equal to

return (_m[t].foo!=0);

due to the accidental use of the logical or which has only 0 and not zero (most compiler reutrn 1, few -1) here. Since the lower bits of the shifts are zero, most compilers that are not completely draindead will remove the first part.

Not complety bad, but still risky is the use of the shift operator without brackets around. Especially next to an arithmetic or invertation they will do domething completely different.

And now, back to topic ...

[DaleStan]

so then it would be equivalent to

return _m[t].bar & 0x1F || _m[t].foo

No, that's just a proof that it is valid. To prove "logical meaning", you have to come up with a situation where that would be a useful operation.

prissi: Sorry, no. _m[t].bar<<3 is not guaranteed to be 0. It's equivalent to "return (_m[t].bar<<3)!=0 || _m[t].foo!=0;"