Junction restrictions hook #1579

Elesbaan70 · 2022-05-22T23:20:13Z

Resolves #902

An IJunctionRestrictionsHook API implementing the idea discussed in #1557.

I had originally planned make this change before beginning the optimization/reorg of JunctionRestrictionsManager, but it turned out that I had to do most of that work in conjunction with this. But all the redundancies can't finally be eliminated until the patches are gone from the stable builds of NC2 and NCR.

Relevant PRs for the Node Controller projects:
MacSergey/NodeController30#55
kianzarrin/NodeController#33

kianzarrin · 2022-05-23T00:26:25Z

TLM/TMPE.API/Manager/IJunctionRestrictionsManager.cs

@@ -574,5 +576,17 @@ public interface IJunctionRestrictionsManager {
        /// Updates the default values for all junction restrictions and segments.
        /// </summary>
        void UpdateAllDefaults();
+
+        event Action<FlagsChangedEventArgs> FlagsChanged;


what is this?

It's incomplete. It's an event that will be invoked when junction restrictions flags change on a segment end, but FlagsChangedEventArgs is missing some properties right now.

Great example of this being a work in progress. I've just created the draft pull request so there's visibility on what I'm doing. 🙂

To further elaborate, it's intended to be a clear and supported way for mods like AN to know when junction restrictions change. That way you don't have to detect changes based on assumptions about the implementation. Instead, you have a direct promise that this will always work.

mods like AN to know when junction restrictions change.

isn't that what notifier does?

mods like AN to know when junction restrictions change.

isn't that what notifier does?

Yes, but INotifier is limited in the amount of specific information it can share about the change. By adding specific events at the manager level, you can provide more useful information. And if implemented correctly, when no one is using them all they cost is a check for null.

I notifier does tell which manager caused the update. and it has space for manager specific even args. although the event args is just an object and do not have hard-coded type safety.

although the event args is just an object and do not have hard-coded type safety.

And that's the key difference. Manager-owned events put less responsibility on the consumer. Fewer opportunities for error.

kianzarrin · 2022-05-23T07:14:40Z

TLM/TMPE.API/Traffic/Enums/JunctionRestrictionFlags.cs

+        AllowForwardLaneChange = 1 << 3,
+        AllowEnterWhenBlocked = 1 << 4,
+        AllowPedestrianCrossing = 1 << 5,
+    }


I need to control TrafficLights too. not sure if it would fit here. segmnetId would be irrelevant.

maybe this could be a rule override hook?

maybe this could be a rule override hook?

I anticipate that in general, hooks would correspond to managers. There might be exceptions, but since that's the existing organization of the API, it's the pattern that makes sense.

I am patching Traffic lights too. and could be causing problems because my patch will break if we overload those methods. I can easily fix my patch but I don't know about NCR.

Traffic light manager does not seem to manage default value for traffic lights though. so that is a difference. I am only patching if traffic lights are configurable.

…with compatibility

Elesbaan70 · 2022-05-29T04:27:18Z

This is basically done now, but I want to get NCR working before taking it out of draft status.

Elesbaan70 · 2022-05-29T18:37:19Z

I had originally planned make this change before beginning the optimization/reorg of JunctionRestrictionsManager, but it turned out that I had to do most of that work in conjunction with this. But all the redundancies can't finally be eliminated until the patches are gone from the stable builds of NC2 and NCR.

krzychu124 · 2022-06-06T21:01:28Z

Solve conflicts and sync with master? 😅

Elesbaan70 · 2022-06-15T02:24:44Z

We need to determine the correct resolution to the duplication of @kianzarrin's JunctionRestrictionFlags and my JunctionRestrictionsFlags before this can be merged.

Elesbaan70 · 2022-06-18T05:01:17Z

The number of external mods patching junction restrictions is increasing. This really needs to get merged.

Elesbaan70 · 2022-06-19T15:40:13Z

The end objectives here were:

Simplify the Junction Restrictions API
Cache Junction Restrictions calculations to improve the efficiency of pathfinding

The ordinary way to do this would be to make a new cached API, and have the existing methods call the new ones. Unfortunately, this could not be done straightforwardly, because the API methods are being patched by Node Controller, and now also by Adaptive Networks. The conventional approach would leave the new API methods unaffected by the patches, breaking those mods.

Therefore, the following strategy is being followed:

Phase 1: Caching and Hooks API
1. Re-implement the patched API methods as privately implemented interface methods so that the patch is no longer altering the API entry points. These still call the existing patched methods so that the patches continue to work.
2. Create a caching layer between the new API and the patched methods.
3. Introduce a "hooks" API that will allow external mods to alter Junction Restrictions behavior without patching.
4. Release to stable.
Phase 2: External mods change over to the hooks API and release to stable.
Phase 3: We remove the extra patched method layer, simplifying and cleaning up the final implementation.

This PR represents Phase 1.

Elesbaan70 · 2022-06-19T15:55:36Z

Since most of the TMPE implementation was calling the API methods directly via JunctionRestrictionsManager instead of through its interface, they all had to be updated when the deprecated API was changed to privately implemented interface methods. This was necessary because the methods they were calling are now an uncached implementation detail, and are not guaranteed to work correctly if called from outside.

This means that a lot of files were changed. Following is a list of the files with functional changes. All other changes are refactoring and eliminating deprecated API calls.

kvakvs

I'm OK with this

krzychu124 · 2022-06-20T21:30:32Z

TLM/TLM/Network/Data/SegmentEndId.cs

+            StartNode = startNode;
+        }
+
+        public static bool operator ==(SegmentEndId x, SegmentEndId y) => x.Equals(y);


Hmm, no override Equals and GetHashCode?
It will generate garbage, not to mention it will be quite slow compared to version with the Equals

krzychu124 · 2022-06-21T22:22:03Z

TLM/TLM/Manager/Impl/JunctionRestrictionsManager.cs

+            => segmentRestrictions[segmentId].GetValue(segmentId.AtNode(startNode), flags);
+
+        public bool GetValueOrDefault(ushort segmentId, bool startNode, JunctionRestrictionFlags flags)
+            => segmentRestrictions[segmentId].GetValueOrDefault(segmentId.AtNode(startNode), flags);


I'm not sure if it's a good idea to do lazy recalculation, especially on "get" while that method might be called by multiple threads at the same time (main, simulation, or any of pathfinding threads (1+)).

I double that.

kianzarrin · 2022-06-23T09:39:57Z

TLM/TLM/Manager/Impl/JunctionRestrictionsManager.cs

            }

-            public bool IsDefault() {
+            private bool Set(SegmentEndId segmentEndId, JunctionRestrictionFlags flags, JunctionRestrictionFlags newValues, JunctionRestrictionFlags newMask) {


maybe rename flags to values?

kianzarrin · 2022-06-23T09:42:33Z

TLM/TLM/Manager/Impl/JunctionRestrictionsManager.cs

@@ -1368,70 +973,219 @@ private struct JunctionRestrictions {

            private JunctionRestrictionFlags defaults;



what is the difference between mask and valid? maybe a bit of doc?

Elesbaan70 added 2 commits May 5, 2022 10:26

Hook API definition

d6699b2

Fill in missing pieces

918ea2d

kianzarrin reviewed May 23, 2022

View reviewed changes

Elesbaan70 mentioned this pull request May 24, 2022

Api update #1577

Merged

Elesbaan70 added 17 commits May 26, 2022 07:46

No FlagsChanged event until all the redundant mess has been eliminated

7eaf0e4

Merge branch 'master' into junction-restrictions-hook

23a1294

rename "invalid" restrictions to "orphaned"

5461b9d

Rename antipatterned SegmentEndId class

1f91a91

SegmentEndId

5fad9ae

Implement to-be-deprecated APIs privately, eliminate TernaryBool

47fd73d

Remove a lot of redundant code

438bab0

FlagsAttribute missing from JunctionRestrictionFlags

a9cfa05

Lazy update of default values

3c139af

Cache configurable calculations

fca9d25

Updates to IJunctionRestrictionsManager

a230ca7

Clean up all the redudant setters

ed2dd2a

separate and condense deprecated methods

ef6aa5b

clean up some notifications

b00d0e7

Make as many deprecated methods as possible public/obsolete, to help …

2fac55b

…with compatibility

several things broken

46eb439

IJunctionRestrictionsHook implementation

211182b

Elesbaan70 added 2 commits May 29, 2022 01:28

If I don't rename this, every future developer will curse me

f0a6ba1

A couple of nasty bugs

4280116

Elesbaan70 marked this pull request as ready for review May 29, 2022 18:37

Elesbaan70 requested review from kianzarrin, kvakvs and krzychu124 May 29, 2022 18:38

kianzarrin assigned Elesbaan70 May 29, 2022

Merge branch 'master' in to junction-restrictions-hook

5570ad4

Elesbaan70 added the DO NOT MERGE YET Don't merge this PR, even if approved, until further notice label Jun 15, 2022

Elesbaan70 added 2 commits June 17, 2022 23:55

Reconcile dulicate enum

636d1e2

Merge branch 'CitiesSkylinesMods:master' into junction-restrictions-hook

485305a

Elesbaan70 added code cleanup Refactor code, remove old code, improve maintainability API API for external mods JUNCTION RESTRICTIONS Feature: Junction restrictions and removed DO NOT MERGE YET Don't merge this PR, even if approved, until further notice labels Jun 18, 2022

Elesbaan70 requested review from kianzarrin, kvakvs and krzychu124 and removed request for kvakvs, krzychu124 and kianzarrin June 18, 2022 15:17

kvakvs approved these changes Jun 20, 2022

View reviewed changes

krzychu124 reviewed Jun 21, 2022

View reviewed changes

kianzarrin reviewed Jun 23, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Junction restrictions hook #1579

Junction restrictions hook #1579

Elesbaan70 commented May 22, 2022 •

edited

kianzarrin May 23, 2022

Elesbaan70 May 23, 2022

Elesbaan70 May 23, 2022

kianzarrin May 23, 2022

Elesbaan70 May 24, 2022

kianzarrin May 24, 2022

Elesbaan70 May 24, 2022

kianzarrin May 23, 2022

kianzarrin May 23, 2022

Elesbaan70 May 24, 2022

kianzarrin May 24, 2022

Elesbaan70 commented May 29, 2022

Elesbaan70 commented May 29, 2022

krzychu124 commented Jun 6, 2022

Elesbaan70 commented Jun 15, 2022 •

edited

Elesbaan70 commented Jun 18, 2022

Elesbaan70 commented Jun 19, 2022 •

edited

Elesbaan70 commented Jun 19, 2022

kvakvs left a comment

krzychu124 Jun 20, 2022

krzychu124 Jun 21, 2022

kianzarrin Jun 21, 2022

kianzarrin Jun 23, 2022

kianzarrin Jun 23, 2022

		@@ -1368,70 +973,219 @@ private struct JunctionRestrictions {

		private JunctionRestrictionFlags defaults;

Junction restrictions hook #1579

Are you sure you want to change the base?

Junction restrictions hook #1579

Conversation

Elesbaan70 commented May 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Elesbaan70 commented May 29, 2022

Elesbaan70 commented May 29, 2022

krzychu124 commented Jun 6, 2022

Elesbaan70 commented Jun 15, 2022 • edited

Elesbaan70 commented Jun 18, 2022

Elesbaan70 commented Jun 19, 2022 • edited

Elesbaan70 commented Jun 19, 2022

API Declaration and Instantiation

JunctionRestrictionsManager changes

Rename a deprecated class to avoid confusion.

kvakvs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Elesbaan70 commented May 22, 2022 •

edited

Elesbaan70 commented Jun 15, 2022 •

edited

Elesbaan70 commented Jun 19, 2022 •

edited

`JunctionRestrictionsManager` changes