Cache the relationship between Lane ID (NetLane) and Lane Index (NetInfo.Lane)

[Elesbaan70]

Core Functionality

This adds the following method to IExtSegmentManager:

    uint GetLaneId(ushort segmentId, int laneIndex);

and adds a new IExtLaneManager with the following methods:

    bool GetSegmentAndIndex(uint laneId, out ushort segmentId, out int laneIndex);

    int GetLaneIndex(uint laneId);

    NetInfo.Lane GetLaneInfo(uint laneId);

Any existing code I could identify that did this work has been replaced with calls to the new methods. I imagine there are probably other places I didn't catch.

Added Bonus

Also, this adds a class under the Patch namespace, NetManagerEvents, which patches NetManager to provide simple high-level event notifications that can be consumed by managers ad hoc:

• ExtLaneManager uses it exclusively for monitoring network changes.
• The new functionality in ExtSegmentManager uses it for monitoring network changes.
• LaneConnectionManager has been updated to monitor the ReleasingSegment event instead of overriding HandleInvalidSegment.

The ReleasingSegment event will be better in many cases than using HandleInvalidSegment because:

1. It always occurs immediately as segments are becoming invalid, because it's patched into the right NetManager operation. It's just more reliable.
2. It occurs while the segment is still valid. Some HandleInvalidSegment overrides are acting on data that might not be in a valid state, which may lead to problems. NetManagerEvents.ReleasingSegment addresses that issue.

[originalfoo]

Would the Managers/AbstractGeometryObservingManager benefit from any of the NetManager event patches you added - or even be made obsolete? (I've not checked yet, just random thought on quick glance)

[originalfoo]

Probably outside scope of this PR, but...

When a segment change is detected, could we iterate the lanes and compile:

• laneInfo.m_laneType (| the flags)
• laneInfo.m_vehicleType (| the flags)
• laneInfo.m_finalDirection - (count the directions - ignoring None; and | the flags)

With that lane summary stored on an ExtSegment we could very quickly check if a segment has lanes suitable for specific TMPE tool, especially in overlays code, etc. Things like GetSortedLanes() could first check if the segment has required lane/vehicle types, direction, etc. before having to do any iteration (which itself will be much easier now thanks to existing stuff in this PR).

Maybe have extensions like netSegment.Matches(laneType?, vehicleType?, direction?), etc? As an example of where it would be useful see MayHaveCustomSpeedLimits() extension which could be condensed to:

public static bool MayHaveCustomSpeedLimits(this ref NetSegment segment) =>
    segment.IsValid() &&
    segment.Directions != 0 && // extension that gets the compiled direction flags
    segment.Matches(
        laneTypes: SpeedLimitManager.LANE_TYPES,
        vehicleTypes: SpeedLimitManager.VEHICLE_TYPES);

The original is checking service/subservice however that can be problematic as there's often decorative networks in workshop that are using the service/subservice that TMPE is interested in so we get false positives when checking at the segment level.

We could probably merge that stuff in to a dedicate 'quick check' extension something like:

public static bool IsApplicable(
    this ref NetSegment segment,
    NetInfo.LaneType? laneTypes = null,
    VehicleInfo.VehicleType? vehicleTypes = null) {

    if (!segment.m_flags.CheckFlags(
        required: NetSegment.Flags.Created,
        forbidden: NetSegment.Flags.Collapsed | NetSegment.Flags.Deleted))
            return false;

    var extSeg = ...

    return
        extSeg.Directions != 0 &&
        (laneTypes == null || (extSeg.laneTypes & laneTypes) != 0) &&
        (vehicleTypes == null || (extSeg.vehicleTypes & vehicleTypes) != 0);
}

Then we can quick check if a segment is relevant to a TMPE tool:

if (someSegment.IsApplicable(someTool.LANE_TYPES, someTool.VEHICLE_TYPES) {
   ...
}

If there's an interface that ensures LANE_TYPES and VEHICLE_TYPES are defined on a class, we could also do something joyful along the lines of...

// elswhere
if (someSegment.IsApplicableTo(SpeedLimitManager) ...

[Elesbaan70]

Would the Managers/AbstractGeometryObservingManager benefit from any of the NetManager event patches you added - or even be made obsolete?

It is possible long-term, and in my opinion that wouldn't be a bad thing. I dislike the whole paradigm of infrastructure code knowing about all of its consumers.

But AbstractGeometryObservingManager does a lot more than NetManagerEvents, and even where their functionality seems to overlap, there might be use cases that the existing events don't cover. So this would need to be treated as a separate project where these questions are examined carefully.

I created NetManagerEvents because I needed lane events and because I was concerned about what I saw in LaneConnectionManager. (I don't remember the specifics, but I believe it makes assumptions that require the segment to still be valid.) I think it's best to take a minimalist approach, shifting over to this paradigm only as the need arises.

[kianzarrin]

Can we have a single threaded benchmark? there is a benchmark project.

[kianzarrin]

might be better to call this extSegment. but this is OK too.

[kianzarrin]

maybe rename to OnReleasingSegment()?

[kianzarrin]

on the second thought this is fine too.
https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/names-of-type-members#names-of-events

CS uses the On... convention.
Notifier uses events that begin with On. also we have OnSegmentChange/Modified/Replacement and several other events like this.
In other places we don't use the On... naming convention. So its a mixed bag so far.

[Elesbaan70]

In addition to the naming convention on the page you linked to, Microsoft's convention, at least as implemented in places like WinForms, is that "On..." is used on virtual methods that are called by event handlers. So basically, it's a second layer of event handling within the context of a specific class that monitors an event.

[kianzarrin]

[Note]
maybe you can create this attribute:

    public class HarmonyPatch2 : HarmonyLib.HarmonyPatch {
        public HarmonyPatch2(Type delcaringType, Type delegateType, bool instance = false){
            info.declaringType = delcaringType;
            info.methodName = delegateType.Name;
            var args = delegateType.GetMethod("Invoke").GetParameters().Select(p => p.ParameterType); // or use what we have in transpiler utils.
            if (instance) args = args.Skip(1); // skip arg0 because its instance method.
            info.argumentTypes = args.ToArray();
        }
    }

And then use it like this:

private delegate void PreReleaseSegmentImplementation(ushort segment, ref NetSegment data, bool keepNodes);

[HarmonyPatch2(typeof(NetManager), typeof(PreReleaseSegmentImplementation))]

The advantage of doing it this way is that you are less likely to make mistake copy pasting the function from dnSpy.

[Elesbaan70]

I wasn't even using dnSpy. I did this all from within Visual Studio. But I'm still enough of a novice with Harmony that I'd rather wait to do things like this until I have a better handle on the big picture of how it works.

[kianzarrin]

[lower priority] simple benchmark.

[kianzarrin]

I am not sure if benchmark is possible with Unity objects like NetInfo. So I am approving this as it is. if you managed to do any speed comparison between the current code and the previous lane util code that would be nice.

[Elesbaan70]

Where are we at on this? It would probably be good to get it merged soonish if it's going to be accepted, since the conflicts with #354 are starting to get messy.

[originalfoo]

Is this supposed to be in the API?

[Elesbaan70]

I don't want it to be that way, but that's how ExtSegmentManager is written. As much as I'd like to secure my reputation as the API killer, I'd rather not change that unless forced to as I basically was in the traffic light stuff.

[Elesbaan70]

It's not really an API. It's exactly the same mess as the traffic light stuff.

@krzychu124 do you have an opinion on this? The nice thing in this case is, it's a much smaller problem, and easier to define, than the traffic light stuff. If we did want to clean it up, we could make sure there's new API in place to do whatever people might have needed with it.

[krzychu124]

I would leave it like it is. For now nothing knows about its existence and before it will go to Stable we might have few chances to throw it away from there 😄

[krzychu124]

Code looks good, I'm not sure yet if should go before or after #1479
Both affect lane connector so could be released to Test at the same time.
I think it might be easier to merge it after since this one does not change logic, just replaces calls.
@kianzarrin any thoughts in which order it could be easier to proceed if we want to move forward?

[kianzarrin]

I think #1479 is more complicated and harder to fix conflicts. So after #1479 is good idea.