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

Updating Traffic Incidents #1719

Merged
merged 4 commits into from Dec 19, 2018
Merged

Updating Traffic Incidents #1719

merged 4 commits into from Dec 19, 2018

Conversation

ghost
Copy link

@ghost ghost commented Dec 5, 2018

[@nvkelso: followup issue to https://github.com//issues/1598.]

  • Add warning_level to be available
  • Adding traffic_flow to show the effect of the incident has had on the flow of traffic

- Add warning_level to be available
- Adding traffic_flow to show the effect of the incident has had on the flow of traffic
@ghost
Copy link
Author

ghost commented Dec 5, 2018

@zaczkows please have a look. I've added the impact on the flow of traffic and made the previously "internal warning level" values to be publicly available.

@nvkelso
Copy link
Member

nvkelso commented Dec 5, 2018

Regarding warning_level, I don't think we should include this as it's routing focused not map display focused. It should inform the min_zoom, and even a possible collision rank. Please make a stronger case for it if you want it.

Previous discussion around this from Oct. 5th:

I think we're already setting the kind_detail of traffic_flow features in the traffic_flow layer to those values. So again I'm not sure why this change is necessary. Please provide more information.

@nvkelso nvkelso self-requested a review December 5, 2018 19:09
Copy link
Member

@nvkelso nvkelso left a comment

Choose a reason for hiding this comment

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

See comments in #1719 (comment).

@ghost
Copy link
Author

ghost commented Dec 6, 2018

Regarding warning_level, I don't think we should include this as it's routing focused not map display focused. It should inform the min_zoom, and even a possible collision rank. Please make a stronger case for it if you want it.

Previous discussion around this from Oct. 5th:

I think we're already setting the kind_detail of traffic_flow features in the traffic_flow layer to those values. So again I'm not sure why this change is necessary. Please provide more information.

Hi @nvkelso,

Within the discussion it was said "warning_level could be used to filter out incidents based on zoom level, so I think that using that for min_zoom would work fine". But one could also want to use the warning_level for other purposes such as displaying more dangerous incidents larger than other for instance.

Regarding the addition of traffic_flow it doesn't necessarily need to be of that name, but what I was trying to express with this field is the effect that the Incident has on the traffic around it, for example. There can be an accident, this could simply be slowing down the traffic flow because it has blocked one lane - or it could be blocking the whole road, thus freezing the flow of traffic.

As the field is optional, if it is unknown, it should not be populated in the first place.
@nvkelso
Copy link
Member

nvkelso commented Dec 12, 2018

I can see adding warning_level as an optional property (not common, not common-optional). Really that's a design decision that's individual to a map style implementation and entirely dependent on veracity of that data (which you have well covered ;). Please update the PR.

But I continue to view traffic_flow property in this context as whole routing focused and inappropriate for map display.

- Remove traffic_flow from Traffic Incidents
- Make warning_level optional
@ghost
Copy link
Author

ghost commented Dec 14, 2018

I can see adding warning_level as an optional property (not common, not common-optional). Really that's a design decision that's individual to a map style implementation and entirely dependent on veracity of that data (which you have well covered ;). Please update the PR.

Moved the warning level to optional :)

But I continue to view traffic_flow property in this context as whole routing focused and inappropriate for map display.

I've removed the field. Will need to see if there's another way to fulfil such a use case. I can see how you see this as routing focused, the use case one is trying to fulfill with including such information is to display the effect of the incident.
For example, a construction icon with a black background when its a fully disruptive construction site (No traffic), or a green background when its not obstructing traffic, or yellow when its causing some disruptions.

@nvkelso
Copy link
Member

nvkelso commented Dec 19, 2018

But I continue to view traffic_flow property in this context as whole routing focused and inappropriate for map display.

I've removed the field. Will need to see if there's another way to fulfil such a use case. I can see how you see this as routing focused, the use case one is trying to fulfill with including such information is to display the effect of the incident.
For example, a construction icon with a black background when its a fully disruptive construction site (No traffic), or a green background when its not obstructing traffic, or yellow when its causing some disruptions.

That sounds like a very technical display of data rather than for a general audience ;)

My guess is the combination of min_zoom for these features in this layer, and also displaying with the traffic_flow lines from the other layer will mostly achieve what you want. But I'm happy to revisit in the next release. Let's get this merged now!

and which values are low and highest.
Copy link
Member

@nvkelso nvkelso left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

NOTE: the list of values for warning_level was confusing so I reworked that.

@nvkelso nvkelso merged commit 3337912 into tilezen:master Dec 19, 2018
@ghost ghost deleted the patch-2 branch February 8, 2019 14:02
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.

1 participant