CitiesSkylinesMods / TMPE

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

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

Closed Elesbaan70 closed 2 years ago

Elesbaan70 commented 2 years ago

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:

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 commented 2 years ago

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 commented 2 years ago
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: ```csharp 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: ```csharp 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: ```csharp 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... ```csharp // elswhere if (someSegment.IsApplicableTo(SpeedLimitManager) ... ```
Elesbaan70 commented 2 years ago

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 commented 2 years ago

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

Elesbaan70 commented 2 years ago

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.

kianzarrin commented 2 years ago

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