[mkgmap-dev] refactoring

Mon Apr 21 13:10:36 BST 2014

Hi Gerd,

I have had a quick look on it.

> Hi all,
>
> please test / review the changes in the branch.
>
> Users: You should find no differences in map output between trunk r3215
> and branch r3214,
> but maybe the log is different.
>
> Programmers: please have a closer look at the source.
>
> The intended changes :
> 1) Create new class AccessTagsAndBits which contains code
> to map tags which are relevant for routing to bit masks.
>
> I prefer to have  to bit 1 for "access allowed" to avoid
> double negations like in "if (!noAccess[i]) ..."
> I hope that is okay for you as well.
+1

>
> All routines which read data are now using the same
> bit constants regarding CAR,FOOT,BICYCLE,TRUCK, etc.
> to fill a bit mask field.
> The write routines convert the internal representatio to
> that used in the img format.
>
> @WanMil:
> Special case: Code in  StyledConverter near roadLog.isInfoEnabled()
> formats the content of the TabAAcesss field in RoadDef which
> contains the img format. I think this is okay because the intention
> of the log is to show exactly the content of this field.

 From my point of view it doesn't matter to have the exact bit coding 
here. The intention of the output was to get an information about the 
access restrictions. For ease of use I decided to use the bit 
representation of the map but if another output is easier to code or to 
read please feel free to change.

>
> 2) Create class ConvertedWay which combines an
> OSM Way instance with a reference to a GType instance.
> Instead of copying and modifying fields (roadClass, roadSpeed)
> in GType instances this is now done in ConvertedWay.
> Many tags which are relevant for routing are evaluated once
> when the ConvertedWay instance is created.
> The results are kept in bit masks, this allows a quick
> compare.
> Also, RoadMerger uses this class instead of creating new
> Road instances.

Ok.
One question regarding line 550:
if (road1.getRouteFlags() != road2.getRouteFlags())
If the route flags are different are the ways always not mergable?
In such a case the code lines after are important for debugging only. 
Some CPU cycles can be saved by adding if (logger.isDebugEnabled()).
I also suggest to move the low cost checks to the beginning of 
isWayMergable(). Before refactoring all checks were working on tags and 
therefore had the same cost.

>
> @WanMil: The new code produces a different log on debug
> level because some comparisons are done in different order.
> I've changed the code to compare the bit mask fields instead of the
> tags, but the code still lists (most of) the compared tags
> to be able to produce debug info.
> Let me know if that is okay for you. Another solution could be to
> implement the debugging in ConvertedWay e.g. something
> like
> public String compareAccess(ConvertedWay other)
> could return a String like
> "mkgmap:truck is different 'no' <> 'null' "

I don't mind about the output as long as it contains the required 
information in a readable format :-)

>
> 3)
> With r3214 I've renamed a few methods. Please check if
> you find better names:
> http://www.mkgmap.org.uk/websvn/revision.php?repname=mkgmap&rev=3214

some ideas:
Element.getEntrySetIterator() => Element.getTagIterator()

Some other comments:

RestrictionRelation.setExceptMask should be updated to the new access 
rules. But maybe this change should be applied not in the refactoring 
branch without any change in the output.
Example:
delivery is not an access tag and should be removed
psv should set bus and taxi (if they are not set too)
motorcycle should be ignored
vehicle should be considered.
...

HighwayHooks.usedTags should not be static.
It should be possible to compile one tile with make-opposite-cycleways 
and another without. So the usedTags field should be different.

WanMil