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

boost lane marker hovering #626

Closed

Conversation

kianzarrin
Copy link
Collaborator

fixes #625
added DetermineHoveredLane() to DetermineHoveredElements(). Then I use this to find the correct lane marker.

declared HoveredLaneID in TrafficMangerTool and SubTool.

Additionally I took the opportunity and removed setters from SubTool.HoveredNodeID and SubTool.HoveredSegmentID. Such setters are unused, unnecessary and using them can cause bugs.

Copy link
Collaborator

@kvakvs kvakvs left a comment

Choose a reason for hiding this comment

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

Nice

Copy link
Member

@krzychu124 krzychu124 left a comment

Choose a reason for hiding this comment

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

It doesn't fix anything regards raycasting, it makes marker selection weird in some cases, not to mention it makes detection a bit slower (~40-50%)

  • before:
    Benchmark#MaybeCreateBenchmark#DetermineHoveredElements: avg. 0.06269605ms
  • after:
    Benchmark#MaybeCreateBenchmark#DetermineHoveredElements: avg. 0.09171094ms
    Benchmark#MaybeCreateBenchmark#DetermineHoveredElements: avg. 0.1039268ms

hover_1
hover_2

@kianzarrin
Copy link
Collaborator Author

@krzychu124

It doesn't fix anything regards raycasting.

There was a rendering issue and there was a raycasting issue. I changed the raycasting to use all the lane area rather than only marker. I have a feeling that when you say fix raycasting you mean something else.

not to mention it makes detection a bit slower (~40-50%)

Yeah I was worried about the performance. other pull requests I created reduce performance even more. There are ways to increase performance but should I? I mean 0.1ms is not much is it? If it is then we should create an issue and I can work on some ideas I have.

@kianzarrin
Copy link
Collaborator Author

This is really bad with bridges. I need to fix this.

@kvakvs
Copy link
Collaborator

kvakvs commented Jan 27, 2020

Lane marker raycast should not target the ground level but the road plane, at the height where the marker is located. Then it will always look correct, at any height on or under any bridge. If that is done, then also render depth can be reduced to only cover range from slightly under the road level to slightly above the tallest car or train-car level.

@kianzarrin
Copy link
Collaborator Author

@kvakvs

Lane marker raycast should not target the ground level but the road plane,
My biggest problem with lane markers is when a truck is over it. targeting road plane does not help with that.
I am using mouse pos instead of hit pos which is wrong. I will fix that right now.

@kianzarrin
Copy link
Collaborator Author

@krzychu124

it makes marker selection weird in some cases

what is the expected behavior?

@kianzarrin
Copy link
Collaborator Author

kianzarrin commented Jan 29, 2020

This still doesn't work when there is a bridge passing over selected markers.

@originalfoo originalfoo changed the title #625 boost lane marker hovering boost lane marker hovering Jan 29, 2020
@originalfoo originalfoo added LANE ROUTING Feature: Lane arrows / connectors UI User interface updates Usability Make mod easier to use labels Jan 29, 2020
@originalfoo
Copy link
Member

testing...

@originalfoo originalfoo added the DO NOT MERGE YET Don't merge this PR, even if approved, until further notice label Jan 30, 2020
@originalfoo
Copy link
Member

originalfoo commented Jan 30, 2020

Overall I like where this is heading; when it works, it works really well - much easier to pick and place the connector lines.

However, there are still some issues. You're probably aware of these, but I'll go through each one I found just in case...

Roads under bridges are cumbersome to work with

  • Difficult to select node
  • Difficult to select lane markers

under brdige

Tunnels are also cumbersome to work with

  • Difficult to select lane markers
  • Underground view (PgDn while tool active) doesn't improve usability

tunnel

Road bridges can be cubersome to work with

  • Difficult to select lane markers on the bridge

road bridge

Rail bridges are more cumbersome to work with

  • The connector line does not appear

rail bridge

Nodeless junctions make some markers inaccessible

  • A nodeless junction can result in lane markers appearing on top of each other
  • Only the topmost marker can be selected
  • Example below is from Railway collection; most of its networks have nodeless variants
  • Any network that offers nodeless junctions (eg. some CSUE roads)
  • I think in recent CSUR Toolbox mod they found a workaround (see "Bug Fixes" section in description of that mod)

nodeless

Things I didn't test:

  • Monorails
  • Tram roads (in particular the LRT networks - especially sinking one - by clus will need testing)
  • Metro
  • The CSUE, CSUR, etc roads by AmamIya

@originalfoo originalfoo added the Accessibility Color blindness, etc. label Jan 30, 2020
@kianzarrin kianzarrin closed this Feb 1, 2020
@kianzarrin
Copy link
Collaborator Author

@aubergine10 OH no! why did you tested something I already had said it won't work? I was waiting for you guys to tell me if I should close down this review:
Screenshot (374)

I am doing it now!

@originalfoo
Copy link
Member

I liked the premise of the idea; in the places where it worked, it worked really well and made it easier to interact with the lane markers; sadly it just made other places much more difficult.

@kianzarrin kianzarrin deleted the 625-boost-lane-hover branch February 3, 2020 06:41
@originalfoo originalfoo added this to the 11.1 milestone Feb 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accessibility Color blindness, etc. DO NOT MERGE YET Don't merge this PR, even if approved, until further notice LANE ROUTING Feature: Lane arrows / connectors UI User interface updates Usability Make mod easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lane connector should raycast the whole lane
4 participants