Remove flawed TTL API #1495
Remove flawed TTL API #1495
Conversation
… way to be sure."
… risk of impacting other mods, and offers little real benefit.
… conditionally compiled code.
| } else { | ||
| AddSelectedNode(HoveredNodeId); | ||
| if (ttlToolMode_ == TTLToolMode.ShowLights) { | ||
| SetToolMode(TTLToolMode.SelectNode); |
There was a problem hiding this comment.
[Note] I think TMPE policy is that using this is recommended. I don't care either way
There was a problem hiding this comment.
This was Visual Studio doing things I didn't ask it to. :-(
| offsetScreenPos.x + (modeWidth / 2f) + | ||
| (7f * zoom * (numInfos + 1)) + (infoWidth * numInfos), | ||
| offsetScreenPos.y - (infoHeight / 2f), | ||
| offsetScreenPos.x + modeWidth / 2f + |
There was a problem hiding this comment.
[Note]
I think your auto code cleanup has been setup to do some extra stuff.
I also think its .editorConfig fault for not explicitly handling this situations and leaving it to the person's IDE settings.
There was a problem hiding this comment.
it just means there is more code to review.
There was a problem hiding this comment.
and increases chance of conflict.
There was a problem hiding this comment.
Yes, I did not tell Visual Studio to do any of that, nor did I explicitly configure it to happen automatically. It's extremely frustrating. But I can go back and revert these if y'all believe it's worthwhile.
There was a problem hiding this comment.
No I don't think its worth while.
But maybe you can play with .editorconfig not to do this. But I think that's outside of scope of this PR
There was a problem hiding this comment.
for some reason you have to use text editor to modify values.
There was a problem hiding this comment.
LGTM. just renames.
@Elesbaan70 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? |
originalfoo
added
code cleanup
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. |
|
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 |
|
@Elesbaan70 pls update to latest master branch |
|
Ugh, @Elesbaan70 I messed up your PR with recent merged PR - 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. |
|
@Elesbaan70 I've done most of the conflicts from the |
Yep, I'll be on that this evening. |
|
@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. |
|
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. |
|
@Elesbaan70 pls merge this in to |
|
I don't have write access |
This is my solution to the problem of false and unmaintainable traffic light API, as discussed here: #967 (comment)
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.