Junction restrictions cleanup

Elesbaan70 commented 2 years ago

This is a preparatory change related to #1502. Junction restrictions will be the example of the best way to implement the new persistence strategy. (Traffic lights have to take a sub-optimum approach because of some design issues in that code.)

I'm making this a separate PR because just cleaning it up is enough work that everyone will want to review that on its own.

This seems to remove existing API, but it really doesn't, because the "API" structures that it removes were not available through any API methods.

Dev Tasks

[x] Move internal junction restriction structures out of the API project
[x] Eliminate TernaryBool from the internal implementation, replace with bit masking
[x] Eliminate data layer redundancies

Test Tasks

[x] RM Crossings
[x] Roundabout Builder
[x] Adaptive Networks
[x] Node Controller 2
[x] Node Controller Renewal

krzychu124 commented 2 years ago

@kianzarrin How this change will affect your NodeController mod?

kianzarrin commented 2 years ago

I think it has to be tested with NC2/NCR

Elesbaan70 commented 2 years ago

I've tested it with RM Crossings.

How do NC2/NCR use this?

Is AN affected? Or does it just use IJunctionRestrictionsManager for this?

kianzarrin commented 2 years ago

How do NC2/NCR use this?

when changing node to U-turn, crossing, stretch then traffic rules are not forced but rather are default rules are changed (this is what values revert to when you press delete).

This is done by patching TMPE functions specially ternary bool.

Type:	Uturn	Corssing	Stretch	Stretch
rule	Uturn	Crossing	enter blocked junction	Crossing
Default	Yes	Yes	Yes	No
Configurable	No	No	No	No

Elesbaan70 commented 2 years ago

I'll test this in a day or two.

Assuming I haven't broken anything so far, I'm going to drop the remaining task from this PR, since it's not strictly necessary for the persistence changes. But I'm going to come back to this later and propose an API-based change that can be guaranteed stable regardless of what happens to the implementation.

Patching is for unauthorized/unsupported interference in a program's intended behavior. That doesn't apply to this situation, so there are better ways of doing it.

Elesbaan70 commented 2 years ago

@kianzarrin I've tested NC2 and NCR. They still work as you described, except that U turns are configurable on U-turn nodes. This is how it should work, since you might want to allow U turns one way but not the other, so I'm guessing your (very helpful, thank you) table was in error on that point?

Elesbaan70 commented 2 years ago

I have tested AN with this change.

Elesbaan70 commented 2 years ago

I have removed the task to eliminate redundant logic in JunctionRestrictionsManager. After I've finished the persistence redesign, I will create an API-level hook that Node Controller can use. In conjunction with that, I will create pull requests for both Node Controller projects to use that hook, but those PRs will be drafts until the hook itself is released to Stable.

Elesbaan70 commented 2 years ago

I'm taking this out of Draft status, but please look at the list of tests on external dependencies which I have added to the original task list, and let me know if I missed anything.

kianzarrin commented 2 years ago

@kianzarrin I've tested NC2 and NCR. They still work as you described, except that U turns are configurable on U-turn nodes. This is how it should work, since you might want to allow U turns one way but not the other, so I'm guessing your (very helpful, thank you) table was in error on that point?

Yes maybe error.

I am glad that it works :)

kianzarrin commented 2 years ago

how did you test tough?

Elesbaan70 commented 2 years ago

how did you test tough?

RM Crossings

Verified that crosswalks disappear when pedestrian crossing is disabled.

Roundabout Builder

Depended on aubergine's sign-off.

Adaptive Networks

In the asset editor, added a prop that depends on the Zebra Crossing flag, and verified that it behaves as expected.

Node Controller

Fully verified the configurable status for each relevant node type.
Verified the default for pedestrian crossing and u-turn.
Enter Blocked Junction will need to be tested using live traffic, or logging could be used to verify the default value. In any case, my confidence is very high because it uses the same mechanism as the others, and there's nothing really special about it.

kianzarrin commented 2 years ago

Verified the default for pedestrian crossing and u-turn.

how? it does not show the sprites if its not configurable.

kianzarrin commented 2 years ago

I find the AI a bit confusing. it makes it more clear for me if Get* was renamed to Calculate*. but that would break NC.

I checked the NC patches and its only patching Get* methods which you have not changed. so it should work just fine.

Elesbaan70 commented 2 years ago

how? it does not show the sprites if its not configurable.

U turns are configurable on a u-turn node. Since that behavior makes sense to me, that's why I asked if your table was in error.

The pedestrian crossing state is self-evident if RM Crossings is enabled.

Elesbaan70 commented 2 years ago

I just realized that AN flags can be used to verify the default state of all the restrictions of concern. I've unchecked the Node Controller testing tasks and will do a more complete test.

kianzarrin commented 2 years ago

I am sure its all fine. you haven't changed any of the Get* stuff.

Elesbaan70 commented 2 years ago

Enter Blocked Junction is fully tested. This is ready to be merged.

Elesbaan70 commented 2 years ago

@krzychu124 I don't have high enough privileges here to even specifically request reviews. Any way that can be addressed?

CitiesSkylinesMods / TMPE