Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ask "Is this inside a building" #5248

Closed
wants to merge 13 commits into from
Closed

Ask "Is this inside a building" #5248

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
93c005f
AddIsAmenityIndoor
qugebert Sep 1, 2023
1848f18
AmenityIndoor: Filter for nodes in buldings
qugebert Sep 13, 2023
0ca2f37
Update AddIsAmenityIndoor.kt
qugebert Sep 13, 2023
b1c613e
typo
westnordost Sep 15, 2023
c035cdf
add empty space
westnordost Sep 15, 2023
a0d0e45
Remove AddIsDefibrillatorIndoor
qugebert Sep 15, 2023
3a5932f
AddIsAmenityIndoor: Performance optimization
qugebert Sep 17, 2023
25128cd
Merge pull request #6 from streetcomplete/master
qugebert Sep 27, 2023
6b31bda
featureDictionary -> getFeature
qugebert Sep 27, 2023
f350d62
fix typo
qugebert Sep 27, 2023
df65c34
Revert "Merge pull request #6 from streetcomplete/master"
qugebert Sep 27, 2023
1681194
Revert "featureDictionary -> getFeature"
qugebert Sep 27, 2023
5f167b9
Merge branch 'AddIsInside' of https://github.com/qugebert/StreetCompl…
qugebert Sep 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions app/src/main/java/de/westnordost/streetcomplete/quests/QuestsModule.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import de.westnordost.streetcomplete.quests.air_conditioning.AddAirConditioning
import de.westnordost.streetcomplete.quests.air_pump.AddAirCompressor
import de.westnordost.streetcomplete.quests.air_pump.AddBicyclePump
import de.westnordost.streetcomplete.quests.amenity_cover.AddAmenityCover
import de.westnordost.streetcomplete.quests.amenity_indoor.AddIsAmenityIndoor
import de.westnordost.streetcomplete.quests.atm_cashin.AddAtmCashIn
import de.westnordost.streetcomplete.quests.atm_operator.AddAtmOperator
import de.westnordost.streetcomplete.quests.baby_changing_table.AddBabyChangingTable
Expand Down Expand Up @@ -61,7 +62,6 @@ import de.westnordost.streetcomplete.quests.crossing_island.AddCrossingIsland
import de.westnordost.streetcomplete.quests.crossing_kerb_height.AddCrossingKerbHeight
import de.westnordost.streetcomplete.quests.crossing_type.AddCrossingType
import de.westnordost.streetcomplete.quests.cycleway.AddCycleway
import de.westnordost.streetcomplete.quests.defibrillator.AddIsDefibrillatorIndoor
import de.westnordost.streetcomplete.quests.diet_type.AddHalal
import de.westnordost.streetcomplete.quests.diet_type.AddKosher
import de.westnordost.streetcomplete.quests.diet_type.AddVegan
Expand Down Expand Up @@ -431,7 +431,7 @@ fun questTypeRegistry(

112 to AddWheelchairAccessPublicTransport(), // need to look out for lifts etc, maybe even enter the station

113 to AddIsDefibrillatorIndoor(), // need to go inside in case it is inside (or gone)
113 to AddIsAmenityIndoor(featureDictionaryFuture), // need to go inside in case it is inside (or gone)

// inside camping sites
114 to AddCampType(),
Expand Down
106 changes: 106 additions & 0 deletions app/src/main/java/de/westnordost/streetcomplete/quests/amenity_indoor/AddIsAmenityIndoor.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
package de.westnordost.streetcomplete.quests.amenity_indoor

import de.westnordost.osmfeatures.FeatureDictionary
import de.westnordost.streetcomplete.R
import de.westnordost.streetcomplete.data.elementfilter.toElementFilterExpression
import de.westnordost.streetcomplete.data.osm.geometry.ElementGeometry
import de.westnordost.streetcomplete.data.osm.geometry.ElementPolygonsGeometry
import de.westnordost.streetcomplete.data.osm.mapdata.Element
import de.westnordost.streetcomplete.data.osm.mapdata.MapDataWithGeometry
import de.westnordost.streetcomplete.data.osm.osmquests.OsmElementQuestType
import de.westnordost.streetcomplete.data.user.achievements.EditTypeAchievement.*
import de.westnordost.streetcomplete.osm.Tags
import de.westnordost.streetcomplete.quests.YesNoQuestForm
import de.westnordost.streetcomplete.util.ktx.toYesNo
import de.westnordost.streetcomplete.util.math.isCompletelyInside
import de.westnordost.streetcomplete.util.math.isInMultipolygon
import java.util.concurrent.FutureTask

class AddIsAmenityIndoor(private val featureDictionaryFuture: FutureTask<FeatureDictionary>) :
OsmElementQuestType<Boolean> {

private val nodesFilter by lazy {
"""
nodes with
(
emergency ~ defibrillator|fire_extinguisher
or amenity ~ atm|telephone|parcel_locker|luggage_locker|locker|clock|post_box|public_bookcase|give_box|ticket_validator|vending_machine
)
and access !~ private|no
and !indoor and !location and !level and !level:ref
""".toElementFilterExpression()
}

/* We only want survey nodes within building outlines. */
private val BuildingFilter by lazy {
qugebert marked this conversation as resolved.
Show resolved Hide resolved
"""
ways, relations with building
westnordost marked this conversation as resolved.
Show resolved Hide resolved
""".toElementFilterExpression()
}

override val changesetComment = "Determine whether various amenitys are inside buildings"
westnordost marked this conversation as resolved.
Show resolved Hide resolved
override val wikiLink = "Key:indoor"

//TODO: Generisches Item
override val icon = R.drawable.ic_quest_defibrillator
override val achievements = listOf(CITIZEN)

override fun getTitle(tags: Map<String, String>) = R.string.quest_is_amenity_inside_title
westnordost marked this conversation as resolved.
Show resolved Hide resolved
override fun getApplicableElements(mapData: MapDataWithGeometry): Iterable<Element> {
val bbox = mapData.boundingBox ?: return listOf()
val nodes = mapData.nodes.filter { isApplicableTo(it) }
val buildings = mapData.filter { BuildingFilter.matches(it) }.toMutableList()

val buildingGeometriesById = buildings.associate {
it.id to mapData.getGeometry(it.type, it.id) as? ElementPolygonsGeometry
}

buildings.removeAll { building ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

val buildingBounds = buildingGeometriesById[building.id]?.getBounds()
(buildingBounds == null || !buildingBounds.isCompletelyInside(bbox))
}

//Reduce all matching nodes to nodes within building outlines
val nodesInBuildings = nodes.filter {
buildings.any { building ->
val buildingGeometry = buildingGeometriesById[building.id]

if (buildingGeometry != null)
it.position.isInMultipolygon(buildingGeometry.polygons)
else
false
}

}

return nodesInBuildings
}

override fun isApplicableTo(element: Element) =
nodesFilter.matches(element) && hasAnyName(element.tags)
Copy link
Member

@westnordost westnordost Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


private fun hasAnyName(tags: Map<String, String>): Boolean =
featureDictionaryFuture.get().byTags(tags).isSuggestion(false).find().isNotEmpty()

override fun getHighlightedElements(element: Element, getMapData: () -> MapDataWithGeometry): Sequence<Element> {
/* put markers for objects that are exactly the same as for which this quest is asking for
e.g. it's a ticket validator? -> display other ticket validators. Etc. */
val feature = featureDictionaryFuture.get()
.byTags(element.tags)
.isSuggestion(false) // not brands
.find()
.firstOrNull() ?: return emptySequence()

return getMapData().filter { it.tags.containsAll(feature.tags) }.asSequence()
}

override fun createForm() = YesNoQuestForm()

override fun applyAnswerTo(answer: Boolean, tags: Tags, geometry: ElementGeometry, timestampEdited: Long) {
tags["indoor"] = answer.toYesNo()
}
}

private fun <X, Y> Map<X, Y>.containsAll(other: Map<X, Y>) = other.all { this[it.key] == it.value }


1 change: 1 addition & 0 deletions app/src/main/res/values/strings.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1052,6 +1052,7 @@ Before uploading your changes, the app checks with a &lt;a href=\"https://www.we
<string name="quest_internet_access_no">No connection</string>

<string name="quest_is_defibrillator_inside_title">Is this defibrillator (AED) inside a building?</string>
westnordost marked this conversation as resolved.
Show resolved Hide resolved
<string name="quest_is_amenity_inside_title">Is this inside a building?</string>

<string name="quest_kerb_height_title">What’s the height of this curb?</string>
<string name="quest_kerb_height_flush">Same level as road surface</string>
Expand Down