Conditional Access Restrictions

[boldtrn]

This PR adds the feature of checking conditional Access Restrictions. Currently only restrictions that last longer than one day are supported. The WayAcceptor currently uses the current day as comparison value.

Currently the following date formats are supported:

• 2014 Aug 10-2014 Aug 14
• Aug 10-Aug 14
• Jul-Aug
• Aug
• Sa-Su
• Su
• All of the above formats support the ranges in revers, e.g. Nov-Jan (matches Nov, Dec, Jan)

Things that are not supported yet:

• Multiple conditions like:
  • (2014 Aug 10-2014 Aug 14; Sa-Su)
  • no @ (2014 Aug 10-2014 Aug 14); no @ (Su)
• Things like: easter, PH, SH, summer, winter, ...

(Allowance of already restricted ways, like yes @ (Su) is now supported)

Partially fixes #374

[karussell]

Thanks!

The unsupported things are no problem. One thing which I do not understand is 'Allowance of already restricted ways': Do you mean that access=no and access:conditional=yes @ (date time) is not yet possible?

[boldtrn]

Do you mean that access=no and access:conditional=yes @ (date time) is not yet possible?

Yes. This is not yet implemented. During the acceptWay method we have to add an allowance check. Currently, this seems like a conceptual issue to me. We are using a accept fast or fail fast strategy in the acceptWay method.

Once we returned 0, we cannot return the acceptBit. I didn't want to change that without discussing it.

[karussell]

In acceptWay we have a section where one such condition could be included IMO:

   String firstValue = way.getFirstPriorityTag(restrictions);
   if (!firstValue.isEmpty())
   {
      if (restrictedValues.contains(firstValue) /*&& HERE*/)                return 0;
      if (intendedValues.contains(firstValue))                return acceptBit;
   }

The foot&bike need simpler handling:

if (way.hasTag(restrictions, restrictedValues) /*&& HERE*/) return 0;

Later on we can put this all into one method. For now unit tests should be sufficient e.g. for car&bike.

[boldtrn]

@karussell I added the allowance in all encoders (motorcycle, foot, bicycle, and car) and wrote tests accordingly.

[karussell]

Do you think this is fixed with the name change? Or do we have to change something on the conceptual level?

Hmmh, not sure as we still would get if !rejector.accept(way) then return 0 (or maybe I'm wrong here with mind-based renaming ;))

What about one class (like ConditionalTags) with two more direct methods like reject and accept?

[boldtrn]

I did some refactoring. I guess it should readable now :).

[karussell]

Nice - thanks! Just a tiny thing to discuss (sorry for the nit picking): are there shorter and still nice method names? http://stackoverflow.com/questions/2230871/when-is-a-java-method-name-too-long

I can't find better ones, just would move the is to the beginning: isRestrictedWayConditional(ly)Permissive and isPermissiveWayConditional(ly)Restricted

or could we remove the conditional(ly) as the class and therefor the variable has this in the name?

[boldtrn]

You are right. The is should be in front.

I try to name methods in a way that no additional documentation is necessary (whenever possible ;)). And I think the conditional(ly) is important in this case. Without it would be: isPermissiveWayRestricted, this sounds a bit weird, I guess. But this is probably something that is discussable, so I can remove it if you want to?

[karussell]

I try to name methods in a way that no additional documentation is necessary (whenever possible ;))

Nice, that is exactly like it should be :) !

this sounds a bit weird, I guess

Yes, you are right. Let us keep this but rename to permissive instead permissed :)

[boldtrn]

Let us keep this but rename to permissive instead permissed :)

Ok I changed permissed to permitted. I think this should be the better naming for this?

[karussell]

Okay, this should be better :)

Also isRestrictedWayConditionallyPermissed need this change and in isPermittedWayConditionallyRestriced a 't' is missing in restricted.

To make it mergable we also need to avoid throwing exception here:
if (conditionalTag.contains(";")) and just print a warning instead (?)

In the class ParsedCalendar the methods should use the Java style like isYearless instead of yearless. I know the latter has advantages and we had this style in the early days, but the Java standard makes it easier for others when using autocomplete etc

In the new files the license header is missing.

// TODO: Ferries have conditionals, like opening hours or are closed during some time in the year

What would be needed to implement this?

[boldtrn]

What would be needed to implement this?

Mhm, most ferries I saw I have quite complex opening hours (or 24/7). We are not yet able to parse these opening hours. See this query http://overpass-turbo.eu/s/dvv. So currently no easy solution available. Some ferries are seasonally closed, but these opening hours are setup quite complex.

Firs we have to consider the opening_hours tag (Some things change here, so we'd need a OpeningHoursParser).
Then we have to support combined opening_hours like: opening_hours=Mo-Su 8:00-20:00; Nov-Mar off.
Then we have to parse the opening hours of (every?) way.

[karussell]

Ok, then we postpone this for ferries, of course. Also related to #439

Then we have to parse the opening hours of (every?) way.

Those with highway tags and also probably of every nodes with barriers

[karussell]

I made a few minor modifications and while trying this on real OSM data I stumbled over:

• the log info is too much for the case conditionalTag.contains(";") I vote for not logging this at all and adding this as todo for Recognize conditional restrictions #374
• May I add the format dd.MM. and remove the failing test? Or why did you add an explicit test for this that this should fail? The same for DateRangeParser.parseDateRange("Sat");

[boldtrn]

the log info is too much for the case conditionalTag.contains(";") I vote for not logging this at all and adding this as todo for #374

I think this is a good idea. Also I think this could be doable with some clever transformations. I just found the OpeningHoursParser of OSMAnd. The do not have a good strategy for the ; as well (as far as I can tell from skimming the code - see the FIXME ;)). Also there is this Issue. Maybe we should investigate their code in more detail?

May I add the format dd.MM. and remove the failing test? Or why did you add an explicit test for this that this should fail? The same for DateRangeParser.parseDateRange("Sat");

I added the failing tests to show the limitations of the parsing and I wrote them for me while developing to check if everything behaves as expected. I tested against the specification, which is: Sat is not the correct form for Saturday, according to OSM. I think there was a reason why I did not add the dd.MM format. Maybe only because it is not correct according to the documentation, but I am not sure. Please go ahead ;).

[karussell]

Maybe we should investigate their code in more detail?

Please not. It is GPL :(

Maybe only because it is not correct according to the documentation, but I am not sure. Please go ahead ;).

Okay, then will keep rejecting Sat but accepting dd.MM(.)

[karussell]

If 'Aug 10-Feb' is used then currently all days in February do not lay inside this interval. Is this according to the wiki or do we need to fix this (potentially in a separate issue)

[karussell]

I've merged this now

[karussell]

Created a new issue for this #645