Ask "Is this inside a building"

[qugebert]

Related Issue: #5175

I had no idea for a good quest icon, can someone else take care on this?

This quest should replace "AddIsDefibrillatorInside", but i noticed that the defibrillator quest is used in CONTRIBUTING_A_NEW_QUEST.md for explanation, so we should either adapt this documentation or keep the AddIsDefibrillatorIndoor file

[westnordost]

I had no idea for a good quest icon, can someone else take care on this?

Sure, I will

[westnordost]

Yeah, definitely delete AddIsDefibrillatorIndoor, please. We can fix the documentation afterwards.

Also, since you replace/rename the quest, you should add an alias to typeAliases, i.e.
"AddIsDefibrillatorIndoor" to AddIsAmenityIndoor...

[westnordost]

Another review, I found one bug and one "lint" error

[westnordost]

isApplicableTo is called also offline for any element that is updated due to the user changing it.

So for example when you solve the building quest, the app subsequently checks with this function whether for the new version of the building any new quests apply. In this case, the building levels quest will return true here, so a new quest is created.

Now, this means for your implementation here that if any e.g. amenity=atm is updated through any other quest (e.g. checking whether it is still there), the method here will always return true, ie.e. the AddIsAmenityIndoor quest is created. We do not want that.
In fact, as long as we do not have any information about the surrounding buildings, we cannot answer whether it applicable or not. Or well, if the given element does not match the nodesFilter or does not have any name, then we can say that it is not applicable, but otherwise, we can't tell. In this case, we want to return null.

Also have a look at for example the housenumber quest as it also depends on the surrounding building geometry.

[qugebert]

Ok, i'm not sure if i understand this right.
Would it just be
override fun isApplicableTo(element: Element) { if (!nodesFilter.matches(element) || !hasAnyName(element.tags)) false else null }

or should i think of a more complex filtering?

[westnordost]

For this function, yes. However, you use this function also in getApplicableElements, so there you should probably inline nodesFilter.matches(element) && hasAnyName(element.tags)

[westnordost]

A further performance improvement would be possible here.

Currently for every node, you check if it is contained within any building that is near one of these nodes.

You could also reduce the nodes beforehand that are not near any building. (add all nodes near buildings to a set and then only iterate through these instead of all nodes)

However, probably not worth it because I guess there are not that many "small amenity nodes" looking at the filter you wrote, so not required for this PR to be merged. Just wanted to note this.

[westnordost]

Something went wrong with your merge, look at the changes on github now.

[qugebert]

Yes,but i don't really know, what went wrong.
I wanted to adapt the functions hasAnyName etc. so that it works with the changes from yesterday (PR #5273), but of course I needed the current version for testing, so I to merged into my branch, but after that the build didn't work neither in AndroidStudio nor on Github actions (a java.lang.UnsupportedClassVersionError where i don't believe it can come from these changes), so I reversed it and now everything is completely botched.
I'll create a new/clean branch and PR with references to this one

[qugebert]

Ok, sorry for the botched merge, i now created a new pull request
#5278