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.

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

Enable platform quests on relations #5183

Merged
merged 1 commit into from
Aug 21, 2023

Conversation

burrscurr
Copy link
Contributor

@burrscurr burrscurr commented Aug 8, 2023

This commit also considers public_transport=platforms that are relations (e.g. multipolygons). See #5182 for more details. Fixes #5182.

@mnalis
Copy link
Member

mnalis commented Aug 8, 2023

Have you tested this pull request?
(i.e. have you tested in practice that it does fix the problem you note, without breaking other cases)?

@burrscurr
Copy link
Contributor Author

I did not test the changes so far - this was more intendet to outline which quests are (probably) affected and the changes that I think solve the issue.

Should have made that more clear, though. I will change this to a draft.

@burrscurr burrscurr marked this pull request as draft August 8, 2023 22:04
@burrscurr
Copy link
Contributor Author

I now have checked that the bench, bin, is lit, shelter and name quests are now also displayed for relation platforms and are answerable. The quests also (still) come up if the platform is a node or way. (Note: I did not change anything on the bus stop ref quest, because I am not sure whether that one intentionally just searches for nodes.) I'm not sure what else could be affected by the change, so if anyone with more insight into SC internals can point me to other functionality that requires testing, I'd be very grateful.

@mnalis
Copy link
Member

mnalis commented Aug 13, 2023

I now have checked that the bench, bin, is lit, shelter and name quests are now also displayed for relation platforms and are answerable. The quests also (still) come up if the platform is a node or way

That seems to be about it. StreetComplete Quests are self-contained, so no other quest should behave differently except the quests you changed. As you tested them they work OK for both old and new behaviour, it looks good to me. Feel free to mark it as ready for review.

(Note: I did not change anything on the bus stop ref quest, because I am not sure whether that one intentionally just searches for nodes.)

Good question. I recall some issues with problems when some transport information is mapped both as node and way (see e.g. #5132 and linked issues), so I'm not sure if there is a reason for that.

But that one can be always be added as separate PR if that turns out to be safe to do (and if not safe, another PR documenting in the code why it only looks for nodes would be welcome.)

@westnordost
Copy link
Member

Hmm, I think nothing speaks against this...

The bus stop quests especially in regard to railway platforms have been subject of various discussions in the past, but I guess since railway platforms are now also included in the quest, it makes sense to also look for relations (i.e. railway platforms with a hole to accommodate stairs).

Looking at git blame, other than me, @FloEdelmann has some lines of code there, so I am just going to ping him if he has any concerns.

@westnordost westnordost marked this pull request as ready for review August 17, 2023 12:32
@FloEdelmann
Copy link
Member

I don't think anything speaks against this :)

@westnordost westnordost merged commit 6d8f787 into streetcomplete:master Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Public transport platform quests aren't available if platform is a relation
4 participants