Timed traffic lights is a mess and needs house keeping.

[kianzarrin]

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:

TMPE/TLM/TLM/TrafficLight/Impl/TimedTrafficLightsStep.cs

Line 17 in 7b7bc45

// TODO class should be completely reworked, approx. in version 1.10

TMPE/TLM/TLM/TrafficLight/Impl/TimedTrafficLightsStep.cs

Line 483 in 7b7bc45

// TODO improve & remove

TMPE/TLM/TLM/TrafficLight/Impl/TimedTrafficLightsStep.cs

Line 790 in 7b7bc45

// TODO checking agains getCurrentFrame() is only valid if this is the current step

TMPE/TLM/TLM/TrafficLight/Impl/TimedTrafficLightsStep.cs

Line 1357 in 7b7bc45

// TODO IMPROVE THIS! Liskov substitution principle must hold.

TMPE/TLM/TLM/TrafficLight/Impl/TimedTrafficLights.cs

Line 332 in a7ccdfc

// TODO currently, this method must be called for each node in the node group individually

TMPE/TLM/TLM/Manager/Impl/TrafficLightSimulationManager.cs

Line 556 in a7ccdfc

// TODO most of this should not be necessary at all if the classes around TimedTrafficLights class were properly designed

TMPE/TLM/TLM/TrafficLight/Impl/CustomSegmentLights.cs

Line 69 in a7ccdfc

// TODO improve & remove

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.

---

EDIT: TTL should also invoke the notifier.

[kianzarrin]

@kvakvs are you planning to fix this code as part of your TTL uplift?

[kianzarrin]

this blocks #959

[kvakvs]

This may happen after i am done with speed limits UI, which is in progress right now.
The plan was to fix the TTL UI, shall i start with TTL logic first?

[Monniasza]

Is there any progress?

[kvakvs]

No

[Monniasza]

Does anyone want to fix it?

[kvakvs]

Yes, me. Eventually, after the previous task is done.

[Monniasza]

Yes, me. Eventually, after the previous task is done.

It is blocked by this task.

[Monniasza]

Please add this to the 11.6.0 milestone.

[Monniasza]

Yes, me. Eventually, after the previous task is done.

@kvakvs Please it now.

[s2500111]

This is also hindering progress in #278 ...

[Elesbaan70]

Well, I guess it's time I joined the party here. I have begun work on enhancing timed traffic lights to group lanes according to more properties than just vehicle type. The specific objective is to support flags related to lane displacement, #1392 (the issue is out-of-date as compared to the current development plan).

I initially planned to keep the API changes as isolated as possible and maintain backward compatibility. However, after examining the code and its history, and discussing it with @krzychu124, I have concluded that the current code has what I would call a false API. This "API" is really just a complete exposure of the internal implementation to the outside world. It is evident that the interfaces were never meant to be used that way.

The original purpose of the interfaces is unclear. Maybe someone had unit testing in mind, but they really don't help with that. You can't just slap an interface on something and call it testable. There are design methodologies that need to be followed.

As it stands now, these interfaces just create an added layer of complexity that contributes to what @kianzarrin was saying:

messy code prevents future progress

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/Elesbaan70/TMPE/tree/delete-false-api

Comments?

[kvakvs]

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.
Good luck tho.

[brunoais]

is adamant that the traffic light API has no users at this time.

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...

[snowflitzer]

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 30 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 snowflitzer and I am good friend of LemonG

Please let me know

[kvakvs]

Please let me know

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

[Elesbaan70]

I have done more than 5 years in C# and more than 30 years programming

15 years in C# and about 40 years programming (30 professionally). Looks like we have a powerhouse here? :D

@snowflitzer we could probably collaborate.

[Elesbaan70]

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.

@kvakvs This is true, of course. Explaining my plan a little more thoroughly might help. On the point of uninterrupted attention, I don't think any of us are being paid for this, so "uninterrupted" is relative. I do have a day job, but it's not in software and is not mentally demanding, so this is as uninterrupted as it gets unless someone tells us they're retired and doing this full time. ;-)

I could rewrite this, but that's not what I'm doing. I am doing work to make it easier to revise what is already there. My plan is to leave the existing structure and logic completely intact.

If you look at the single commit that I've made in my delete-false-api branch, you will find that it consists of changing the code line-by-line to refer to the internal implementation instead of to the "false API." For example, every occurrence of ICustomSegmentLights gets changed to CustomSegmentLights.

Why would I do this? Because the interfaces are not serving any of the purposes interfaces normally serve, and they are in the way. Removing interfaces is certainly unusual, but consider this:

1. They are in the API, but it is clear from their design that they were not intended to be an API. They fully expose the internal implementation.
2. They provide no abstraction. Most or all of the properties and methods that should be private or internal are exposed to the outside world through these interfaces.
3. Because the interfaces don't do the normal work of an interface, their effect is the opposite of a well-designed abstraction layer. Instead of simplifying code changes, they increase the lines of code that must be touched for even the most minor changes.
4. The only true abstraction in any of this was that CustomSegmentLights needs to reference CustomSegmentLightsManager and TimedTrafficLightsStep through a common interface. But this abstraction was malformed because it repurposed an interface specific to CustomSegmentLightsManager. Creating a new internal interface for this purpose was the only structural change that I made.

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.

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.)

[Elesbaan70]

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...

@brunoais 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.

[brunoais]

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)

[Elesbaan70]

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)

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 belief/assumption that no one is actually using it.

[originalfoo]

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?

[Elesbaan70]

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.