MAP REWRITE patch 0.9

[DaleStan]

Say you have property *foobar in the old representation. In the new one, it gets split into *foo and *bar.

With the getter/setter functions you only need update 1 place - change the functions to get/set the *foo and *bar instead of just *foobar

That doesn't really help anything; you still have to reconstruct foobar in order to return it, because you can't know whether the function calling the getter is looking for foo or bar (or oba, for that matter).

In order to do that, you need to split GetFooBar() into GetFoo() and GetBar(), and then figure out which function each call to GetFooBar() needs to use.

[mkxx]

We need to get the get/set accessors implemented before any changes to the map array itself.

Changing the map array first (as this patch does) is doing it the wrong way around.

I was talking about RAW map array getters and setters. I undestand that you are talking about abstraction (eg. GetTileType(), SetTileType(), HasTileType()) but if you will change the whole Tile struct, you'll have lot of work then... This patch groups all RAW map array accesses into two functions. Is easiest to maintain only data structure and two functions than all the getters and setters.

[peter1138]

If you have a script to replace the code with get/set accessor functions, then sorry, I don't want to see it.

This is not something which can be scripted.

[mkxx]

In order to do that, you need to split GetFooBar() into GetFoo() and GetBar(), and then figure out which function each call to GetFooBar() needs to use.

But you still access to old map array in these functions. This patch is something like small (temporary) step between old map array and new map array. If you replace some data structures in Tile struct, there is very easy to make game work with this patch. What you need to do is only to rewrite raw map getter and setter functions.
After new data structures will be created, we can access them directly in OTTD sourcecode - without my getter and setter functions.

>> Look into the picture in my next post...

[Korenn]

the point is that you're now already going a step too far.

first create a patch (not a script) that implements getters and setters for the current map arrays. THAT would be helpful, what you're doing now doesn't help a lot, you're exchanging one bad thing by something a bit less bad, instead of paving the way so that we can put in a good thing.

[mkxx]

My English is bad. Sourcecode is better than words - is that understandable??[/img]

[Korenn]

but why the last step? the point is having the getters and setters, not replacing the map array with a new kind of map array.

[mkxx]

but why the last step? the point is having the getters and setters, not replacing the map array with a new kind of map array.

see Brianetta's post on first page.
We need temporary RAW getter and setter for introduce new map array structure.
As long as we access map array only via RAW getter and setter, we can edit whole data structure of Tile struct.
After new Tile struct will be introduced, developers should edit main source code and replace all RAW getter and setter occurences BACK with direct access to map array (but with direct access to new data structure) - you understand?

[peter1138]

mkxx. Please stop and think.

These are the type of get/set accessors we want:

Code: Select all

static inline TileType GetTileType(TileIndex tile)
{
  assert(tile < MapSize());
  return GB(_m[tile].type_height, 4, 4);
}

static inline void SetTileType(TileIndex tile, TileType type)
{
  assert(tile < MapSize());
  SB(_m[tile].type_height, 4, 4, type);
}

static inline bool IsTileType(TileIndex tile, TileType type)
{
  return GetTileType(tile) == type;
}

And you know the best bit? We already HAVE these particular accessors...

[mkxx]

mkxx. Please stop and think.
...
And you know the best bit? We already HAVE these particular accessors...

peter1138: Stop and think. Until new map array will be introduced, you'll need some functions, which provide compatibility between source code and new (particularly new) map array.

[LordOfThePigs]

Sorry, but this stuff is just not helping code legibility. One other huge benefit of the way we are wanting to do it is that we have to go over a huge part of the code and rewrite it. This gives us the occasion to make it legible. Check the snippet I posted on the first page and tell me which version of the two you can understand....

*waits until you finished reading*

get my point?

Maintainability is also something that is really important. The whole point of the map rewrite is that we want to remove direct access to *any* data structure forever! The problem is your patch is fine as long as we want to change the structure once and for all, and that is not what we want to do. We want to be able to change the internal structure at any time (yes I know: plan ahead... blah blah blah... but we all know what ends up happening), and with minimal effort.

To make it short, we need:

- Legibility
- Flexibility

both of these bring us to Maintainability

Your patch doesn't bring any of these two point IMO.

[peter1138]

Until new map array will be introduced, you'll need some functions, which provide compatibility between source code and new (particularly new) map array.

Those accessor functions ARE the compatibility layer...

[ValHallA|SW]

Okay, If I am correct, this is what you are trying to do:

current:

Code: Select all

if(<weird stuff>) {

}

your code:

Code: Select all

if(<simple stuff>){

}

function simple_stuff {
   <weird stuff>
}

your code w/ map rewrite:

Code: Select all

if(<simple stuff>){

}

function simple_stuff {
   <NEW weird stuff>
}

or:
You are getting the code that will be changed after the map rewrite in a separate function


*edit: peter1138 just told me this wasnt really the point :'(


another edit:
The last stage in your image is one the devvers do not want to get to. They want accessor functions not only to make sure the shift will be easy, but also to make accessing parts of the new map array simpler. Your image keeps those things 'hard'; ie with _m[t].tile instead of GetTileType

[DaleStan]

I'm not sure who "you" is, but that's basically what's needed.

Pretty much, every instance of GB/SB, every instance of the bitwise booleans (&, |, ^, and ~), and every reference to _m needs to be moved into a collection of accessors in a single source file, and then replaced with calls to those accessors.

That's not what this patch does. (_m[tile].m5&1) needs to be changed into something like GetBridgeDirection(tile). Then, when diagonal bridges appear, that'll continue to work; GetBridgeDirection will just return one of four values instead of one of two.

(Without looking up the context of that line, I'm not exactly certain that's what that does, but that's the only instance in TTD where bit 0 of L5 is significant by itself. And the point is the same, even if the function is incorrect.)

[mkxx]

DaleStan:
- "you" - that was grammatic mismatch :) replace it with "we" or "openttd devs" ;)
- I understand you. You want to separate drect access to map array from source code. Yes.



- My idea is... Forget my last patch and everything I wrote here before.
Here is described the process of map rewrite:
Step 1. Devs are creating the "access layer" between source code and actual map array (create accessors for every getted/setted value in actual map array).
Step 2. After that, new map array will be introduced (replace byte variables m1...m5 with structs and unions). At this time, "access layer" is not compatible with new map array.
Step 3. Whole "access layer" needs to be rewritten, because this code is incompatible with new data structure.
Step 4. After "access layer" will be rewritten, we can compile OTTD again. (As we won't able to do in step [3]).

- Step 3 will take a lot of time. For a long time, it won't be possible to compile latest source code and check it for errors, etc. It will be possible to compile source code after Step 3.

- I will describe idea of "temporary access layer" (between "access layer" and map array). The "temporary access layer" should be used only in Step 3.
- What is "temporary access layer"? This is couple of functions - wrappers.
Functions in "access layer" still want to access to map array in old style (via m1...m5), but we have new data structure - completely different.
Therefore we'll use "temporary access layer" wrappers everywhere in "access layer". ***
(Example: Wrapper gets request to m1 variable for given tile. Then it concatenates some variables from new data structure and returns them in format of old m1 back to "access layer". (Similary works the setting wrapper).)
- Why is "temporary access layer" important? Until all functions in "access layer" will be rewritten, we won't be able to compile ottd 'cause of incompatible source code.
We'll replace direct access to map array in "access layer" with wrappers from "temporary access layer". (Source code will be compatible with new map array and we are able to compile, run and test ottd.)
Now we can easily rewrite "access layer". Functions need to access new data structure directly, not through "temporary access layer".
After Step 3 will be completed ("access layer" will be completely rewritten to use new map array), "temporary access layer" will be removed because no functions in "access layer" use it.

*** = This will be done in batch operation - not manually. I have script on my pc, which does it. (With this script I made the big patch, which was here before.)

This patch is only temporary thing, which provides full functionality of ottd between steps 2 and 4.
The one difference with/without "temporary access layer" is that
- without: we will not be able to compile and run latest ottd until whole "acess layer" wil be rewritten
- with: we need to write "temporary access layer" wrappers which is little work, then we'll be able to compile ottd during "access layer" rewrite process.

Is it now understandable??

[Dextro]

No you got it wrong! When you setup the access layer you set it up like so:

Code: Select all

function simple_stuff {
   <weird stuff>
}

Now we can just alter the <weird stuff> so it isn't weird anymore and does what you want and the function will still give out the same result, if it doesn't then I guess you don't know how to code very well and shoudln't be doing this dirty bit of the job (just kidding)

Got it now?

If not use my command-line analogy again: think of the function like a tool you need to use from a command line. The GUI (the game in this case) would pass command line switches to the tool. That tool would give out a result and the GUI would use that result.

If you alter the way the tool works it doesn't matter as long as the result still comes out. And if you add extra switches it won't be a problem because the GUI isn't made to use them and the tool still takes the old switches...

It's not a perfect analogy but I think it should work... if it doesn't then I'll screem like a little girl...

...or not

[DaleStan]

I think so. I'll repeat what you just said, and you correct me if/as necessary:

Instead of changing the accessors in-sync with the map array, you're proposing that the accessors call temporary accessors, and the temporaray accessors are changed in-sync with the map array?

And then, when the map array is fully rewritten, the accessors are changed to access the new array directly and the temp accessors are removed?

I'm failing to see how this is an improvement:
1) If the permenant accessors are changed in-sync with the array changes, then compilability is preserved.
2) If temp-accessors are used, then *they* have to be changed in-sync with the map array, in order to preserve compability.

[egladil]

A more hands on example of how the devs want to do things (as I understood it):

In pathfind.c at line 796, you can find this:

Code: Select all

if (_m[tile].m2 & SignalAlongTrackdir(track)) {
...
}

and in rail.h line 499 you can find this:

Code: Select all

static inline SignalState GetSignalState(TileIndex tile, Trackdir trackdir)
{
	assert(IsValidTrackdir(trackdir));
	assert(HasSignalOnTrack(tile, TrackdirToTrack(trackdir)));
	return ((_m[tile].m2 & SignalAlongTrackdir(trackdir))?SIGNAL_STATE_GREEN:SIGNAL_STATE_RED);
}

Combine this, and you get

Code: Select all

if (GetSignalState(tile, track) == SIGNAL_STATE_GREEN) {
...
}

As I understand it, what mkxx wanted to do instead was:

Code: Select all

#define GET_M2(tile)        _m[tile].m2

and then

Code: Select all

if (GET_M2(tile) & SignalAlongTrackdir(track)) {
...
}

and then update GET_M2 as the maparray changes. Am I correct?

Because as far as I can see, the devs aproach would make the code much easier to understand and maintain than the second one.

As for your (mkxx) steps, I don't believe step 3 is the hardest one. I would say step 1 will take the most time.

[mkxx]

Yes :o) You are near :))

Code: Select all

#define GET_M2(tile)     _m[tile].m2
if (GET_M2(tile) & SignalAlongTrackdir(track)) { 
... 
}

that's true!

But - if you change map array, youll have something like this:

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)

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

[ValHallA|SW]

However, the map rewrite includes some improvement of the code. Using "_m[tile].tt.rail.type" is unwanted and unnecessary; Using GetRailType(tile) gives much better readable code than using "_m[tile].tt.rail.type"...

Mainly you should not use GET_M2(tile), but GetRailType(tile).