opened
kianzarrin
username_0
There are many untracked TODOs in TTL code explaining what a mess the code is. there are too many to list here but some of them are:

https://github.com/CitiesSkylinesMods/TMPE/blob/7b7bc450cd799cdca78c4bb163a447790925de63/TLM/TLM/TrafficLight/Impl/TimedTrafficLightsStep.cs#L17
https://github.com/CitiesSkylinesMods/TMPE/blob/7b7bc450cd799cdca78c4bb163a447790925de63/TLM/TLM/TrafficLight/Impl/TimedTrafficLightsStep.cs#L483
https://github.com/CitiesSkylinesMods/TMPE/blob/7b7bc450cd799cdca78c4bb163a447790925de63/TLM/TLM/TrafficLight/Impl/TimedTrafficLightsStep.cs#L790
https://github.com/CitiesSkylinesMods/TMPE/blob/7b7bc450cd799cdca78c4bb163a447790925de63/TLM/TLM/TrafficLight/Impl/TimedTrafficLightsStep.cs#L1357
https://github.com/CitiesSkylinesMods/TMPE/blob/a7ccdfcb529e471e64d58b5b1eb2cfe84d48ba41/TLM/TLM/TrafficLight/Impl/TimedTrafficLights.cs#L332
https://github.com/CitiesSkylinesMods/TMPE/blob/a7ccdfcb529e471e64d58b5b1eb2cfe84d48ba41/TLM/TLM/Manager/Impl/TrafficLightSimulationManager.cs#L556
https://github.com/CitiesSkylinesMods/TMPE/blob/a7ccdfcb529e471e64d58b5b1eb2cfe84d48ba41/TLM/TLM/TrafficLight/Impl/CustomSegmentLights.cs#L69

in each of the files above search for TODO to get a more complete list.

messy code prevents future progress. In particular, as part of #959 I want to take a snapshot of timed traffic lights without causing too much future deserialization challenges.
Timed traffic lights is a mess and needs house keeping.
issue
created
kianzarrin
username_0
@username_1 are you planning to fix this code as part of your TTL uplift?
comment
created
kianzarrin
username_0
this blocks #959
comment
created
kvakvs
username_1
This may happen after i am done with speed limits UI, which is in progress right now.
comment
created
Monniasza
username_2
Is three any progress?
comment
created
kvakvs
username_1
No
comment
created
Monniasza
username_2
Does anyone want to fix it?
comment
created
kvakvs
username_1
Yes, me. Eventually, after the previous task is done.
comment
created
Monniasza
username_2
It is blocked by this task.
comment
created
Monniasza
username_2
Please add this to the [11.6.0](https://github.com/CitiesSkylinesMods/TMPE/milestone/35) milestone.
comment
created
Monniasza
username_2
@username_1 Do it now.
comment
created
Monniasza
username_2
comment
created
s2500111
username_3
This is also hindering progress in #278 ...
comment
created
Elesbaan70
username_4
My proposal is that we completely remove these interfaces, while leaving the underlying implementation intact. @krzychu124 is adamant that the traffic light API has no users at this time. This is the right time to get rid of the mess, while we still can. It will allow us to focus on the implementation without having to worry about an untenable API layer. We can take a minimalist approach to rebuilding the API as use cases are identified.

I have a branch which does just that. It completely removes the `TrafficManager.API.TrafficLight` namespace, and the three traffic light APIs in `TrafficManager.API.Manager` are emptied of all their contents for a fresh start.

https://github.com/username_4/TMPE/tree/delete-false-api

Comments?

![alien-sygourney-weaver](https://user-images.githubusercontent.com/95266227/159147560-237548ca-754a-4dc5-b68f-385216391b33.gif)
comment
created
kvakvs
username_1
Big rewrites only possible and successful if you 100% clearly understand the current code and have a new plan and uninterrupted attention for the entire duration. If for any reason you're not able to 100% fully commit your attention to this, then you should go gradually removing small things and replacing with new small things, while checking that the code isn't broken beyond repair on each step of your work.

For now i'd be carefully sceptical about any full rewrite of anything.
comment
created
brunoais
username_5
You mean, code users, right? I use it much (not very much, but much) in my cities. I do confess I spend too much time with them...
comment
created
snowflitzer
username_6
Hey guys, If you guys like I am happy to assist in rewriting and cleaning out the code. 
I have done more than 5 years in C# and more than 40 years programming, I am not saying I am better than you guys but I am very good at analysis and organisation. Maybe give me a go and I am happy to help. 

You guys know me as username_6 and I am good friend of LemonG

Please let me know
comment
created
kvakvs
username_1
Fork on Github, check out source, build it and experiment with it, create a branch with your changes (make sure the changes are short and easy to review and the intent for the changes is clear), and create a pull request when done. https://github.com/CitiesSkylinesMods/TMPE/wiki/Contributing
comment
created
Elesbaan70
username_4
15 years in C# and about 40 years programming (30 professionally). Looks like we have a powerhouse here? :D

@username_6 we could probably collaborate.
comment
created
Elesbaan70
username_4
This was my original plan. Unfortunately, this isn't really possible because all the traffic light classes are so tightly coupled. Every class goes in and plays with the internal implementation of every other class, so anything you do has a cascading effect across the whole thing.

This is a weird situation because well-designed interfaces are supposed to keep this from happening. But because of their design, they have the opposite effect. Strange as it is, removing them makes future work simpler.


(A quick note: When I committed these changes, I had not yet done a detailed line-by-line review. When I did this, I discovered that Visual Studio had done some automatic refactoring that I did not expect, removing extraneous parentheses from some mathematical expressions. None of these represent functional changes, but it's still a mystery to me why VS would do this. I'm considering rolling all of these back, but I'm afraid that might be more error-prone than just leaving them.)
comment
created
Elesbaan70
username_4
@username_5  Do you just mean that you use TMPE in your cities? Or do you mean that you've written code that relies on traffic light types in `TrafficManager.API` namespace?

If the latter, then any significant change will impact you because the current "API" design is untenable. But the work I have done makes no logic changes and negligible structural changes, so it would be very easy to account for on your end. Your needs could also provide a use case to study when designing a new API.
comment
created
brunoais
username_5
I use custom semaphores in my city and I don't use the API. I'm not a code user but I am a feature user (as a player)
comment
created
Elesbaan70
username_4
Gotcha. No breaking changes are planned, functionally. That's not how I roll. ;-)

Even breaking the API would only be acceptable based on its extreme unviability combined with the assumption that no one is actually using it.
comment
created
aubergine10
username_7
IMO we should just rewrite TTL in a way that makes sense, with no regards to backwards compatibility or API.... And only once that's done, then try and find a way to convert old data/api to the new format. I don't expect many mods will be using TTL API - _maybe_ Adaptive Networks?
comment
created
Elesbaan70
username_4
I'm actually a future consumer of the API, but I won't be doing anything with it for a while (probably months before I'm even to that point in development), and I'd rather have something stable that doesn't expose TMPE's innards. I will only need the ability to request traffic light state by lane, and to be notified when the state changes. This should be simple calls to a manager, not a deep dive into TTL's object model.
comment