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

Turn On Red (Previously Right On Red) #25

Merged
merged 19 commits into from
Feb 9, 2019

Conversation

FireController1847
Copy link
Collaborator

@FireController1847 FireController1847 commented Feb 3, 2019

Reference to VictorPhilipp/Cities-Skylines-Traffic-Manager-President-Edition#114

Finally learned how to do this. Separated it from all the other stuff in that PR, and then realised the way it was being done was stupid so did it a better way lol

Sorry 'bout my formatter (I just used default VS2017). In the diffs it looks super off center but in Visual Studio it looks fine (different between tabs & spaces)

But here it is, ready to go!

I also snuck in another debug mode for resources, that way we can understand if there's a missing resource or something (when a resource doesn't load, it crashes the entire menu so it doesn't show up. Spent hours debugging just to find out that a resource wasn't loading).

@krzychu124
Copy link
Member

Is it bug less? 😄
I will try to find some time tomorrow to look at this.
I think I've seen this as separate worshop mod

@krzychu124 krzychu124 added the feature A new distinct feature label Feb 3, 2019
@krzychu124 krzychu124 self-assigned this Feb 3, 2019
@FireController1847
Copy link
Collaborator Author

Is it bug less? 😄
I will try to find some time tomorrow to look at this.
I think I've seen this as separate worshop mod

I have been playing with it for a while now and it seems to work well. However there are a few things to be done (see the TODO comments). I've seen the separate workshop mod as well, however I feel like this is 1) much more fitting for this mod and 2) I was unable to get the other mod to work with TM;PE.

@krzychu124
Copy link
Member

krzychu124 commented Feb 3, 2019

You mean this?

// TODO: If is a wreckless driver, yield / run
// TODO: Disallow cutting into flowing traffic, disrupting the flow.
// TODO: If turning onto a one-way road, allow turning for the opposite direction.

Would be nice to resolve some of them before merging :)

@FireController1847
Copy link
Collaborator Author

FireController1847 commented Feb 3, 2019

I'll try and find some time this week to see if I can't resolve some of those. However with some of those I may be unable to, since I'm still learning how this mod works in its entirety. I'm pretty sure one-way roads won't be that hard, same with wreckless drivers. The cutting into the flow of traffic might be a little harder though. I'll take my best shot when I get the chance

Also, in the future I'd love to help out with more features. One in particular I've been thinking of is having the cars pull over when there's an emergency vehicle near them, although that one would be a lot more complicated than this would be (considering I had a guide with this one also xD)

@krzychu124
Copy link
Member

Don't worry I'm still learning too. Codebase of this game is huge and sometimes complicated :)
BTW I will try to add wiki page in which I'll describe how to attach debugger to running game and place breakpoints at any line 😄

@FireController1847
Copy link
Collaborator Author

FireController1847 commented Feb 4, 2019

Don't worry I'm still learning too. Codebase of this game is huge and sometimes complicated :)

It is extremely complicated. Like, why are there over 15 classes just to control chirper? And WHY are events tied so deeply into the messages for chirper? Even with that, there's no generic list of messages for each event? And that's just one example. 😫

BTW I will try to add wiki page in which I'll describe how to attach debugger to running game and place breakpoints at any line 😄

That sounds awesome, I'd love to see an example of how to debug and place breakpoints. Heh, I spent more time than I'd like to admit figuring out that it logs to a file called "TMPE.log" in the game directory, as well as how to enable debugging. 😃

@FireController1847
Copy link
Collaborator Author

FireController1847 commented Feb 4, 2019

I've fixed two of the four checks. I might be able to work on the other ones tomorrow, and I've got an idea on how to do it.

@FireController1847
Copy link
Collaborator Author

FireController1847 commented Feb 4, 2019

Okay, I'm kind of at a loss, and I have a few questions that you might or might not be able to answer.

  1. Is there any way to get the ID of a vehicle from the Vehicle structure?
  2. Is there any way to get the lane (or segment, but lane preferred) that a vehicle is currently on?
  3. Is there any way to do the opposite; get the vehicles that are currently on a lane?
  4. What is this "space" concept when it comes to lanes?
  5. What the actual heck does NetLane#CheckSpace() do?
  6. What is NetLane#GetReservedSpace()?

I need to know these since this is how I plan on detecting the amount of space to make a turn, and if it is "safe" to make that turn. Other than that, all the other issues have been fixed :)

* On a 3-way road, when there is a road going straight and one branching off, you can't go straight if the light is red.
* On a one-way road, if you're going straight, you can't go straight if the light is red.
* Add better checks for future bugs.
* When detecting if the vehicle is going straight, change from comparing the vehicle's position to the vehicle's turning road (the right or left) to comparing the vehicle's position with the target road (the road the vehicle is going to).
* Compare both the start and end segments when detecting straightness.
@krzychu124
Copy link
Member

If I find some time tomorrow I will try to answer few of your questions because currently I am busy and I have no time for searching and analysis. Moreover I didn't even started learning how path fining algorithm really works. It's quite complicated as you know. Iwill try to find some time and make some notes

@FireController1847
Copy link
Collaborator Author

No worries! I'll also keep digging and see if there's anything I've missed while looking through the code.

@krzychu124 krzychu124 self-requested a review February 5, 2019 23:22
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.

I found small issues in your code, but nothing critical yet 😉
Besides that I have one problem. After disabling this feature from mod options menu I can still see these signs while I'm clicking on node in Junction restrictions mode. BTW you should add visibility condition when selected node has no traffic lights (automated, manual, vanilla - doesn't matter). I think there is no point do display feature controls when there is no traffic lights 😄
Tell me what do you think about my suggestions.

@krzychu124
Copy link
Member

Another issue 😉 As you can see it would be great to hide this sign if there is no right turn 😄
zrzut ekranu 50
Would be nice to see this feature in upcoming patch 1.10.15 😉

@krzychu124
Copy link
Member

#33 (comment)
I've added my thoughts about where this feature should have been located
(toggle button to enable/disable).

* Hopefully it's alright if I used Google Translate
* I spent some time in Photoshop to create icons that I felt fit in much better with the current ones.
* I added an option in segments to check if the segment has a preferred segment (segment to the right on a red light, for example).
* Fixed junction restrictions for turn-on-red
* Change the option name and position in the options menu
* Only show the junction restriction when it applies
@FireController1847
Copy link
Collaborator Author

FireController1847 commented Feb 7, 2019

Hopefully the latest commit solves almost all of your review requests. I've also created some cool looking signs for this. Here's the PSD files I used to make them, as it provides an easy template for making future signs as well. I found the font that is used in the SOS sign, and it turns out that font has a BUNCH of cool road signs on it. It's called, "Roadgeek 2005." You should search it up, it's pretty neat if you ever need to create some more road signs.

On another note, I've noticed that yield signs do exactly what the TODO is having an issue with; detecting right-of-way. I'll take a look at that soon and see how it detects if there's another car in the lane (as when yield signs are enabled, cars do not turn unless it is clear).

By the way, I'm at a loss at why trucks and more work-related vehicles refuse to turn on red.

@FireController1847
Copy link
Collaborator Author

Alright. Other than one final bug against some vehicles not turning on red (the same ones that stop at green lights before continuing, like wtf is up with that? trucks I've noticed do this), this is ready. Wohoo!

@krzychu124
Copy link
Member

Argh... it still not working properly but now in other place. 😭 I think I don't understand that segment ends too... I hope there is no bug up there 😄

@FireController1847
Copy link
Collaborator Author

Which place is it not working? I've tried many different roads and it's working for me

@krzychu124
Copy link
Member

Try this: (right hand driving) left and straight are one-way outgoing

zrzut ekranu 76

@FireController1847
Copy link
Collaborator Author

FireController1847 commented Feb 9, 2019

That is the intended behavior. If turning from a one-way road to another one-way road, you are allowed to turn left at red (it's kind of confusing because it draws the wrong sign, but I don't know how I'd be able to detect when it's a left-on-red for one-ways, and quite frankly I think it's unnecessary work).

@krzychu124
Copy link
Member

krzychu124 commented Feb 9, 2019

Isn't it make cutting other vehicles way? Say I am at right lane, as you can see I can't even turn left (lane arrows) It's confusing to me 😕

Better example:
zrzut ekranu 78
Issue still exists as above

@FireController1847
Copy link
Collaborator Author

FireController1847 commented Feb 9, 2019

I guess from the outer lane, it would technically be cutting into the inner lane, but I think that's illegal Here's some references.

  • The google search I used
  • Left turn from a one-way street into a one-way street. Start the turn from the far left lane. Watch for pedestrians, motorcyclists, and bicyclists between your vehicle and the curb because they can legally use the left turn lane for their left turns. Turn into any lane that is safely open, as shown by the arrows. (ref 1)
  • Drivers also can make a left turn onto a one-way street, unless a posted sign prohibits the movement. The same rules apply: First, come to a complete stop at the red light, and give the right of way to other vehicles in or approaching the intersection. (ref 2)

@originalfoo
Copy link
Member

Have you tested this on roads with bus and bike lanes?

Also, in terms of whether to stop or yield, does it need integrating with the stop/yield/priority signs?

@FireController1847
Copy link
Collaborator Author

FireController1847 commented Feb 9, 2019

@aubergine10 No, this only applies to traffic lights.
@krzychu124 In your edited example, since it is not turning from a one-way into another one-way road, it should not be an option. Is it an option in that example?

By the way, the check for this is in the detection code, not the preferred lane code. Unfortunately I don't know of any way to add it to the preferred lane code, which would prevent the sign from being enabled in that situation.

if (hasPriority && (uTargetSegment == uTurnSegment || (currentSegGeo.IsOneWay() && turnSegGeo?.IsOneWay() == true))) {

Ok, that's odd, it's letting them turn. Will investigate.

@krzychu124
Copy link
Member

Ok, I understand this and yes option to turn right is visible 😕

zrzut ekranu 80

  • Technically if we look closely that outgoing straight road could be registered as right and I have no idea how to hide that sign in this case...

@FireController1847
Copy link
Collaborator Author

FireController1847 commented Feb 9, 2019

You know, what? I gotchya, mate! Haha, I have no idea why this worked but it did. (It's the same logic as before, just 1) separated and 2) adding a check if the opposing direction is a OutgoingOneWay road.

@krzychu124
Copy link
Member

Yup it's working now. Merging, after almost 80 comments 😄

@krzychu124 krzychu124 merged commit 47a7d31 into CitiesSkylinesMods:1.10.15 Feb 9, 2019
@FireController1847
Copy link
Collaborator Author

FireController1847 commented Feb 9, 2019

Hey, you know, it's worth it to take the time a bugtest! Glad we got it to work! Now it's time for me to move on to my next major project for this mod: Better emergency vehicles, particularly, having cars pull over for them. 🤔

@krzychu124
Copy link
Member

I know but it's not master branch yet 😉

@krzychu124
Copy link
Member

I think we've forgot about tram tracks...

@FireController1847
Copy link
Collaborator Author

FireController1847 commented Feb 9, 2019

That's.. unfortunate. I'll check it out.

@originalfoo
Copy link
Member

originalfoo commented Feb 9, 2019

Would it be worth releasing the existing stuff (fixed Esc, localisations, icons) to Steam so people have some improvements, and release the Turn on right/left thing as a separate beta mod to allow community to heavily test it (similar to how Loading Screen Mod has two workshop pages)?

@FireController1847
Copy link
Collaborator Author

Well, actually, it seems that tram tracks also have issues with u-turn and other restrictions. I don't think it's limited to right turns.

@krzychu124
Copy link
Member

krzychu124 commented Feb 9, 2019

Nonetheless it's worth fixing, but I think it needs new issue
With regards to releasing new version I think it's possible and I will add (Beta) to new toggles in options menu 😄

@originalfoo originalfoo added this to the 1.10.15 milestone Feb 14, 2019
@originalfoo originalfoo added the technical Tasks that need to be performed in order to improve quality and maintainability label Jul 16, 2019
@originalfoo originalfoo added JUNCTION RESTRICTIONS Feature: Junction restrictions TRAFFIC LIGHTS Feature: Traffic lights - toggle, timed, etc Overlays Overlays, data vis, etc. labels Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new distinct feature JUNCTION RESTRICTIONS Feature: Junction restrictions Overlays Overlays, data vis, etc. technical Tasks that need to be performed in order to improve quality and maintainability TRAFFIC LIGHTS Feature: Traffic lights - toggle, timed, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants