Complete.Org: Mailing Lists: Archives: freeciv-dev: January 2002:
[Freeciv-Dev] Re: [PATCH] Map cleanups (PR#1208) 		Home

[Freeciv-Dev] Re: [PATCH] Map cleanups (PR#1208)

[Top] [All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index] [Thread Index]
To: freeciv-dev@xxxxxxxxxxx
Cc: bugs@xxxxxxxxxxxxxxxxxxx
Subject: [Freeciv-Dev] Re: [PATCH] Map cleanups (PR#1208)
From: jdorje@xxxxxxxxxxxxxxxxxxxxx
Date: Mon, 7 Jan 2002 01:18:57 -0800 (PST) 
Ross W. Wetmore wrote:


At 03:45 PM 02/01/06 +0100, Raimar Falke wrote:

On Sun, Jan 06, 2002 at 04:32:02AM -0800, jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:

Ross W. Wetmore wrote:


<snip snip -- a lot of needless context removed...>


[Patch 1: mapclean_00]
On the other hand, this will make many existing patches to these files (map.[ch]) fail to apply. In most cases this should be easily fixable. It is in any case a short-term problem.
Almost any patch of more than trivial size has a pretty high probability of this :-). 

Yes, but this one might appear to some to have no other purpose :-).
Don't get me wrong - I'm in favor of the cleanup, and I don't have any strong objections to your ordering. But as this is the strongest objection (IMO) to running indent over all code too, I felt it worth mention here.
I think you're getting too defensive here. Save it for things I'm actually criticizing (see below) :-).
I do think these should eventually all go into a separate file topology.[ch], and if there is some agreement on this I'd provide a separate patch to accomplish this. So it shouldn't stand in the way of inclusion. 


I think that it is hard to come up with a good (everybody agree to it)
organization. Also a seperation like map_topology.[ch],
map_tiles.[ch], map_part3.[ch],... may be better. The borders are just
more clear this way. So I think it should be applied but there may be
other opinions.
A clumping and any discussion of what is related to what, is a not unreasonable first step to some future split. Certainly if a file has 
obvious subsections, gets big enough to be unweildy, or parts need to
be specialized for different architectures this is relatively easy
once the patterns and fault lines are established.

It is probably premature to "make" a split without some of this thought
and discussion of long term goals though.

Once there is a reasonable concensus to what sort of things will be
accepted, then someone should be given the responsibility and latitude
to just go and do it, and the fights that have been fought and decided
should not be allowed a rehash unless there is some new issue, or a real abuse of privilege.
I agree (1) that such a separation is worthwhile and (2) that this change is a good first step. 


[Patch 2: mapclean_01]
* A lot of places that replace map_get_tile(x, y) with MAP_TILE(x, y). This is faster and just as correct. Some of them are in one-line if() statements which have not been separated out; Raimar may want them separated (although I imagine other maintainers won't care).
There was an earlier partial update. The presumption here is that completing it for consistency within the file is not unreasonable. 

As I said, I like the change.



It would be better IMO to get rid of MAP_TILE altogether and make
map_get_tile a macro, but that's easily done later.


Ack.



It is trivial to do this. In fact the corecleanups had these sort of
global macros but the discussion at the time was not in favour of the
sort of widespread use.

Note, all one needs to do is add the appropriate map.h #define and
rename within this file in the same way that is_real_tile() or normalize_map_pos() are handled. 

The local flavour of this was I thought part of the original decision.
Have the winds changed?
Actually, I don't think it's completely trivial. Simply swapping in the guts of MAP_TILE for a macro-ized map_get_tile will have potential side effects since arguments will be evalutated more than once. This will require scrutiny of the code, and preferably renaming it to MAP_GET_TILE.
In my quick macro-izing patch (mentioned below), I didn't do any of these checks, and everything seemed to work. 



Then we have the following comments for normalize_map_pos:
+FIXME: This should test for realness using is_real_tile() before touching + coordinates and update only if needed, thus avoiding side effects.
normalize_map_pos() has _inherent_ side effects, and trying to pretend otherwise is foolishness IMO.
Clearly you haven't looked at the (whole) patch. Side effects have been removed from the current CVS bringing them into line with the original 
corecleanup. "_inherent_" is another of those mental religious terms
not founded in reality here :-).
The "side effects" are that x and y are changed if they are real and non-normal. I am in favor of making normalize_map_pos _not_ change x and y if they're unreal (as you say, some parts of the code assume this), but against making it a defined part of the interface. But I don't think it's a sticking point; we'll call this a skirmish.
As I've said before, there are three things we could legitimately do to unreal coordinates: (1) leave them as they are, (2) wrap them linearly as if they were real (the equivalent of Gaute's wrap_map_pos), or (3) find the nearest_normal_map_pos(). #1 is my preference and yours, but defining things this way would be bad. I would write this change like so: 



void_tile-ism is the philosophy of converting bogus values to pseudo-real
values so the local code does not break. The breakage is deferred to much
later in the code where the original culprit can not be identified and
thus held to blame. It therefore spawns a fatalistic philosophy wrt dealing
with error conditions as they occur :-).
See, I program map.c internals and I don't even know what you mean by it :-).
The problem with void_tile was that it was a grosse hack. nearest_real_tile is not a hack, but has many of the same dangers. 



If you do not build this into the normalize_map_pos() interface, and it
is trivial and cost-free to guarantee it's adherence, then there will be
numerous constructs of the form if( is_real_tile() && normalize_map_pos(&x,&y) ) 
  if( am_i_sane() && normalize_map_pos(&x,&y) )
  save_values(x,y);
  if( !normalize_map_pos() && restore_values() )
plus all the places that don't do anything and make the wrong assumptions.
There are some already.
Yes, some places do assume the values aren't varied, mostly in GUI code. I believe code of the form 

void put_tile(int abs_x, int abs_y)
{
  int map_x = abs_x, map_y = abs_y;

  if (normalize_map_pos(&map_x, &map_y)) {
    ptile = map_get_tile(map_x, map_y);
    put_1_tile(ptile, abs_x, abs_y);
  } else {
    put_black_tile(abs_x, abs_y);
  }
}
is better, and helps us to avoid the current sort of problems we see in the GUI internals. My key point is that any unreal position should be treated quite differently from a real position, and encouraging usage of this form will blur that distinction. 



Making this a defined part of the interface does not break existing code
and fixes a lot more, including that yet to be written.


It is very likely that all of that code is broken anyway.



I don't consider there to be any real grounds for either alternative
I am not arguing for either of the alternatives, just saying that we should discourage this use by not making it a defined part of the interface (as it is not now). 



In the following comment
 /*
- * Sets (dest_x, dest_y) to the new map position or a real map
- * position near. In most cases it is better to use MAPSTEP instead of
- * SAFE_MAPSTEP.
+ * Sets (dest_x, dest_y) to the new map position or a nearby real


position.


+ *
+ * In almost all cases it is better to use MAPSTEP and deal with the


unreal
+ * coordinate case explicitly, instead of SAFE_MAPSTEP and the void_tile-ish 
+ * problems that produce surprises later.
  */
although I agree with you in principle ("in almost all cases it is better to use MAPSTEP"), the "void_tile-ish problems that produce suprises later" part is completely misplaced. 

Anyone who sees SAFE_MAPSTEP() and the original description is clearly
going to make the wrong assumptions about the "safety" of using this.


True enough.
Secondly, this comment is aimed at convincing the casual user to not use this macro, but it will only be understood by someone who knows the history of the internals of map.c. 



Any casual user that doen't understand the use of this shouldn't use it.
Anything to convey this message in clear terms is IMHO only beneficial :-).
But this does not convey it; it just means nothing to most people. A more beneficial comment would be to say "you should not use this function unless you _really_ know what you're doing". 



Ack. void_tile is gone and shouldn't been mentioned anymore.



It is a pity the philosophy hasn't gone. Until it has, remembering past
history can only help to avoid repeating past mistakes. I disagree quite
strongly with your opinion and expressed desire to suppress others here :-).
There seems to be an awful lot of redefining everything from variable names to concepts by those that seek to reimplement discredited notions or just put their stamp on things - not a good sign and too much is 
being lost in the process. More history needs to be retained :-).
It should not be mentioned in the interface. It should be mentioned in the internals. Writers of map.c should know it, but those who use the interface are best off not being confused by it.
This, unfortunately, is one of the problems with macro-izing these functions. 



This comment
  +/* This is a temporary measure to track down
  +   non-normal cordinate use */
   #define CHECK_MAP_POS(x,y) assert(is_normal_map_pos((x),(y)))
seems quite wrong to me. Although the current form of CHECK_MAP_POS may be quite temporary, it's neither feasible nor desirable IMO to go back to the old system of using normalize_map_pos everywhere and returning void_tile if we ever end up with an unreal tile. One possible change would be something like 



The old system used normalize_map_pos() and dealt with the unreal problem.
The old-old system and the new system would return pseudo-real values and just continue merrily along their way.
Not quite. map_get_tile() is the crux of this issue (since it is the source of void_tile), so let's look at that. For convenience, I've broken down the MAP_TILE and map_inx macros. 

current code (introduced Oct 11th):
  struct tile *map_get_tile(int x, int y)
  {
    CHECK_MAP_POS(x, y);
    return map.tiles + x + y * map.xsize;
  }

old code (introduced Sep 25th):
  struct tile *map_get_tile(int x, int y)
  {
    int is_real = normalize_map_pos(&x, &y);

    assert(is_real);

    return map.tile + x + y * map.xsize;
  }

old-old code:
  struct tile *map_get_tile(int x, int y)
  {
    if (!normalize_map_pos(&x, &y))
      return &void_tile;
    else
      return map.tiles + x + y * map.ysize;
  }
As you can see, the purpose of the void_tile hack was to allow unreal coordinates to be dealt with. Without void_tile, all we can really do in the unreal case is abort or continue on and hope.
Given this, the CHECK_MAP_POS change is beneficial since it standardizes this procedure. Currently there is no call to normalize_map_pos, but by changing the macro we can control this behavior game-wide. If we desire even more safety, we can encourage the chances of "hope" succeeding by adding a nearest_real_pos call if normalize_map_pos fails. The assertion would work the same in either case. 



#ifdef DEBUG
# define CHECK_MAP_POS(x,y) assert(is_normal_map_pos((x), (y)))
#else
# define CHECK_MAP_POS(x,y)               \
#   if (!normalize_map_pos(&(x), &(y))) { \
#     freelog(...);                       \
#     abort();                            \
#   }
#endif



The use of normalize_map_pos() everywhere was decried for performance
reasons, it *is* the robust solution. By adding back these checks you are
forfeiting the performance benefit. In the discussion of the time it was argued that eventually there would be a benefit, so this overhead vs the proper solution was only temporary. If you now think this needs to be permanent, then the solution is to return to using normalize_map_pos() which you have done here. But in the original code it was often used properly in context which this has now lost in favour of fairly widespread generic constructs like map_inx(). 

I do agree that something like this is a reasonable default for dealing
with unreal conditions (note not unnormal ones) in a purely generic context, but once at the head of a routine, rather than on every map 
or tile lookup was a much better solution.


CHECK_MAP_POS should be called in cases where:
  - There is no way for us to deal with unreal coordinates and
  - We expect the input coordinates to be normal.
In this context, it can handle everything with the desired combination of speed, safety, and debugging ability. The current CHECK_MAP_POS goes for all speed when we're NDEBUG and all debugging when we're DEBUG.
but calling CHECK_MAP_POS "temporary" just seems like a misplaced attempt to make it so :-).
That was your statement and therefore a condition on the decision that was made at the time - just recording the history to make sure it doesn't 
manage to lose itself :-).
It is the implementation which is temporary. I'm sorry if that wasn't clear.
I think your attack on CHECK_MAP_POS is generally unfounded. In most places it is used, it is replacing previous code that was identical or nearly so; in others it may be adding desired checks where previous ones were missing, and it's even possible it's being misused in some places. But there is no fundamental problem with the macro itself. 



I think the discussion items are things that do need to be resolved as
they underlie a number of larger tasks that are bogging down :-).


Indeed.  There's a lot of miscommunication on these issues.


[Patch 3: mapclean_02]



a)  map_city_radius_iterate -> city_map_checked_iterate
   This fixes unnormalized access to map_get_terrain which can no longer
   deal with real but non-normal coordinates.
As a side note, I don't think either of these names are particularly good. map_city? checked? huh? I have to look the city macros up every time I use them. 



This should be largely out_of_scope. The names are in CVS for the most
part.


This is out of scope; it's just an aside.



c)  is_normal_map_pos() -> normalize_map_pos()
   Returned values must be normalized, so just do it and return.



   do {
     *x = myrand(map.xsize);
     *y = myrand(map.ysize);
-  } while (!is_normal_map_pos(*x, *y));
+  } while (!normalize_map_pos(x, y));
 }
This change is incorrect. Consider an iso-rectangular cylinder. In this case, all positions will be real, and so will be accepted by normalize_map_pos. However, it is only normal positions that we are interested in. Changing the code like this will result in some positions being picked more often than others. 



Consider the iso-rectangular cylinder, it has a map.xsize x map.ysize
native coordinate set where all values are treated exactly the same
as the current standard rectangular cylinder.
But what you are missing is the native coordinate conversion because that isn't in yet. Look at the prototype/corecleanup version. 
    do {
      *x = myrand(map.xsize);
      *y = myrand(map.ysize);
      native_to_map_pos()&x,&y);
    } while (!normalize_map_pos(x, y));
which typically takes one pass per coordinate requested. Native coordinates are by definition normal as they are currently in the standard map. By normal one means no real coordinate appears twice. 

For now, there is only standard == native coordinates, so this is correct.

This solution takes the tack of properly defining the random selection
to use the native normal space. Other solutions do things in cruder
more expeensive ways but hopefully not in Freeciv.


Again, no.
The current working definition is that everything is in flat coordinates; no concept of "native" coordinates has yet been introduced. In flat coordinates, is_normal_map_pos is definitely what we want. We don't want to find any non-normal position, since that will imbalance the probability of finding each position.
In native coordinates each position will be guaranteed to be normal. So in this case the normalize_map_pos is unnecessary; we might as well stick with is_normal or leave the check out entirely.
Given that neither of these systems have been implemented, we should stick by the method that will work equally well for either of them; especially since that's the way the function was originally designed and written.
As another side note, "native" coordinates are not guaranteed to be flat or even to exist, so relying on something like this is unsafe in the general case. It is better to write something of this form: 
  int index = myrand(map_index_size());
  index_to_map_pos(index, *x, *y);
even this assumes that a continuous index_to_map_pos is possible, which may not be feasible (though it's certainl _possible_) under all topologies, so it might be best to do 
  int index;
  do {
    index = myrand(map_index_size());
  } while (!index_to_map_pos(index, *x, *y));
I had thought we were tentatively agreed on this index_to_map_pos system...
If you want to rewrite the function entirely to take topology into account, that is a valid change. (I'd wait to get a second topology before doing this, however :-).) Doing it like this is just asking for trouble.
No, it is perfectly safe. Since in fact is_normal_map_pos() just calls normalize_map_pos() and throws away the values, using an extra function call in the process, this change is actually more efficient so it is worth doing this part now. 

It is certainly unsafe in my implementation of general-topologies.



d)  is_real_tile() is a test, not a full normalization, make it so.


This spreads out knowledge about the topology. Provide numbers which
show how much faster it gets this way.



Accept it. Numbers aren't needed for the obvious.

Clearly, the test will be more efficient than the function call to
the full algorithm that includes the test.
This restores the original CVS behaviour that the corecleanups did not change. 

Calling a full normalization function to return is_real, vs separating
the is_real test condition of normalize_map_pos() into a separate element that can be called on its own or as part of the full process 
is just a better partitionning of the interface.
Or to follow your logic get rid of is_normal_map_pos() for the same reason, that it spreads the topology knowledge too far. Plus the fact that it only tests for the soft condition that doesn't matter, whereas is_real_map_pos() 
at leasts tests for the correct hard condition of unrealness.


Raimar has gotten rid of is_normal_map_pos, and for the same reason :-).
Efficiency concerns aside, it is possible to implement all of normalize_map_pos, is_real_tile, and is_normal_map_pos in terms of your value-returning nearest_real_pos function. But all of this is unnecessary IMO. We might as well just implement the functions to achieve the fastest speed.
Unfortunately, the fastest speed means macro-izing all of them, at which point it really doesn't matter which is implemented in terms of the other. 



e)  normalize_map_pos() should test for realness before it undertakes
   any transformation. This fixes any potential bugs from side effects.


Heh.  The bug is not here, but you should fix it anyway :-).



Touche ...

But actually, there are GUI spots that had comments about not adjusting
the values for unreal tile positions. The fix to map_adjust_*() at these
points is normalize_map_pos() and this preserves that characteristic so
workarounds aren't needed.
The GUI spots that had comments about not adjusting the values for unreal tile positions were all done that way because they assumed the position would be real. These have all been fixed to 
  is_real = normalize_map_pos(dest_x, dest_y);
  assert(is_real);
However, I believe there are some older GUI uses that do assume normalize_map_pos leaves the position untouched. Most or all of these are probably incorrect, although it would take a while to track them down. 



Note one alternative would be to get rid of nearest_real_pos
entirely and give this functionality to normalize_map_pos.


Ack. However I have no idea how hard it is to convert the remaining 3
(three) cases.



void_tile-ism is so hard to stamp out ... it truly amazes me, this
tenacity in its adherents.

The solution is of course to stomp out this function period, just like
void_tile, map_adjust_y(), SAFE_MAPSTEP and the rest of the mutations.
Hey, I wasn't seriously suggesting it - in fact it would be ludicrous to do this - I'm just saying it's _possible_.
I do think SAFE_MAPSTEP is wrongly implemented. It needn't use nearest_real_pos but should instead do 
  #define SAFE_MAPSTEP(x, y)
  {
    int _x = x, _y = y;
    if (!normalize_map_pos(&x, &y)) {
      x = _x;
      y = _y;
    }
  }
I also think it's dangerous to have this as a globally defined macro, so I agree with Ross on this point.
Similarly, the use of nearest_real_pos in map_get_player_tile is wrong (this should just be a CHECK_MAP_POS which is included in map_inx), and I'm pretty sure the one in auto_settler_do_goto is as well (although I'm not familiar with that code).
In truth, the only necessary use of nearest_real_pos should be in canvas_pos_to_map_pos. Although the whole GUI system is a mess, what it breaks down to is that when a user clicks on an unreal tile to center the canvas, it is instead centered on the nearest real tile. If we are willing to give up this functionality, the function can be removed entirely. 



g)  RANGE_CHECK macros are added and used to improve performance by
   reducing execution in most cases where applicable by a factor of two.
I find the whole RANGE_CHECK_0 thing pretty unreadable and worthless. Even if such a thing were included in CVS, I'd still write 
  if (0 <= a && a < b)
instead of
  if (RANGE_CHECK_0(b, a))
Although I haven't checked it, I find it hard to believe that any worthwhile compiler couldn't optimize this.
All the ones I've looked at don't. 
The double test is an endemic operation in all the code, normalization
for instance, it was worth a couple percent overall :-).
Actually, I think my biggest problem isn't with the macros themselves but with them being placed into map.h. This is not map code, and should be put into shared.h or some other good place.
That aside, I still do not think the insignificant increase in speed (one operation!) is in any way worth the loss in code legibility. Macro-izing or even inlining functions will attain many orders of magnitude larger of a savings. Spending time on this when there are so many more worthwhile things to improve seems like misplaced energy...but I will not oppose the change on this point. 



h)  Wrap and clipping macros are added to standardize code use. Overtime
   all map code that does local flavours should be updated to use these.
map_wrap and map_clip should also go into shared.h as WRAP and (already there) CLIP. You will then have 

shared.h:
  #define CLIP(lower, this, upper) /* ... */
  #define WRAP(lower, this, upper) /* ... */

map.h:
  #define map_wrap_x(x) WRAP(0, x, map.xsize)
  #define map_wrap_y(y) WRAP(0, y, map.ysize)

  #define map_clip_x(x) CLIP(0, x, map.xsize)
  #define map_clip_y(y) CLIP(0, y, map.ysize)

  #define map_adjust_x(x) map_wrap_x(x)
  #define map_adjust_y(y) map_clip_y(y)
This is still a misplaced change IMO since it is easier IMO to simply remove map_adjust_[xy], but it's not entirely unworthwhile. Certainly having WRAP defined in shared.h will be convenient (general-topologies does this). 



i)  map_adjust_x() and map_adjust_y() are redefined to be explicit
   wrap and clip operations for the standard cylindrical map, as the
   first instance of standardization :-).
This is very bad IMO. Standardization requires that map_adjust_x/map_adjust_y be removed entirely. This change just adds a lot of importance to their definitions - you take 7 lines of #definitions that are currently only used in a very few places (hopefully soon to be removed), and change them into 30 lines of #definitions within #definitions with #definitions, plus extensive comments explaining how they should be used (including saying that they're about to be removed, fortunately :-). 


Ack. map_adjust_[xy] should die/definition moved out of
common/map.h. Move it (in its current form) to client/mapview_common.c
I will be happy.



I think we all agree these need to go (along with SAFE_MAPSTEP, ... :-).
map_adjust_*() goes as soon as the code doesn't need it, which may be blocked on Andreas right now. 

Actually it's blocked on us :-).
The only remaining uses of map_adjust_x are in gui-mui, which shouldn't be an immediate concern as it doesn't compile anyway (I provided a patch to attempt to fix it, but the MUI maintainer is working through some other issues first), and in mapview_common.
We must introduce find_representative_map_pos or unnormalize_map_pos to move forward here. So I will reopen this discussion shortly.
It is gone in the corecleanups, so this is just one of those intermediate inefficiencies because everyone want things done in small steps :-).
It is also gone in general-topologies, but has been replaced by something different :-). 



j)  map_to_index() is defined as the start of the map_inx() removal.
   When the corecleanup and topology fixes are all applied, this will
   be completely gone. index_to_map_pos() as the reverse operation is
   defined in the standard manner, i.e. returns updated coordinates
   and a status return indicating success or failure.
I definitely like the name change (although I would call it map_pos_to_index). I'm not sure what you mean by map_inx removal, but I don't think I'm going to like it :-). 



The leading "pos" was I believe debunked by others, (Tony, maybe?).

map_inx was long ago to be renamed, but it will live on and continue to
multiply as map_to_index().
I have no problem with the removal of the leading pos as long as there's a trailing one. So it should be either map_pos_to_index or map_to_index_pos. The former seems better to me. 



All these "coordinates" are basically interconvertible with the internal
standard game coordinates. There are thus two functions to write for any
new one, <blort>_to_map_pos() and map_to_<blort_pos>.


  <blort>_to_map_pos and map_to_<blort>_pos
or
  <blort>_to_map_pos and map_pos_to_<blort>
Given the number of misnomers we already have, I'm not too worried about a few more, so I don't think this is worth much argument. 



k)  Macro forms for is_real_tile() and family are defined for the
   cylindrical map. These will be upgraded to full topological forms
   when further capabilities (i.e. topological state) are added.
This is something Gaute has wanted to do for a while, and as you've found the speedup is very significant. It makes the code pretty ugly/unreadable, but it's tough to argue with the results. 



Luckily, it is broken down into small steps. Each step is not that
difficult to work out. Treat the steps as atomic operations once you
have understood them. The entire process is thus quite digestible and
very true to the conceptual explanation.
My problem isn't with the implementation (macros are certainly ugly, but that's acceptable), but that there's no separation between interface and implementation. For short macros this is not an issue, but for complex ones like the topology ones will eventually be it is. It makes it much harder for a freeciv programmer to use the macros without being bogged down in them. 



l)  The generic border tile macro is added and used in the current case.


Ugh.
Summary: I don't like most of these changes at all. So we need another opinion (aside from the is_normal->normalize change which must be fixed). 



Reread the above counter. *must* is such a strong word for something
that is technically very straightforward and provably correct, more
efficient, etc.


s/correct/incorrect/ :-)

And the efficieny issue is completely moot IMO.



We need to get past the religious debate sometime though (sigh).


Indeed.



Times were ...
 9:05  CVS-Jan04
 7:03  patched
Very impressive. But a greater speedup can be achieved just by changing is_real_tile, is_normal_map_pos, normalize_map_pos, AND map_get_tile into macros - without the need for RANGE_CHECK or any of the other implementation hacks you use.
In my test, I get the following times for an autogame (compiled with DEBUG and -O2 and only run once, so maybe not a very good benchmark...): 

CVS: 3m39s = 219 sec
mapclean patch: 3m6s = 186s ~ 15% speedup
simple macro patch: 2m36s = 156s ~ 28% speedup
In my books, 2 in 7 is about 28%. I'm not sure I understand the "greater" comment. map_get_tile() is left out here. It is one of the key functions 
identified in the corecleanup section of map.h that used to be macro-ized.
In your single benchmark, you achieved a 2/9 = 22% decrease in runtime for your patch.
In my single benchmark, I achieved a 15% decrease in runtime for your patch and a 28% decrease for mine. 

Hence, "greater".
I'm not particularly in favor of such changes, but I do think if we're going to make such changes they should be done in the correct order: i.e. first macro-ize the most-called functions (normalize_map_pos and map_get_tile), then work from there. Having a more efficient implementation is quite insignificant compared to the overhead of a function call, although we might as well improve that while we're changing the code. 



BTW: the corecleanup runs at about half this speed on the same topology.
     But it has iterator loop and other things that are going to be much
more controversial when they come out, even without the first exposure backlash, i.e. 6 month settling time.
Making commonly-used iterator loops (mostly adjc_dir_iterate) faster can also have a significant increase in speed. Case in point: adding the is_border_pos check to adjc_dir_iterate decreased overall runtime by about 4% (mostly since it decreased normalize_map_pos calls by about 90%). 


jason
[Prev in Thread] Current Thread [Next in Thread]