-
Notifications
You must be signed in to change notification settings - Fork 85
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
Half overlay #564
Half overlay #564
Conversation
…le() Overlay node raduis is average of all connected segmetns.
Found one small issue (with normal lane arrow tool selected): That half of the segment does not contain any outgoing lanes (it's the 'incoming end' of a one way road). While clicking has no effect, it would be better if the overlay was not displayed when the segment half has no outgoing lanes (ie. tool not applicable). Edit: Alternatively, it could show red 'half overlay' instead of the usual blue color. Not sure which would be best approach (if tool not applicable: no overlay, or different color)? |
Wow great observation! This was already the problem without my half segment highlights. Its a Good idea to fix it as part of PR. I like the Idea of the red overlay too. currently I already use red for when segment selected and ALT is down in my previous commit. So I think I need to change that too.
|
Looks orange to me (when Alt is held down while hovering segment, or Ctrl is held down while hovering a node). |
With this update I see only the highlighted half of the segment gets updated when Alt+Click which is much better. I wonder if it would be worth adding that toggle feature I discussed in #548? So Alt+Click toggles the lane between turning lane or vanilla lane depending on it's current state. |
Possible bug with Alt+Click: I'm on a left hand traffic (LHT / RHD) map, so this turning lane choice seems wrong (it would cut across oncoming traffic): Although maybe I'm not understanding it correctly? In UK, the dedicated turning lane is usually the one of the left, with the lane to the right going ahead+right. Here are some examples... At a junction: At a roundabout (where 'turning lane' means 'you must exit roundabout at next junction'): We even cut up bus lanes to make room for turning lanes: I suspect it may be different in other countries (ie. not purely dependent on the driving side). As such, a mod option may be beneficial at some point? (would have to think carefully how to word it) |
Further to my comment above about which side should get the turning lane, I noticed that if there are three lanes to choose from vanilla automatically chooses these lane arrows: And in that situation an Alt+Click does change the left lane in to a turning lane (which seems correct): (sorry, I know this is getting off topic of the PR, but thought I'd mention it just in case) |
Maybe when holding See #391 (comment) for example of lane highlighting. |
I am not sure if that is practical. The calculations that needs to be done in each game click will be too much (unless if i cache it I guess). In another PR maybe. Mean while I have done this: hmmm. I am going to change the color of alt hover to be strong blue and different from selected segment. |
@aubergine10 Sorry I missed some of your messages
Not any more. I already fix this.
In the last PR we talked about state machine. I created #567 to address it.
Nice catch! that is a bug. ~~I think its more fitting to fix it in #567 . ~~ EDIT:
Damn it I have been using LHD in my code to refer to British roads.:
in my defense its the same terminology as the interface:
|
I regularly make similar mistake, confusing the four terms: LHT, RHD, RHT, LHD See my comment in #567 regarding 'nearside' and 'farside' terminology which is driving direction agnostic. |
After playing around with the highlights I can still spot some caveats. Please put this review on halt until I fix them. I will continue the discussion about colors and other stuff in #548.
EDIT: sorry for the inconvenience. |
I'm not sure it is as user is reporting the exception when save loads or, if not then, when saving a game. Also, we've had similar error reported (see #566) for TM:PE STABLE v10.20 which doesn't even contain these features. I suspect something in base game or steam common distro has changed which is triggering some of the errors reported. That being said, good catch on the null exception when hovering other half of selected segment! |
hoverNodeId will change to the other end of the segment if there is no junction
Ah, I see - the other end of that short node in your GIF doesn't actually have lane arrows, despite being "junction node", because it's 1-way street. Good catch! So: Only half-highlight if there are lane arrows at both end of segment, otherwise highlight whole segment. |
Yes, both segments are 1-way streets, but different hover behavior, which might annoy user and provoke missclicks - you can't click on red highlighted segment (OK) but on that weird blue, sure, then lane arrows window will show up near the other node(I am pretty sure user won't expect that). |
Good catch. I will fix it. :) |
Would it be worth removing the red highlight also? For color vision impaired users, distinguishing between red and blue might be difficult (I'd have to read through my notes on #287 to check). Maybe instead of red highlight, just don't highlight at all. That way it's simple UX: If it highlights, you can click it to do something, otherwise it's not relevant to the selected tool. |
@aubergine10
Good idea! Makes my job easier too :) . In any case I will fix the problem @krzychu124 raised because fixing it also makes my code shorter. One more case. What if we have a junction with one way out. changing lane arrows don't make any sense as there is only one direction to go to. So maybe we shouldn't highlight it? I don't know how to take U turn into consideration. That said it is possible to have lane arrows toward wrong direction for example : in such circumstances you can use lane arrow tool to fix the arrows. It would be nice if we could automatically remove lane arrows toward lane direction. Also we should not allow the user to put lane arrows toward wrong directions. So talking all that into consideration do you think I should allow selection of cases where there is only one way out? |
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.
Code looks good.
Did not try in game.
Agreed.
U-turns are a mess at the moment, there's like 2 or 3 ways to do them (off the top of my head: Lane connectors, and junction restrictions). We should only have one way to implement u-turns. I think there's already some issues tracking that. Possibly related issues: #364, #347, #365, #293
Yup, this is a general problem with all the tools. We need to have a think as to how to handle that situation better and, in particular, make sure the user realises their road changes just screwed up their customisations. Possibly related issues: #368, #457, #43 Will test this PR again later this evening if time permits. |
Should train tracks be excluded? They have no visible lane arrows so any changes made would be invisible to user, which would result in difficult to diagnose problems with rail routing. I suspect this is more general issue with the lane arrows tool. Same with tram tracks: And monorail: And metro: There's some flags (#513 for example) that indicates what vehicles can use the network, so you could filter to just those that allow cars? |
Regarding that stuff above about filtering out non-car networks, that could be a separate PR if desired as it's general issue of the lane arrow tool and not specific to the work you've been doing. The remaining comments from @krzychu124 are more important for this PR. |
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.
See my comment above.
@aubergine10
Yes that one is fixed already.
Good idea. This PR is already growing out of control. Created #585. If there are no bugs in this code lets merge this in before the code gets out of date. |
I am ready for the next round of this PR |
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.
LGTM from usability perspective 👍
Usability relating to other networks being tracked separately in #585.
@krzychu124 will still need to check over the stuff he commented on, which should now be fixed, prior to merge.
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.
Finally 😃 👍
fixes #548 :
When hovering over a segment with the lane arrow tool only half of the segment is highlighted letting the user know which half is being modified.
fixes #555: I took the opportunity to fix this related issue.
I Removed buggy/redundant/weird code .
node highlights have the average radius of all connected segments see #555 (comment)
EDIT by aubergine: Just adding image of the 'half overlay' so reviewers know what to look for:
EDIT: fixed a bug regarding Alt+Click on a (LHT / RHD) map (see ,#564 (comment))