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

Resurvey quest dismissed by splitting way #3567

Closed
lukashass opened this issue Dec 6, 2021 · 16 comments
Closed

Resurvey quest dismissed by splitting way #3567

lukashass opened this issue Dec 6, 2021 · 16 comments
Labels
wontfix idea rejected because it is out of scope or because required work is not matching expected benefits

Comments

@lukashass
Copy link

After answering a resurvey quest (.i.e a quest for a value that is already present but old) with "Differs along the way..." and splitting the way no new quests appear for the newly created split ways. I had this problem with a "What is the surface of this path?" quest, but this probably applies to others, as well.

I suspect that no quests appear for the split ways since they have been created/updated by splitting and thus are not eligible for resurvey.

How to Reproduce

  1. Answer a resurvey quest with "Differs along the way..." and split the way.
  2. 💥 No new quests appear for the split ways.

Versions affected

StreetComplete v38.0

@lukashass lukashass added the bug label Dec 6, 2021
@HolgerJeromin
Copy link
Contributor

HolgerJeromin commented Dec 6, 2021

Resurvey checks for the "last touched for x years" state of a osm way.
By splitting you touch the way, so the resurvey is not triggered afterwards.
I got bitten by that a few times.
All fixes for that were hacks so IMO no one was implemented right now.

Edit: Found my "not as bad of a hackery" suggestion:
#2883 (comment)

@westnordost
Copy link
Member

I suspect that no quests appear for the split ways since they have been created/updated by splitting and thus are not eligible for resurvey.

Yes, correct. I am sorry but this cannot be solved because OSM has no notion of keeping change-dates for single tags, only for the whole element. If a check_date:<key> tag is present, it will be used correctly but if there is none, you are out of luck.

@westnordost westnordost added wontfix idea rejected because it is out of scope or because required work is not matching expected benefits and removed bug labels Dec 6, 2021
@westnordost
Copy link
Member

(Also anyway another problem is that there is no notion of splitting ways in OSM. Splitting ways in OSM is a hack: Actually new ways are created, thus no history carries over to the new way(s), it is treated like a new object.)

@matkoniecz
Copy link
Member

as workaround you may do edit in Vespucci, create note or wait few years for quest to reappear

@riQQ
Copy link
Collaborator

riQQ commented Dec 6, 2021

(Also anyway another problem is that there is no notion of splitting ways in OSM. Splitting ways in OSM is a hack: Actually new ways are created, thus no history carries over to the new way(s), it is treated like a new object.)

Just to add more information. The original way id with its history is preserved in the chunk with the most nodes. But the other chunks are new ways without any history.

/* Instead of deleting the old way and replacing it with the new splitted chunks, one of the
chunks should use the id of the old way, so that it inherits the OSM history of the previous
way. The chunk with the most nodes is selected for this.
This is the same behavior as JOSM and Vespucci. */
val indexOfChunkToKeep = nodesChunks.indexOfMaxBy { it.size }
val tags = originalWay.tags.toMutableMap()
removeTagsThatArePotentiallyWrongAfterSplit(tags)
return nodesChunks.mapIndexed { index, nodes ->
if(index == indexOfChunkToKeep) {
Way(originalWay.id, nodes, tags, originalWay.version, currentTimeMillis())
}
else {
Way(idProvider.nextWayId(), nodes, tags, 0, currentTimeMillis())
}
}

@FloEdelmann
Copy link
Member

Could a workaround be setting check_date to the last modification date when splitting a way?

See #3130 (comment) by @peternewman (reply to @westnordost).

@matkoniecz
Copy link
Member

Could a workaround be setting check_date to the last modification date when splitting a way?

It would be necessary to guess it or add fake one.

@HolgerJeromin
Copy link
Contributor

HolgerJeromin commented Dec 7, 2021

It would be necessary to guess it or add fake one.

The old date is known. So it could be set, yes.
This was my workaround mentioned in
#2883 (comment)
Tobias was not happy.

@matkoniecz
Copy link
Member

matkoniecz commented Dec 7, 2021

The old date is known

That is what I described as a guessing because we know only date of latest edit (which could be for example bot or revert of vandalism), not latest check date.

@westnordost
Copy link
Member

westnordost commented Dec 7, 2021

Could a workaround be setting check_date to the last modification date when splitting a way?

This workaround would currently not do anything because CompareTagAge::matches only checks for the timestamp an object was edited and the check_date:<key> tags. But I see no reason why it could not also check for the check_date keys too as a fallback.

So, this workaround (set check_date to last edit timestamp) could work relatively uncomplicated.

The issue is of course that the check_date is not updated by any resurvey quest, only the check_date:<key> keys and rightfully so. So in these cases, there will be a check_date attached to the way for ~forever. Setting check_date:<key> to the last edit timestamp is not possible because the split-way-action doesn't and shouldn't know the context in which quest it is being done and even if it knew, setting check_date:surface would make little sense, as the resurvey-quests for cycleways, sidewalks, parking lanes etc etc should be affected too.

@HolgerJeromin
Copy link
Contributor

because the split-way-action doesn't and shouldn't know the context in which quest it is being done...

Sure? It did in the past: #2865

@HolgerJeromin
Copy link
Contributor

setting check_date:surface would make little sense, as the resurvey-quests for cycleways, sidewalks, parking lanes etc etc should be affected too.

The other quests were not shown to the users. So it is ok to "burn" them with touching them (as it is a general resurvey limitation and not related to splitting).
But the current quest was shown to user and he interacted with it. So delaying the quest for a few years is very confusing.

@Helium314
Copy link
Collaborator

Is it actually necessary to update the timestamp when splitting?
As far as I understand the timestamp in OSM will be set to upload time anyway, so timestamp in local db does not need to be updated.

@westnordost
Copy link
Member

The timestamp updated on splitting? Where?

@Helium314
Copy link
Collaborator

I didn't check in code, but the surface resurvey quest is gone after splitting, even without upload.

@westnordost
Copy link
Member

westnordost commented Oct 17, 2022

Indeed.

SplitWayAction, line 134-136.

Changing that to the timestamp of the original way would be OK by me as a fix (that would only work for auto-sync off). So if you care, you can try if this works and create a PR. (Also add comments why this is done)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix idea rejected because it is out of scope or because required work is not matching expected benefits
Projects
None yet
Development

No branches or pull requests

7 participants