Previously in GraphHopper it was possible to extend the existing instructions a bit with Annotations.
As far as I can see, that is not possible anymore, only if your encoder is an instance of GenericBicycleEncoder and then only for GetOffBike. That seems quite unflexible.
Would it be possible to register some sort of Instruction logic, that gets executed on top of the existing logic? Maybe something similar we do for GetOffBike or maybe like a more generic annotation?
One use case where this could be interesting is turn-lane support. So if an encoder registers for turn-lane support, the turn-lane feature gets executed on top of the regular instruction logic.
One use case where this could be interesting is turn-lane support.
This should not be an extension and directly included. Customization in the code comes with overhead and we should avoid it if possible (of course I do not know if it is possible here but most âinstruction annotationâ use cases really should be a core part of GH instead of an external extension mechanism)
BTW: Iâm not overly happy with the current if-else code inside this class, so there is of course room for improvement. But removing it from the FlagEncoder was also necessary as this class will soon likely disappear.
Yes, the lane support was one idea that came to my mind, but I think there are other cases as well.
The reason why I created this thread here is that I saw that GetOffBike requires a BikeCommonFlagEncoder, which seems very unflexible. So I was thinking it would be nice if we could make this a bit more flexible.
For example one thing I am currently struggling with is that many encoded values use enums. Enums are nice, but they are not extendable. So if I need to change something, I have to copy the enums, copy the parsers, etc. and change the required part. But then itâs also not compatible with other places in the code, because itâs now using my own enums and not the GH standard classes. One place were this could become an issue is in the instructions and changing the instruction generation code is not that easy right now without forking GraphHopper or copying bigger amounts of code .
Yes I full agree and the instructions are performance critical. So I was thinking if there could be an option that doesnât add a serious overhead if itâs unconfigured to not hurt the default case.
Yes, the instruction code is a bit messy. On the other hand, there is a huge amount of edge cases that needs to be considered, and we are probably not even catching some of the rarer cases .
I still canât imagine a world without flag encoders
GetOffBike can be used with any FlagEncoder. But yes, we could make it more flexible.
So if I need to change something, I have to copy the enums, copy the parsers, etc. and change the required part.
I think, this is a different issue. And of course enums are not extendable per definition so if more values are required they would be included in the core. Surely we can later introduce generic StringEncodedValue
I still canât imagine a world without flag encoders
Theyâll be replaced with a certain encoded value collection, see PtEncodedValues.
Yes, but I think it is related. This is part of the journey I had to opening this thread.
Yes, if they make sense for the general case, I fully agree. If I want to store something custom, thatâs quite hard right now.
Would you be interested in something like this? So that we could for example store Country as StringEncodedValue? This would be relatively equal to EnumEncodedValue, but use String instead. I think Country would be a good example where StringEncodedValue could make sense.
Awesome, I will keep that in mind, if I have a good idea.
Yes I have seen that, but I am still wondering how this will look like for more complex FlagEncoders. Something like Bike2Weight, but yes, that is a different issue .
But not in the instructions or am I missing something?
You mean the instruction feature is for bike FlagEncoder only? Yes, it is not yet intended for something else.
Do you have an example where enums are too limiting?
Would you be interested in something like this?
It was already possible in the initial PR for encoded values. But I removed it as it is important to start with limited possibilities and see what we really need. And string is certainly more flexible, but it is kind of messy inside Java as youâll still have to create String keys and simulate enums.
That said the encoded value mechanism is flexible enough that you should be able to introduce StringEncodedValues although the core does not yet support it (you could even pick the implementation from my PR).
The main questions are:
should we be able to store unknown strings into the database? (If no => enum)
should they be used in the Weighting or in the path details?
In the Weighting the StringEncodedValues are probably better replaced with ints or enums and for path details one could already use something like the StringIndex.
So that we could for example store Country as StringEncodedValue?
Country is an enum
A more generic area ID is probably more a string, but again this could be better implemented with an int ID (not sure) or some combination of StringIndex.
So the use case is not yet 100% clear to me and so it is harder to choose the correct implementation.
I was wondering why? IMHO, if an encoder registers the GetOffBike encoded value, it should be also picked up in the instruction generation. Or is there an issue that if you register bike and car for example it will also pick up the GetOffBike for car?
For example if I create a new bicycle encoder but donât want to reuse the BikeCommonFlagEncoder, I canât use GetOffBike.
I think Country is a great example, where an Enum is too limiting.
The other examples I have are quite custom, where I want to add something that makes no sense in the general case.
I think GraphHopperâs Enums are very well crafted and thought through. I also agree that they are a very elegant way to solve this. In my own implementation I would prefer Enums over Strings every time. I think it makes no sense to store unknown strings, these should be defined beforehand.
My issue is that if I use GraphHopper as a library, I can easily create my own Enums with all the values I like and parse them. But then I canât use them in some of the deeper levels of GraphHopper, like the instructions. And I also cannot change the existing Enums to something else so that I could abuse the existing Instructions code - thatâs why I was thinking about a way to make the instruction generation extendable . I think the instructions are one piece of code where custom EncodedValues could be important.
So I think my issue could be resolved by adding a setter to GraphHopper like setInstructionsFromEdges that I can use to pass down my custom instruction generation. My preferred way would be to pass some hooks to the instruction generation, so that itâs possible to slightly alter the creation of instructions, like when an instruction is created and the content of the instruction. This could be done by extending the InstructionsFromEdges class and to allow for example to overwrite the getTurn method and maybe a way to change the name and annotation of the instruction.
But I also understand that this is a bit of a special case . Probably most people wouldnât want to touch the instruction generation .
So I think my issue could be resolved by adding a setter to GraphHopper like 'setInstructionsFromEdges`
Hmmh, would it be sufficient for you when we provide this as a new constructor to InstructionsFromEdges? I would avoid adding another factory at GraphHopper class level.
I think Country is a great example, where an Enum is too limiting
This would be at least a bit more flexible, but not hurt any existing use cases?
The trouble is, InstructionsFromEdges is used in the PathMerger, which again is used in the RoutingTemplate, which again is used in GraphHopper#calcPaths. Now we do have the option to overwrite the RoutingTemplate in GraphHopper#createRoutingTemplate, which is nice, but then I need to overwrite the RoutingTemplate, to overwrite the PathMerger, to overwrite the InstructionsFromEdges. Thatâs the issue, if I could easily change InstructionsFromEdges, that would be relatively easy.
Yes, we can agree on that . The question for me is, if it would make sense to simply add 200+ countries to the Enum, so that everyone can pick the country they need or if we should make it configurable, if I only need 3 countries, I donât need to waste a couple of Bits for the 200 countries that I could potentially store. Making this flexible could also allow to store states instead of countries, for example in the US, I might want to store states instead of countries.
which is nice, but then I need to overwrite the RoutingTemplate, to overwrite the PathMerger, to overwrite the InstructionsFromEdges.
Clearly we have to move work off of the GraphHopper class and avoid this strong coupling.
if I only need 3 countries, I donât need to waste a couple of Bits for the 200 countries that I could potentially store
I see. This kind of scaling is indeed not possible with enums. Still we should have another use case in mind before we introduce StringEncodedValue and also how to use it (have constants in code similar to enums or put them in the StringIndex)
.
