-
-
Notifications
You must be signed in to change notification settings - Fork 394
AllThePlaces quests #6302
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.
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
base: master
Are you sure you want to change the base?
AllThePlaces quests #6302
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, let's focus on that add All the places-place quest first. Maybe you should just delete the opening hours one for now, to keep the size of the PR managable. It seems to be a stub right now anyway. The PR will be large in any case.
So, you want to introduce a new quest type, one that is not generated from OSM data. For this:
-
AtpDownloaderhooks intoDownloader -
AtpDownloadertellsAtpControllerto persist (update/add/delete) the data -
The responsibility of
AtpControlleris to wrapAtpDaoand provide the ability to listen to updates of the data. -
AtpDao's responsibility is to load/save the ATP data from the database -
Now, you need a
AtpQuestController. It listens to updates fromAtpControllerandMapDataWithEditsSourceand after whatever logic you need to produce quests from these two data sources, in turn creates quests of typeAtpQuestType. -
VisibleQuestsSourcewill listen to updates fromAtpQuestControllerto include those quests for display -
AtpQuestTypeinherits fromQuestTypeandElementEditType, but is not simply aOsmElementQuestTypewhich makes this all so complex. It cannot be aOsmElementQuestTypebecause it doesn't work on OSM elements (it works on the absence of OSM elements. It will be anobject, just like theNoteQuestType. There is just oneAtpQuestType, after all. -
You need a
AtpQuestFormin which whatever data needs to be recorded is recorded. On quest completion, if I understood this correctly, it will create a new OSM element. So, the UI will then create a newElementEditat theElementEditsController. -
The addition of a new
ElementEdittoElementEditsControllerwill then automatically notifyMapDataWithEditsSourceof the update, thus triggering an update toAtpQuestControllertoo. Since theAtpQuestControllerchecks if there is a (similarily named) POI already a the location of the Atp POI, it will tell its listeners (i.e.VisibleQuestsSource) that this one quest (the user just solved) has been removed. That is how it works.
Now, regarding AtpQuestController, you might have noticed that OsmNoteQuestController and OsmQuestController look quite different. The former has no backing DAO in which note quests are persisted. This is because the quest creation logic for OsmNoteQuestController is relatively simple, so it basically just transforms the incoming data (notes) to note quests.
OsmQuestController persist the quests instead because the logic is much more involved (all this quest filter stuff) and thus also has to manage adding/removing quests based on the incoming data.
I can't say which of the two approaches would be more eligible for Atp, but I'd suggest you try the notes approach and see if the performance will be adequate.
That opening hours quest by the way doesn't need all this. It would (probably) be a normal OsmElementQuestType which just additional taps into the Atp data in the same vein as the old now-deleted improveOsm oneway quest did.
So, it would still need the downloader, the controller and the dao.
app/src/commonMain/kotlin/de/westnordost/streetcomplete/data/atp/AtpDao.kt
Outdated
Show resolved
Hide resolved
app/src/commonMain/kotlin/de/westnordost/streetcomplete/data/atp/AtpDao.kt
Show resolved
Hide resolved
app/src/commonMain/kotlin/de/westnordost/streetcomplete/data/DatabaseInitializer.kt
Show resolved
Hide resolved
app/src/commonMain/kotlin/de/westnordost/streetcomplete/data/atp/AtpEntry.kt
Outdated
Show resolved
Hide resolved
| /* See also AddWheelchairAccessBusiness and AddPlaceName, which has a similar list and is/should | ||
| be ordered in the same way for better overview */ | ||
| private val filter by lazy { (""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you copy-pasta that from the opening hours quest? That's of course not acceptable, if it is 100% shared, it should be defined in a shared location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, there should be TODO for it somewhere (added now). But for now I am still unsure should it be a separate quest or added to main opening hours quest.
So have not tried to deduplicate content yet.
app/src/androidMain/kotlin/de/westnordost/streetcomplete/quests/QuestsModule.kt
Outdated
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
d3ea1fa to
784fff0
Compare
|
Is it expected that Due to being in a different subfolder of (I am irritated by |
|
yes, I think thats normal. they are separate gradle modules.
El 30 de agosto de 2025 19:19:40 CEST, Mateusz Konieczny ***@***.***> escribió:
…matkoniecz left a comment (streetcomplete/StreetComplete#6302)
@westnordost
Is it expected that `app/src/androidInstrumentedTest/kotlin/de/westnordost/streetcomplete/data/atp/AtpDaoTest.kt` will have no access to helper function defined in `app/src/commonTest/kotlin/de/westnordost/streetcomplete/testutils/TestDataShortcuts.kt` ?
Due to being in a different subfolder of `src` ?
(I am irritated by `fun createAtpEntry` and `fun atpEntry` - tried to deduplicate but failed so far)
--
Reply to this email directly or view it on GitHub:
#6302 (comment)
You are receiving this because you were mentioned.
Message ID: ***@***.***>
|
It is an attempt to add new quest type. It would take data from https://github.com/alltheplaces/alltheplaces that were processed with https://codeberg.org/matkoniecz/list_how_openstreetmap_can_be_improved_with_alltheplaces_data
you can see processed data at https://matkoniecz.codeberg.page/improving_openstreetmap_using_alltheplaces_dataset/
AllThePlaces POI quest would show to users as "here supposedly POI exists here - can you confirm its existence and specify an exact location?"
Also, it enables improvements to opening hours quest - to detect when OSM data and AllThePlaces data mismatches and to produce quest, before POI age crosses current threshold.
One more possible feature is to even prefill opening hours quest (if no data was collected yet) by AllThePlaces opening hours data (this one is not implemented yet at all).
Relevant discussions include https://community.openstreetmap.org/t/improving-openstreetmap-shop-coverage-with-alltheplaces/119979 and https://community.openstreetmap.org/t/what-you-think-about-importing-opening-hours-data-from-alltheplaces/120608 and https://osmfoundation.org/wiki/Licensing_Working_Group/Minutes/2023-08-14#Ticket#2023081110000064_%E2%80%94_First_party_websites_as_sources and alltheplaces/alltheplaces#5133 and alltheplaces/alltheplaces#8790
Would potentially open way for #5481 (but review of licensing is blocker for now anyway there) and similar ones, but I have no specific plans to work on either for now.
Review of specific spiders like done at https://community.openstreetmap.org/t/what-you-think-about-importing-opening-hours-data-from-alltheplaces/120608/72 could be helpful if anyone is interested. (it is stored in https://codeberg.org/matkoniecz/list_how_openstreetmap_can_be_improved_with_alltheplaces_data/src/branch/master/reviewed_data_sources.py )
If you see bogus data at https://matkoniecz.codeberg.page/improving_openstreetmap_using_alltheplaces_dataset/ reporting it also would be helpful. Probably in https://community.openstreetmap.org/t/improving-openstreetmap-shop-coverage-with-alltheplaces/119979 or https://codeberg.org/matkoniecz/list_how_openstreetmap_can_be_improved_with_alltheplaces_data/issues (@westnordost probably prefers to not get here notifications about all bugs in ATP or my code). Also, if someone is interested in their area but spiders are all disabled now due to lacking review: let me know, I can review at least some.
Helium314#523 was another attempt to do this, alltheplaces/alltheplaces#6787 was discussing making such ATP-OSM comparison a part of the ATP project. Both seem dead as of now.
I started building infrastructure for handling data from new API in SC, mostly copying how note handling is done (all this AtpDao, AtpTable, AtpApiClient)
I am now loooking through OsmQuestController/OsmNoteQuestController and I initially started building AtpQuestController.
So far all code is general, littered with TODOs and there is still ATP-specific code to be actually written. And current code is nonfunctional, quest is not even yet shown on the map. AI is not called, API response is for now hardcoded.
But maybe someone can take a look whether general layout makes sense?
If there is some serious general structure issue then maybe pointing it now would be nice. But I am also entirely fine with people waiting until proper code exists.
Disclaimer: my work on ATP-OSM matching to enable this quest and quest itself is sponsored by European Union via NGI0 via NLnet foundation