search

CitiesSkylinesMods / TMPE

Cities: Skylines Traffic Manager: President Edition
https://steamcommunity.com/sharedfiles/filedetails/?id=1637663252
MIT License
574 stars 85 forks source link

Remove flawed TTL API #1495

Closed Elesbaan70 closed 2 years ago

Elesbaan70 commented 2 years ago

This is my solution to the problem of false and unmaintainable traffic light API, as discussed here: https://github.com/CitiesSkylinesMods/TMPE/issues/967#issuecomment-1073163111

If this or some similar solution is not used, the ability to enhance traffic lights will always be hampered by the need to carry along a fundamentally defective API. It is probably impossible to maintain API compatibility with enhancements of any size, and so the time to nuke the API is now, when we believe that there are no other mods using it.

Elesbaan70 commented 2 years ago

can you please test persistency?

I've done basic testing already, but I've decided to go back and do more. Did you have any specific areas of concern?

Elesbaan70 commented 2 years ago

@Elesbaan70 can you please test persistency?

Persistency is forward- and backward-compatible. Reverting to stable does not work because of #1492 and the versioning issue that will be addressed by #1502. However, reverting to the master branch works fine.

Elesbaan70 commented 2 years ago

On another note, I'm not sure what I should think about the possibility that my first contribution might be nuking an entire API. LOL

originalfoo commented 2 years ago

@Elesbaan70 pls update to latest master branch

originalfoo commented 2 years ago

Ugh, @Elesbaan70 I messed up your PR with recent merged PR - want me to send some commits to fix the merge conflicts?

Elesbaan70 commented 2 years ago

want me to send some commits to fix the merge conflicts?

If you already have it, sure. I suspect I can just do another merge and clean things up pretty easily, in any case.

originalfoo commented 2 years ago

@Elesbaan70 I've done most of the conflicts from the IsStartNode PR so hopefully that will reduce amount of stuff you need to wade through.

Elesbaan70 commented 2 years ago

ok, just fix conflicts :)

Yep, I'll be on that this evening.

Elesbaan70 commented 2 years ago

@aubergine10 Thanks. I did end up reverting it, though. When I used a decent merge tool, it only found one conflict that it couldn't resolve automatically.

Elesbaan70 commented 2 years ago

I highly recommend P4Merge for when merges get messy. https://www.perforce.com/downloads/visual-merge-tool

Watch out with the installer, though. It will try to install their whole version control client, so you have to uncheck everything except the P4Merge option.

originalfoo commented 2 years ago

@Elesbaan70 pls merge this in to master (assuming you can now it's got 2+ approvals)

Elesbaan70 commented 2 years ago

I don't have write access