-
Notifications
You must be signed in to change notification settings - Fork 85
Junction restrictions hook #1579
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.
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
base: master
Are you sure you want to change the base?
Changes from 2 commits
d6699b2
918ea2d
7eaf0e4
23a1294
5461b9d
1f91a91
5fad9ae
47fd73d
438bab0
a9cfa05
3c139af
fca9d25
a230ca7
ed2dd2a
ef6aa5b
b00d0e7
2fac55b
46eb439
211182b
f0a6ba1
4280116
5570ad4
636d1e2
485305a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Text; | ||
using TrafficManager.API.Traffic.Enums; | ||
|
||
namespace TrafficManager.API.Hook { | ||
public interface IJunctionRestrictionsHook { | ||
|
||
/// <summary> | ||
/// An event that allows a handler to modify the results of a <c>GetDefault<i>TrafficRule</i></c> method. | ||
/// </summary> | ||
event Action<FlagsHookArgs> GetDefaults; | ||
|
||
/// <summary> | ||
/// An event that allows a handler to modify the results of an <c>Is<i>TrafficRule</i>Configurable</c> method. | ||
/// </summary> | ||
event Action<FlagsHookArgs> GetConfigurable; | ||
|
||
/// <summary> | ||
/// Invalidates the specified flags so that they will be recalculated. | ||
/// Recalculation is not guaranteed to happen immediately, but is guaranteed to happen | ||
/// before their next use. | ||
/// </summary> | ||
/// <param name="flags"></param> | ||
public void InvalidateFlags(JunctionRestrictionFlags flags); | ||
|
||
public class FlagsHookArgs { | ||
|
||
/// <summary> | ||
/// Identifies the segment for which flag data is being returned. | ||
/// </summary> | ||
public ushort SegmentId { get; private set; } | ||
|
||
/// <summary> | ||
/// Identifies the node on the segment for which flag data is being returned. | ||
/// </summary> | ||
public bool StartNode { get; private set; } | ||
|
||
/// <summary> | ||
/// Identifies which flags are being returned. Unnecessary computation may be avoided | ||
/// by examining this mask to see which flags are being requested. | ||
/// </summary> | ||
public JunctionRestrictionFlags Mask { get; private set; } | ||
|
||
/// <summary> | ||
/// The flag return values. Changes to this property alter the outcome of the underlying operation. | ||
/// </summary> | ||
public JunctionRestrictionFlags Result { get; set; } | ||
|
||
internal FlagsHookArgs(ushort segmentId, bool startNode, JunctionRestrictionFlags mask, JunctionRestrictionFlags result) { | ||
SegmentId = segmentId; | ||
StartNode = startNode; | ||
Mask = mask; | ||
Result = result; | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
namespace TrafficManager.API.Traffic.Enums { | ||
public enum JunctionRestrictionFlags { | ||
AllowUTurn = 1 << 0, | ||
AllowNearTurnOnRed = 1 << 1, | ||
AllowFarTurnOnRed = 1 << 2, | ||
AllowForwardLaneChange = 1 << 3, | ||
AllowEnterWhenBlocked = 1 << 4, | ||
AllowPedestrianCrossing = 1 << 5, | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I need to control TrafficLights too. not sure if it would fit here. segmnetId would be irrelevant. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe this could be a rule override hook? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't that what notifier does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And that's the key difference. Manager-owned events put less responsibility on the consumer. Fewer opportunities for error.