Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace CheckLane() with extensions #1461

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
+53 −61
Draft

Replace CheckLane() with extensions #1461

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aff2cad
Replace `CheckLane()` with extensions
originalfoo Mar 9, 2022
c92a1fa
Trivial xmldoc tweak
originalfoo Mar 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions TLM/TLM/Manager/Impl/SpeedLimitManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -712,9 +712,8 @@ public static bool IsValidRange(float speed) {

/// <summary>Private: Do not call from the outside.</summary>
private void SetLaneSpeedLimit(uint laneId, SetSpeedLimitAction action) {
if (!Flags.CheckLane(laneId)) {
if (!laneId.IsValidWithSegment())
return;
}

ushort segmentId = laneId.ToLane().m_segment;
ref NetSegment netSegment = ref segmentId.ToSegment();
Expand Down
60 changes: 7 additions & 53 deletions TLM/TLM/State/Flags.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,8 +275,8 @@ internal static bool RemoveLaneConnection(uint lane1Id, uint lane2Id, bool start
Log._Debug($"Flags.RemoveLaneConnection({lane1Id}, {lane2Id}, {startNode1}) called.");
}
#endif
bool lane1Valid = CheckLane(lane1Id);
bool lane2Valid = CheckLane(lane2Id);
bool lane1Valid = lane1Id.IsValidWithSegment();
bool lane2Valid = lane2Id.IsValidWithSegment();

bool ret = false;

Expand Down Expand Up @@ -338,7 +338,7 @@ internal static void RemoveLaneConnections(uint laneId, bool? startNode = null)
return;
}

bool laneValid = CheckLane(laneId);
bool laneValid = laneId.IsValidWithSegment();
bool clearBothSides = startNode == null || !laneValid;
#if DEBUG
if (debug) {
Expand Down Expand Up @@ -395,8 +395,8 @@ internal static void RemoveLaneConnections(uint laneId, bool? startNode = null)
/// <param name="startNode1"></param>
/// <returns></returns>
internal static bool AddLaneConnection(uint lane1Id, uint lane2Id, bool startNode1) {
bool lane1Valid = CheckLane(lane1Id);
bool lane2Valid = CheckLane(lane2Id);
bool lane1Valid = lane1Id.IsValidWithSegment();
bool lane2Valid = lane2Id.IsValidWithSegment();

if (!lane1Valid) {
// remove all incoming/outgoing lane connections
Expand Down Expand Up @@ -459,51 +459,16 @@ private static void CreateLaneConnection(uint sourceLaneId,
laneConnections[sourceLaneId][nodeArrayIndex][oldConnections.Length] = targetLaneId;
}

internal static bool CheckLane(uint laneId) {
// TODO refactor
if (laneId <= 0) {
return false;
}

ref NetLane netLane = ref laneId.ToLane();

if (((NetLane.Flags)netLane.m_flags & (NetLane.Flags.Created | NetLane.Flags.Deleted)) != NetLane.Flags.Created) {
return false;
}

ushort segmentId = netLane.m_segment;
if (segmentId <= 0) {
return false;
}

ref NetSegment netSegment = ref segmentId.ToSegment();

return (netSegment.m_flags & (NetSegment.Flags.Created | NetSegment.Flags.Deleted)) == NetSegment.Flags.Created;
}

public static void SetLaneAllowedVehicleTypes(uint laneId, ExtVehicleType vehicleTypes) {
if (laneId <= 0) {
if (!laneId.IsValidWithSegment())
return;
}

ref NetLane netLane = ref laneId.ToLane();

if (((NetLane.Flags)netLane.m_flags & (NetLane.Flags.Created | NetLane.Flags.Deleted)) != NetLane.Flags.Created) {
return;
}

ushort segmentId = netLane.m_segment;

if (segmentId <= 0) {
return;
}

ref NetSegment netSegment = ref segmentId.ToSegment();

if ((netSegment.m_flags & (NetSegment.Flags.Created | NetSegment.Flags.Deleted)) != NetSegment.Flags.Created) {
return;
}

NetInfo segmentInfo = netSegment.Info;
uint curLaneId = netSegment.m_lanes;
uint laneIndex = 0;
Expand All @@ -524,22 +489,11 @@ public static void SetLaneAllowedVehicleTypes(ushort segmentId,
uint laneId,
ExtVehicleType vehicleTypes)
{
if (segmentId <= 0 || laneId <= 0) {
if (!laneId.IsValidWithSegment())
return;
}

ref NetSegment netSegment = ref segmentId.ToSegment();

if ((netSegment.m_flags & (NetSegment.Flags.Created | NetSegment.Flags.Deleted)) != NetSegment.Flags.Created) {
return;
}

ref NetLane netLane = ref laneId.ToLane();

if (((NetLane.Flags)netLane.m_flags & (NetLane.Flags.Created | NetLane.Flags.Deleted)) != NetLane.Flags.Created) {
return;
}

NetInfo segmentInfo = netSegment.Info;

if (laneIndex >= segmentInfo.m_lanes.Length) {
Expand Down
38 changes: 34 additions & 4 deletions TLM/TLM/Util/Extensions/NetLaneExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,23 +7,53 @@ public static class NetLaneExtensions {
internal static ref NetLane ToLane(this uint laneId) => ref _laneBuffer[laneId];

/// <summary>
/// Checks if the netLane is Created, but not Deleted.
/// Checks <paramref name="netLane"/> is <c>Created</c> but not <c>Deleted</c>.
/// </summary>
/// <param name="netLane">netLane</param>
/// <returns>True if the netLane is valid, otherwise false.</returns>
/// <returns>Returns <c>true</c> if valid, otherwise <c>false</c>.</returns>
public static bool IsValid(this ref NetLane netLane) =>
((NetLane.Flags)netLane.m_flags).CheckFlags(
required: NetLane.Flags.Created,
forbidden: NetLane.Flags.Deleted);

/// <summary>
/// Checks if the netLane is Created, but not Deleted and if its netSegment is Created, but neither Collapsed nor Deleted.
/// Checks <paramref name="laneId"/> is not <c>0</c>,
/// then checks netLane is <c>Created</c> but not <c>Deleted</c>.
/// </summary>
/// <param name="laneId">The id of the lane to check.</param>
/// <returns>Returns <c>true</c> if valid, otherwise <c>false</c>.</returns>
public static bool IsValid(this uint laneId)
=> (laneId != 0) && laneId.ToLane().IsValid();

/// <summary>
/// Checks <paramref name="netLane"/> is <c>Created</c> but not <c>Deleted</c>,
/// then checks its netSegment is <c>Created</c> but not <c>Collapsed|Deleted</c>.
/// </summary>
/// <param name="netLane">netLane</param>
/// <returns>True if the netLane and its netSegment is valid, otherwise false.</returns>
/// <returns>Returns <c>true</c> if valid, otherwise <c>false</c>.</returns>
public static bool IsValidWithSegment(this ref NetLane netLane) {
return netLane.IsValid()
&& netLane.m_segment.ToSegment().IsValid();
}

/// <summary>
/// Checks <paramref name="laneId"/> is not <c>0</c>,
/// then checks netLane is <c>Created</c> but not <c>Deleted</c>,
/// then checks its segmentId is not <c>0</c>,
/// then checks its netSegment is <c>Created</c> but not <c>Collapsed|Deleted</c>.
/// </summary>
/// <param name="laneId">The id of the lane to check.</param>
/// <returns>Returns <c>true</c> if valid, otherwise <c>false</c>.</returns>
public static bool IsValidWithSegment(this uint laneId) {
Copy link
Collaborator

@kianzarrin kianzarrin Mar 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already have this in lane extensions:

        public static bool IsValidWithSegment(this ref NetLane netLane) {
            return netLane.IsValid()
                && netLane.m_segment.ToSegment().IsValid();
        }

~~But it does not check for laneId < 0

there is the minor issue that we some times check for laneId < 0 and sometimes don't based on the mood of the programmer at the time!~~

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait uint cannot be less than zero anyway so its fine.
I think we should use the old overload instead of introducing this new one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I know unsigned numbers can't be negative (no idea why so much old code has the <= check on ushort and uint).

But regardless, that's not why we check id != 0, is it?

if (laneId == 0)
return false;

ref NetLane netLane = ref laneId.ToLane();

if (!netLane.IsValid())
return false;

return netLane.m_segment.IsValid();
}
}
}
13 changes: 11 additions & 2 deletions TLM/TLM/Util/Extensions/NetSegmentExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,15 +44,24 @@ public static ushort GetTailNode(this ref NetSegment netSegment) {
}

/// <summary>
/// Checks if the netSegment is Created, but neither Collapsed nor Deleted.
/// Checks <paramref name="netSegment"/> is <c>Created</c> but not <c>Collapsed|Deleted</c>.
/// </summary>
/// <param name="netSegment">netSegment</param>
/// <returns>True if the netSegment is valid, otherwise false.</returns>
/// <returns>Returns <c>true</c> if valid, otherwise <c>false</c>.</returns>
public static bool IsValid(this ref NetSegment netSegment) =>
netSegment.m_flags.CheckFlags(
required: NetSegment.Flags.Created,
forbidden: NetSegment.Flags.Collapsed | NetSegment.Flags.Deleted);

/// <summary>
/// Checks <paramref name="segmentId"/> is not <c>0</c>,
/// then checks netSegment is <c>Created</c> but not <c>Collapsed|Deleted</c>.
/// </summary>
/// <param name="segmentId">The id of the segment to check.</param>
/// <returns>Returns <c>true</c> if valid, otherwise <c>false</c>.</returns>
public static bool IsValid(this ushort segmentId)
=> (segmentId != 0) && segmentId.ToSegment().IsValid();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checking segmentId != 0 is redundant because m_segments[0] holds the default value and therefore is not valid.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just use the old extension.
If checking segmentId != 0 is important - for example if you think someone can accidentally write something to m_segments[0] - then we should do it consistently everywhere.
But That is OCD in my opinion. the old interface would do.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're sure lane / segment zero will never have Created flag set, then yeah, we could ditch the id != 0 check.

@krzychu124 thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First element is always thrown away on load so it should have flag None unless other mod used it and then got serialized to the savegame (bug) (worth checking if they also skip first element or not)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it should be ok to ditch the id != 0 checks?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would check where CO is checking for != 0 -> they know where it is possible. We can only guess

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First element is always thrown away on load so it should have flag None unless other mod used it and then got serialized to the savegame (bug) (worth checking if they also skip first element or not)

If that is the case then we should always check for id != 0.
it would be inconsistent to have 2 extensions methods to check if valid and then we pick one or the other randomly.
In any case I consider this to be a low priority issue. I think its OK to merge the code like this but it might be worth improving it.

Copy link
Member Author

@originalfoo originalfoo Mar 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've realised there are some other issues with this PR, notably that an IsValid() extension of a uint / ushort has no way to determine what class those ids refer to, which is a huge flaw in my thinking of this PR. That being the case, we should always be doing .ToWhatever() first and then IsValid() - but is there any way to, for example, get the lane id from a NetLane instance in the IsValid() stage?

If we need to check != 0 how do we implement that in the least crufty/cumbersome manner?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was what I was afraid of (I should have communicated that better). I thin we should minimize the use of (interger).Extensions(). I think ToNode() or ToSegment() is fine but more than can cause trouble.

Copy link
Member Author

@originalfoo originalfoo Mar 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I was thinking to include id != 0 check in IsValid() is extensions such as uint.IsValidLane() and ushort.IsValidSegment(), example:

internal static bool IsValidLane(this uint laneId)
    => laneId != 0 && laneId.ToLane().IsValid();

// call site example:

if (!someLaneId.IsValidLane())
    return;


public static NetInfo.Lane GetLaneInfo(this ref NetSegment netSegment, int laneIndex) =>
netSegment.Info?.m_lanes?[laneIndex];

Expand Down