-
-
Notifications
You must be signed in to change notification settings - Fork 358
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
ask about barrier on intersection of barrier line and path [ready for review, again] #3515
Conversation
app/src/main/java/de/westnordost/streetcomplete/quests/QuestModule.kt
Outdated
Show resolved
Hide resolved
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.
I'd very much prefer if there was no class hierarchy to share code. There are other ways to share code. (See e.g. KerbUtil)
Part or most of the filter logic could certainly go into a common utils file. The function that is called should have a clear general interface though. The primitives could move to SphericalEarthMath if they are primitive/general enough
see say streetcomplete#3515 (review) I half-remembered this but I was unsure (again)
Some new ideas for the question:
sourced from Discord OSM Server |
Or "What kind of barrier is here?" It doesn't really matter for the user where the app sourced the information from that at this point, there is (very probably) some kind of barrier. |
It is very useful when path and road close to each other have different barriers. In other words I think that in this case this info can be easily made available and is likely to be useful so I want to make it available. In related news I tried making icons and run into some issues described at https://graphicdesign.stackexchange.com/questions/154894/why-path-intersection-in-inkscape-is-not-fully-accurate (I hope that someone will reply there) |
c3309a6
to
c4b6161
Compare
c4b6161
to
34fd289
Compare
I suspect that @westnordost is not entirely convinced about splitting this quest - but I would go in opposite direction, merge in here asking about In meantime I tried making icons. |
24b702a
to
bad8131
Compare
What is the status of this PR, is it ready to review / merge? Any open questions? |
It is ready to merge (after solving minor conflict on language files) but
Why two quests? It is very useful when path and road close to each other have different barriers. I think that in this case this info can be easily made available and is likely to be useful so I want to make it available. I even thought about merging in question about |
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.
app/src/main/java/de/westnordost/streetcomplete/quests/barrier_type/AddBarrierOnRoad.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/de/westnordost/streetcomplete/quests/barrier_type/AddBarrierOnPath.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/de/westnordost/streetcomplete/quests/barrier_type/AddBarrierOnRoad.kt
Outdated
Show resolved
Hide resolved
...n/java/de/westnordost/streetcomplete/quests/barrier_type/DetectWayBarrierIntersectionUtil.kt
Show resolved
Hide resolved
…_type/AddBarrierOnRoad.kt
…_type/AddBarrierOnPath.kt
…_type/AddBarrierOnRoad.kt Co-authored-by: Flo Edelmann <florian-edelmann@online.de>
Co-authored-by: Flo Edelmann <florian-edelmann@online.de>
…_type/AddBarrierOnPath.kt Co-authored-by: Flo Edelmann <florian-edelmann@online.de>
Thanks, nice to hear it (especially as it is likely the first real icon that I made on my own and actually liked) |
app/src/main/java/de/westnordost/streetcomplete/quests/findNodesAtCrossingsOf.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: Flo Edelmann <florian-edelmann@online.de>
Co-authored-by: Flo Edelmann <florian-edelmann@online.de>
Co-authored-by: Flo Edelmann <florian-edelmann@online.de>
Co-authored-by: Flo Edelmann <florian-edelmann@online.de>
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.
Please see the three unresolved comments above:
- ask about barrier on intersection of barrier line and path [ready for review, again] #3515 (comment) (the bottom part; unfortunately this is too large for a suggested change on GitHub)
- ask about barrier on intersection of barrier line and path [ready for review, again] #3515 (comment)
- ask about barrier on intersection of barrier line and path [ready for review, again] #3515 (comment)
After those are resolved, I'll approve this MR.
Co-authored-by: Flo Edelmann <florian-edelmann@online.de>
Hmm, I do not really find this additional bonus change as a better one. Too many things happening at all for me.
This should never happen as only nodes without tags are taken. Though "Error: this should never happen, it is impossible" is not unusual. Not sure. Even if it crashes, then crash and clear info that things went horribly wrong would be better than silent data corruption.
Done. |
Even if it's not currently, there's no reason that function couldn't be used for other barrier related quests which may have barrier set initially (or simply if you add barrier=yes), at which point it would break, so I'd say it's worth changing. |
Co-authored-by: Flo Edelmann <florian-edelmann@online.de>
Oh right, that can happen even now as it is now a generic code.
demonstrated quite well I would take down entire quest - that I implemented recently, nice one. Thanks for spotting! |
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.
Very clean implementation, thank you! Just two small comments:
app/src/main/java/de/westnordost/streetcomplete/quests/findNodesAtCrossingsOf.kt
Outdated
Show resolved
Hide resolved
...t/java/de/westnordost/streetcomplete/quests/barrier_type/AddDetectBarrierIntersectionTest.kt
Show resolved
Hide resolved
Co-authored-by: Tobias Zwick <osm@westnordost.de>
fixes #3372
What would be the good quest question in this quest?
both
are unfortunate and seem clunky to me.
Below are things that I implemented to my satisfaction (more or less) but I can be easily convinced to change that.
Is it a good idea to keep complex "path must actually cross barrier" logic? It was implemented already and will keep out some rare confusing cases.
Is it fine to duplicate this logic and helper function from crossing quest? If no where would be the best place to put it?
I plan to have both path quest and road quest here. For now just path is implemented, with shared code in a separate class.
(why separate quest for roads? Because it is typical to have gate for road and separately mapped path without a barrier along it, and separate quest logos and titles would be an useful help here)