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

Heinous Junction Restrictions overlay #845

Merged
merged 13 commits into from
Apr 28, 2020

Conversation

kvakvs
Copy link
Collaborator

@kvakvs kvakvs commented Apr 16, 2020

Fixes #633

Problem

Different display style of Junction Restrictions overlay was very confusing see #633
⚠ Please note that the code is using segment middle point as the direction, and it is not a bug that on curved segments the signs look a bit off to the side. If this is undesired, i can calculate more precise endpoint tangent which will look better.

Solution

  • Refactored overlay code for Junction Restrictions
  • Now readonly icons show as greyed out, smaller
  • Now active icons show as full opaque, orange tint on hover
  • Now icons always form a grid along the vector towards the segment center, always from the node position. Before: Was sometimes shifted left or right.
  • The logic for which sign to be displayed: Not modified

bild

bild

@krzychu124
Copy link
Member

Probably out of scope of that PR, but why we are showing pedestrian crossing icon if it's not possible to change its state? Highway roads doesn't have pedestrian lanes and also cannot have pedestrian crossing, obviously.

I'll try to test it later today.

@originalfoo
Copy link
Member

This is looking a lot better, I can now clearly see which road the 'enter blocked junction' icons are relating to.

In this case it also seems to be putting them on the correct side of the road (left hand traffic map) which is awesome!

image

However, there's some cases where it seems to put the icons on the other side of the road, like this:

image

I thought it might be something to do with 1-way roads, but on same type of road it sometimes puts them in correct side like so:

image

It's only very minor point; I'm already happy with the big improvement from this PR and we could look at further refinements at a later date.

@kianzarrin
Copy link
Collaborator

Please note that the code is using segment middle point as the direction, and it is not a bug that on curved segments the signs look a bit off to the side. If this is undesired, i can calculate more precise endpoint tangent which will look better.

CS has calculated the middle position between two corners like this

//psudo-code:
pointA, tanA = NetSegment.CalculateCorner(junctionID, left side )
pointB, tanB = NetSegment.CalculateCorner(junctionID, right side)
centerPos = (postA+pointB)/2

//the original code can be find in this overloaded method:
//private void RefreshJunctionData(ushort nodeID, int segmentIndex, ushort nodeSegment, Vector3 centerPos, ref uint instanceIndex, ref RenderManager.Instance data)

you can use dirA and dirB from code above to align the signs. if you want to be even more thorough than that you should travel across curved road using Bezier3.Travel. I have done this is my pedestrian bridge builder:
https://github.com/kianzarrin/PedestrianBridge/blob/d815c3920906965f7539f5d552ee45c45389a1c9/PedestrianBridge/Shapes/RoadBridgeWrapper.cs#L26
and
https://github.com/kianzarrin/PedestrianBridge/blob/d815c3920906965f7539f5d552ee45c45389a1c9/PedestrianBridge/Shapes/RoadSideWrapper.cs#L69

I think calculating the tangent is going to be more complicated.

In any case this is just in case you were interested. I don't think it is so important to put the signs at super accurate place.

@kianzarrin
Copy link
Collaborator

bild

I don't know if this fits the scope of this review but there is no point to show pedestrian crossing overlays on roads without pedestrian crossing.

So IsPedestrianCrossingAllowedConfigurable should check if the segment does not have pedestrian crossing and the segments to the left and right also do not have pedestrian crossings then pedestrian crossing is not configurable. In such a case we can make user's job easier and not show the pedestrian crossings overlay to the user at all.

In future when we have Pedestrian tool (#40) then we can show a disabled pedestrian overlay and show a guide message to the user if he tries to click it. but here ... its too much info.

this is particularly important on roundabouts where there are a lot of traffic rules to set.

@kvakvs
Copy link
Collaborator Author

kvakvs commented Apr 17, 2020

I had good results in the past when calculating tangent as a vector between 0.9f point and 1.0f point (or 0f and 0.1f if inverted direction).

if (this.debug_
   || (configurable
   && (!this.ViewOnly || !allowed))) {

The actual rendering code will render if debug is enabled. Where debug is !viewOnly && DebugSwitch.JunctionRestrictions.Get()

The logic where its displayed in configurable && (!viewOnly || !allowed) might be the reason its visible where it shouldn't? As viewOnly is false, means it will display all configurable locations where its !allowed.

@originalfoo originalfoo added JUNCTION RESTRICTIONS Feature: Junction restrictions Overlays Overlays, data vis, etc. UI User interface updates Usability Make mod easier to use labels Apr 17, 2020
@originalfoo originalfoo added this to the 11.5.0 milestone Apr 17, 2020
@kianzarrin
Copy link
Collaborator

kianzarrin commented Apr 19, 2020

Several issues that is probably outside of the scope of this review and exists in your branch as well as in master:

  • if I turn junction restrictions overlay on and off from options while TMPE panel is open, mass edit overlay stops working until I close and open TMPE panel
  • (your branch and master): activating the junction restriction tool turns off persistent overlay.
  • I wish there was a option I could disable zebra crossings from persistent overlay because with my hide crossings mod, it is no longer all that necessary. maybe if hide crossing mod exists, then take out zebra crossings from persistent overlay?
  • EDIT:the priority signs do not get smaller in persistent overlay unlike other overlays.

@kvakvs
Copy link
Collaborator Author

kvakvs commented Apr 19, 2020

@kianzarrin yes you can copy this comment later to the future PR when i'll be working on the LaneConnectors refactoring, it makes sense to fix those problems then.

@originalfoo
Copy link
Member

Regarding the hide crossings mod: Note that some roads (eg. UK Roads assets) don't have zebra crossings so the only way to see if there is a crossing is via the overlay icons.

@kianzarrin
Copy link
Collaborator

@aubergine10 Regarding the hide crossings mod: Note that some roads (eg. UK Roads assets) don't have zebra crossings so the only way to see if there is a crossing is via the overlay icons.

i said its no longer THAT necessary. meaning that it is necessary but not necessary enough to show it in persistent overlay. at the very least it would be nice to have the option to turn it off.
we are creating a separate pedestrian tool in future so that might be the solution.
Also for highways, we can put a pedestrian prop so that it would not look weird when people cross the street.

if road does not have 2+ pedestrian lanes. then add pedestrian crossing prop.

in any case there are a lot of urban roads without pedestrian crossings.

@kianzarrin
Copy link
Collaborator

looking better:

OLD:

Screenshot (882)
Screenshot (883)

VS NEW:

Screenshot (886)
Screenshot (885)

@kianzarrin
Copy link
Collaborator

kianzarrin commented Apr 19, 2020

@kvakvs I noticed that the sprites are always in the same place regardless of junction size:
Screenshot (867)
Screenshot (866)
Screenshot (886)

you should move the sprites further back as the junction gets bigger. so i think using NetSegment.CalculateCorner() is the only way (and pretty easy too).

@kianzarrin
Copy link
Collaborator

Why is it the your sprites are overlapping? is that intentional?
Screenshot (874)
Screenshot (872)

int numSignsPerRow,
float signSize,
bool viewOnly) {
// Vector3 centerStart = nodePos + (yu * (viewOnly ? 5f : 14f));
Copy link
Collaborator

Choose a reason for hiding this comment

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

commented out code


this.zero_ = GetZeroPosition(
nodePos: nodePos,
xu: this.yBasis,
Copy link
Collaborator

Choose a reason for hiding this comment

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

again x and y are swapping!

@kvakvs
Copy link
Collaborator Author

kvakvs commented Apr 21, 2020

@kianzarrin the C:S code for calculating corners is not used anywhere in TM:PE, there's no example of its meaning. Also the source is massive like 5 pages of dense vector calculations, it can't be fast or simple. I almost matched everything to your suggestion but this will be called for each segment each frame...
Maybe i can find a reasonable way of making it work and be fast.

Copy link
Member

@originalfoo originalfoo left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I agree with what Kian is saying regarding corner radius; if there was some way to ensure the icons do not appear inside the node but instead get pushed outwards so they are over the road segment, that would help with visual comprehension of what roads they relate to. That being said, this is still major improvement over what it used to be; the corner radius stuff could come as subsequent PR?

@kvakvs
Copy link
Collaborator Author

kvakvs commented Apr 22, 2020

Suggest @kianzarrin either add commit to this, or merge and come with your change later. I am literally down to experimentation if i try that corner calculation. That takes significant effort from me while you seem to know what is needed exactly.

@kianzarrin
Copy link
Collaborator

@kianzarrin the C:S code for calculating corners is not used anywhere in TM:PE, there's no example of its meaning. Also the source is massive like 5 pages of dense vector calculations, it can't be fast or simple. I almost matched everything to your suggestion but this will be called for each segment each frame...
Maybe i can find a reasonable way of making it work and be fast.

@kvakvs sorry for the late response. I lost the review! In NetNodeRenderInstance corner is calculated in every call to render instance. I guess all those pages of calculation is not as slow as you think because there are so many branches and only one branch is taken. (i guess branching makes code even slower). In any case I think we should cache this stuff.

Suggest @kianzarrin either add commit to this, or merge and come with your change later. I am literally down to experimentation if i try that corner calculation. That takes significant effort from me while you seem to know what is needed exactly.

Calculating the corner looks scare but it's disappointingly easy once you get to it (disappointing in case you want a challenge). Sure I can write some code and commit that in.

BTW its nice that I can (for some reason) see the conflict. its helpful to know that its nothing.

-define constants to make sure sign sizes have harmony in both normal mode and view mode.
- simplified the code.
@kianzarrin
Copy link
Collaborator

I simplified the code (I hope you don't mind!), changed the private interface a bit and deleted a few methods. also changed some variable names for example zero_ -> origin_. Also fixed the x and y inconsistency (I removed so much code that x and y happen only once :) ).

I use `CalculateCorner -> position and direction next to the crosswalks' for more precise calculations.
Screenshot (903)

I defined some constants to shrink signs harmoniously. now the view only mode signs no longer overlap
Screenshot (904)

PS: the pictures above are a bit old. I moved the overlays a bit further up so that they don't cover the crosswalks.

Copy link
Collaborator

@kianzarrin kianzarrin left a comment

Choose a reason for hiding this comment

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

@kvakvs Overlay.cs should move to UI.Helpers ( I didn't do it my self because it would have made it difficult for you to see what exactly have I changed).

@kianzarrin
Copy link
Collaborator

Screenshot (905)
Screenshot (906)

@kianzarrin
Copy link
Collaborator

In the picture bellow my FPS dropped from 25(no overlay) to 20 (a lot of overlay).
Screenshot (907)

This is far slower than @kvakvs code that drops FPS from 25 to 24

@kianzarrin
Copy link
Collaborator

Screenshot (908)
My FPS drops from 25 to 20 when I view the overlay
@kvakvs code drops FPS from 25 to 24.
Can you maybe cache the overlays?

@kianzarrin
Copy link
Collaborator

Wow my code is so clever it can even realise when the segment end is tilted:
Screenshot (909)

@kvakvs
Copy link
Collaborator Author

kvakvs commented Apr 26, 2020

Not doing any caching at the moment.
This was intended to be a bugfix here not a huge rework :) Thanks @kianzarrin for your changes

Copy link
Collaborator

@kianzarrin kianzarrin left a comment

Choose a reason for hiding this comment

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

@kvakvs This was intended to be a bugfix here not a huge rework

This would be a good argument if this PR (mainly my corner calculations) was not dropping FPS. The FPS does not drop too much so I its tolerable. Tell you what I i'll cache corner calculations and upload it to this PR. That will fix the FPS issue.

TLM/TLM/UI/SubTools/JunctionRestrictionsTool.cs Outdated Show resolved Hide resolved
@kianzarrin
Copy link
Collaborator

I added calculate corner cache, renamed priority signs overlay to traffic signs overlay (because it is only used for junction restriction for now) and changed a couple of comments.

dirX_ is not perpendicular to dirY_ (and I added a comment for that). hopefully that is not confusing. in any case there are axis systems without perpendicular axis so I think that is fine.

@kianzarrin kianzarrin added the DO NOT MERGE YET Don't merge this PR, even if approved, until further notice label Apr 27, 2020
@kianzarrin
Copy link
Collaborator

kianzarrin commented Apr 27, 2020

Calculate corner is being cached to soon returning null 0.

@kianzarrin kianzarrin removed the DO NOT MERGE YET Don't merge this PR, even if approved, until further notice label Apr 27, 2020
@kianzarrin
Copy link
Collaborator

I added patch to Calculate corners after CalculateSegment() when the data is ready. It seems it takes a some while until segments are calculated.

@kianzarrin
Copy link
Collaborator

Created #872 to discuss further when and where to perform recalculations.

@kvakvs kvakvs merged commit 78cd4a7 into CitiesSkylinesMods:master Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JUNCTION RESTRICTIONS Feature: Junction restrictions Overlays Overlays, data vis, etc. UI User interface updates Usability Make mod easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix heinous junction restriction persistent overlay positions
4 participants