AdaptiveGraphRouting: add WeightModifier and EdgeInfo.Flags #920
Conversation
There was a problem hiding this comment.
Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @DmytroMuravskyi)
Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRouting.cs line 109 at r1 (raw file):
if (IsAffectedBy(v0.Point, v1.Point, userHints)) { if (modifierFactor < 1 + _grid.Tolerance)
coloring based on > or < 1 doesn't feel comprehensive enough given the new modifier factor scheme. could we have modifier factors also have a color? Create a property on the WeightModifier object for a color that can be controlled by a developer or let a default random color be assigned to the modifier. Whichever weither modifer "wins" should be the color of the edge.
Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRouting.cs line 466 at r1 (raw file):
{ hintFactor = Math.Min(l.Factor, hintFactor); flags &= l.Is2D ? EdgeFlags.Hint2D : EdgeFlags.Hint3D;
can we write a comment about what this is for?
Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 55 at r1 (raw file):
/// <summary> /// Create WeightModifier that sets factor to all edges lying on given plane.
phrasing "...that sets the factor on all edges lying on a given plane"
Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 61 at r1 (raw file):
/// <param name="factor">Factor of new WeightModifier.</param> /// <returns>Created WeightModifier.</returns> public WeightModifier AddPlaneModifier(string name, Plane plane, double factor)
rename to AddPlanarWeightModifier for clarity i think.
Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 74 at r1 (raw file):
return modifier; }
Just a question, we don't need to act now, but should Hint lines just be another kind of weight modifier, or at least use the WeightModifier infrastructure? They have a role to play in generating the graph but their weights could be managed as "AddLinerWeightModifiers" separately from the grid creation. Let's at least add a TODO nearby where HintLine weights are applied f you think we should move that direction.
Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 77 at r1 (raw file):
private double ModifierFactor(Vertex a, Vertex b) { double modifierFactor = -1;
-1 is a confusing value here, since we're looking for the "min" shouldn't we have this set to MaxValue? Or maybe better, have it be a double? so that we can return null if there is no modified value, and then just don't modify the value if a null is returned.
Elements/src/Spatial/AdaptiveGrid/EdgeInfo.cs line 31 at r1 (raw file):
if (Math.Abs(v0.Point.Z - v1.Point.Z) > grid.Tolerance) { Flags &= EdgeFlags.HasVerticalChange;
To a less experienced developer this is confusing, what does &= with an enum produce... ?
we need to comment here that this is a behavior of a "Flags" enum being treated as a bit set, ans so this is adding the flag to the set of stored flags.
Elements/src/Spatial/AdaptiveGrid/EdgeInfo.cs line 63 at r1 (raw file):
/// <summary> /// Check if edge info has certain flag or combination of flags set.
phrasing "...has a certain flag"
Elements/src/Spatial/AdaptiveGrid/EdgeInfo.cs line 65 at r1 (raw file):
/// Check if edge info has certain flag or combination of flags set. /// </summary> /// <param name="flag">Flag or combination of flags to check.</param>
how does one submit a combination of flags for this check. can we write teh code that would do that in this comment?
Elements/src/Spatial/AdaptiveGrid/EdgeInfo.cs line 74 at r1 (raw file):
/// <summary> /// Bit set of flags storing describing information about edge.
ok, Bit sets and flags attribute are cool but also new to me and unusual for our users I think. We'll need a short description of what is going on here, and
Elements/src/Spatial/AdaptiveGrid/EdgeInfo.cs line 77 at r1 (raw file):
/// </summary> [Flags] public enum EdgeFlags
separate file for enum.
Elements/src/Spatial/AdaptiveGrid/WeightModifier.cs line 8 at r1 (raw file):
{ /// <summary> /// Object that hold function that allows to apply factor multiplier to edge travel cost.
phrasing. "Object that lets you apply an edge weight factor to edges that meet a Condition filter function."
Elements/src/Spatial/AdaptiveGrid/WeightModifier.cs line 9 at r1 (raw file):
/// <summary> /// Object that hold function that allows to apply factor multiplier to edge travel cost. /// If edge passes check of several WeightModifier objects - lowest factor is chosen.
"If an edge meets the nameof(Condition) of several WeightModifier objects the lowest factor is chosen.
Elements/src/Spatial/AdaptiveGrid/WeightModifier.cs line 14 at r1 (raw file):
{ /// <summary> ///
fill in with "Basic constructor for a WeightModifier"
Elements/src/Spatial/AdaptiveGrid/WeightModifier.cs line 27 at r1 (raw file):
/// <summary> /// WeightModifier description.
replace "description" with "name"
Elements/src/Spatial/AdaptiveGrid/WeightModifier.cs line 32 at r1 (raw file):
/// <summary> /// Function, edge need to pass, can include additional data, captured by lambda.
"Filter function that the determines if this WeightModifier applies to an edge."
Elements/src/Spatial/AdaptiveGrid/WeightModifier.cs line 37 at r1 (raw file):
/// <summary> /// Multiplier number that is applied to edge traveling cost.
it's not a multiplier number, it's the final weight if this WeightModifier is the lowest right? So maybe "Weight to be applied according to this WeightModifier"
Elements/test/AdaptiveGraphRoutingTests.cs line 1086 at r1 (raw file):
var c = new RoutingConfiguration(turnCost: 1); var routing = new AdaptiveGraphRouting(grid, c); var func = new Func<Vertex, Vertex, bool>((a, b) =>
we should be testing the "AddPlaneModifer" method as well, maybe use that in both this case and the one above.
Elements/test/AdaptiveGraphRoutingTests.cs line 1256 at r1 (raw file):
var center = new Vector3(5, 5); //Add WeightModifier that wants you to travel through edges not to close and not to far.
phrasing "...through edges that are of medium length, between 1 and 3.5"
There was a problem hiding this comment.
Reviewed 7 of 7 files at r2, all commit messages.
Reviewable status:complete! 1 of 1 approvals obtained (waiting on @DmytroMuravskyi)
Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRouting.cs line 455 at r2 (raw file):
//TODO: consider unifying hint line, offset line and modifiers as single concept. //There still be functions for adding hint/offset lines but everything will be processes inside
phrasing. "There will still be functions for adding hint/offset lines but everything will be processed inside..."
Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRouting.cs line 456 at r2 (raw file):
//TODO: consider unifying hint line, offset line and modifiers as single concept. //There still be functions for adding hint/offset lines but everything will be processes inside //as WeightModifier. Would need to solve function that takes list of groups and define how
phrasing. "We would need to decide on the function that takes a list of these weight modifier groups and defines how multiple factors are combined together..."
Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 76 at r2 (raw file):
/// <summary> /// Check if edge passes any modifier check and return the lowest value among them.
"and returns" add 's'
Elements/src/Spatial/AdaptiveGrid/EdgeFlags.cs line 8 at r2 (raw file):
{ /// <summary> /// Bit set of flags storing describing information about edge.
remove "describing"
Elements/src/Spatial/AdaptiveGrid/EdgeFlags.cs line 12 at r2 (raw file):
/// Use | or |= to combine flags: flag = Hint2D | HasVerticalChange = 1 + 4 = 001 + 100 = 101 = 5. /// Use & or &= to check of one or more flags: flags & Hint3D == 101 & 010 == 0 == None, but /// but flags & Hint2D == 101 & 001 == 001 == Hint2D.
remove extra "but"
Elements/src/Spatial/AdaptiveGrid/EdgeInfo.cs line 66 at r2 (raw file):
/// </summary> /// <param name="flag">Flag or combination of flags to check. /// For example: HasFlag(Hint2D) or HasFlag(Hint2D | Hint3D).</param>
now it is "HasAnyFlag"
Elements/src/Spatial/AdaptiveGrid/WeightModifier.cs line 9 at r2 (raw file):
/// <summary> /// Object that lets you apply an edge weight factor to edges that meet a Condition filter function. /// If an edge meets the nameof(Condition) of several WeightModifier objects the lowest factor is chosen.
I had this wrong I think, does nameof work here? maybe we should be using https://stackoverflow.com/questions/54995797/can-you-use-nameof-or-another-technique-to-embed-a-procedure-name-in-a-code-co
Elements/src/Spatial/AdaptiveGrid/WeightModifier.cs line 32 at r2 (raw file):
/// <summary> /// Filter function that the determines if this WeightModifier applies to an edge.
remove "the" should be "that determines"
There was a problem hiding this comment.
Thanks for all the review process.
Reviewable status:
Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRouting.cs line 109 at r1 (raw file):
Previously, wynged (Eric Wassail) wrote…
coloring based on > or < 1 doesn't feel comprehensive enough given the new modifier factor scheme. could we have modifier factors also have a color? Create a property on the WeightModifier object for a color that can be controlled by a developer or let a default random color be assigned to the modifier. Whichever weither modifer "wins" should be the color of the edge.
Yes that's right, however I don't think we should introduce color into data model:
- It's hard to avoid color overlapping when to things are accidentally the same color.
- Since Edge can be affected by hint and modifier right now I wanted to distinguish hint edge and hint edge affected by modifier.
I was thinking about automatically painting edges in gradient from green to red, where yellow is factor of 1, less than that is green shifted and more than one is red shifted. What's about doing this as separate PR, maybe when we know what we want to do with edges affected by several modifiers?
Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRouting.cs line 466 at r1 (raw file):
Previously, wynged (Eric Wassail) wrote…
can we write a comment about what this is for?
Done.
Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRouting.cs line 455 at r2 (raw file):
Previously, wynged (Eric Wassail) wrote…
phrasing. "There will still be functions for adding hint/offset lines but everything will be processed inside..."
Done.
Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRouting.cs line 456 at r2 (raw file):
Previously, wynged (Eric Wassail) wrote…
phrasing. "We would need to decide on the function that takes a list of these weight modifier groups and defines how multiple factors are combined together..."
Done.
Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 55 at r1 (raw file):
Previously, wynged (Eric Wassail) wrote…
phrasing "...that sets the factor on all edges lying on a given plane"
Done.
Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 61 at r1 (raw file):
Previously, wynged (Eric Wassail) wrote…
rename to AddPlanarWeightModifier for clarity i think.
Done.
Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 74 at r1 (raw file):
Previously, wynged (Eric Wassail) wrote…
Just a question, we don't need to act now, but should Hint lines just be another kind of weight modifier, or at least use the WeightModifier infrastructure? They have a role to play in generating the graph but their weights could be managed as "AddLinerWeightModifiers" separately from the grid creation. Let's at least add a TODO nearby where HintLine weights are applied f you think we should move that direction.
Yes, we can do everything as WeightModifier, added a TODO for this.
Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 77 at r1 (raw file):
Previously, wynged (Eric Wassail) wrote…
-1 is a confusing value here, since we're looking for the "min" shouldn't we have this set to MaxValue? Or maybe better, have it be a
double?so that we can return null if there is no modified value, and then just don't modify the value if a null is returned.
This value is never returned, as 1 is returned if no modifiers are applied. But you are correct, using MaxValue makes everything much simpler.
Code quote:
double modifierFactor = -1Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 76 at r2 (raw file):
Previously, wynged (Eric Wassail) wrote…
"and returns" add 's'
Done.
Elements/src/Spatial/AdaptiveGrid/EdgeFlags.cs line 8 at r2 (raw file):
Previously, wynged (Eric Wassail) wrote…
remove "describing"
Done.
Code quote:
Bit set of flags storing describing information about edge.Elements/src/Spatial/AdaptiveGrid/EdgeFlags.cs line 12 at r2 (raw file):
Previously, wynged (Eric Wassail) wrote…
remove extra "but"
Done.
Code quote:
/// Use & or &= to check of one or more flags: flags & Hint3D == 101 & 010 == 0 == None, butElements/src/Spatial/AdaptiveGrid/EdgeInfo.cs line 31 at r1 (raw file):
has a certain flag
I don't think we should add it in constructor, but I've added few words to EdgeFlags documentation.
Elements/src/Spatial/AdaptiveGrid/EdgeInfo.cs line 63 at r1 (raw file):
Previously, wynged (Eric Wassail) wrote…
phrasing "...has a certain flag"
Done.
Elements/src/Spatial/AdaptiveGrid/EdgeInfo.cs line 65 at r1 (raw file):
Previously, wynged (Eric Wassail) wrote…
how does one submit a combination of flags for this check. can we write teh code that would do that in this comment?
Done and renamed it into HasAnyFlag
Elements/src/Spatial/AdaptiveGrid/EdgeInfo.cs line 74 at r1 (raw file):
Previously, wynged (Eric Wassail) wrote…
ok, Bit sets and flags attribute are cool but also new to me and unusual for our users I think. We'll need a short description of what is going on here, and
I've added AddFlags to EdgeInfo interface and have hidden Flags, so it's little bit more friendly. Also added EdgeInfoFlagsTest test.
Elements/src/Spatial/AdaptiveGrid/EdgeInfo.cs line 77 at r1 (raw file):
Previously, wynged (Eric Wassail) wrote…
separate file for enum.
Done.
Elements/src/Spatial/AdaptiveGrid/EdgeInfo.cs line 66 at r2 (raw file):
Previously, wynged (Eric Wassail) wrote…
now it is "HasAnyFlag"
Done.
Elements/src/Spatial/AdaptiveGrid/WeightModifier.cs line 8 at r1 (raw file):
Previously, wynged (Eric Wassail) wrote…
phrasing. "Object that lets you apply an edge weight factor to edges that meet a Condition filter function."
Done.
Elements/src/Spatial/AdaptiveGrid/WeightModifier.cs line 9 at r1 (raw file):
Previously, wynged (Eric Wassail) wrote…
"If an edge meets the nameof(Condition) of several WeightModifier objects the lowest factor is chosen.
Done.
Elements/src/Spatial/AdaptiveGrid/WeightModifier.cs line 14 at r1 (raw file):
Previously, wynged (Eric Wassail) wrote…
fill in with "Basic constructor for a WeightModifier"
Done.
Elements/src/Spatial/AdaptiveGrid/WeightModifier.cs line 27 at r1 (raw file):
Previously, wynged (Eric Wassail) wrote…
replace "description" with "name"
Done.
Elements/src/Spatial/AdaptiveGrid/WeightModifier.cs line 32 at r1 (raw file):
Previously, wynged (Eric Wassail) wrote…
"Filter function that the determines if this WeightModifier applies to an edge."
Done.
Code quote:
nction, edge need to pass, can include additional data, captured byElements/src/Spatial/AdaptiveGrid/WeightModifier.cs line 37 at r1 (raw file):
"Weight to be applied according to this WeightModifier"
Right now, two factors from WeightModifier are not combined, but it's still combined with factor originated from hint / offset lines.
Elements/src/Spatial/AdaptiveGrid/WeightModifier.cs line 9 at r2 (raw file):
Previously, wynged (Eric Wassail) wrote…
I had this wrong I think, does nameof work here? maybe we should be using https://stackoverflow.com/questions/54995797/can-you-use-nameof-or-another-technique-to-embed-a-procedure-name-in-a-code-co
I tried "cref" but it doesn't see parameter name of a function the class header documentation. Left is as a plain text.
Elements/src/Spatial/AdaptiveGrid/WeightModifier.cs line 32 at r2 (raw file):
Previously, wynged (Eric Wassail) wrote…
remove "the" should be "that determines"
Done.
Code quote:
ion that the determines if this WeightModifier Elements/test/AdaptiveGraphRoutingTests.cs line 1086 at r1 (raw file):
Previously, wynged (Eric Wassail) wrote…
we should be testing the "AddPlaneModifer" method as well, maybe use that in both this case and the one above.
Done.
Code quote:
var func = new Func<Vertex, Vertex, bool>((a, b) =>Elements/test/AdaptiveGraphRoutingTests.cs line 1256 at r1 (raw file):
Previously, wynged (Eric Wassail) wrote…
phrasing "...through edges that are of medium length, between 1 and 3.5"
Done.
There was a problem hiding this comment.
couple more comment changes before merge, but approved!
Reviewed 6 of 6 files at r3, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @DmytroMuravskyi)
Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRouting.cs line 109 at r1 (raw file):
Previously, DmytroMuravskyi wrote…
Yes that's right, however I don't think we should introduce color into data model:
- It's hard to avoid color overlapping when to things are accidentally the same color.
- Since Edge can be affected by hint and modifier right now I wanted to distinguish hint edge and hint edge affected by modifier.
I was thinking about automatically painting edges in gradient from green to red, where yellow is factor of 1, less than that is green shifted and more than one is red shifted. What's about doing this as separate PR, maybe when we know what we want to do with edges affected by several modifiers?
that's fine with me. let's handle this in a separate PR when we are more sure of what we want. I see you added the Factors to the edge weights below so this is fine for now.
Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 55 at r1 (raw file):
Previously, DmytroMuravskyi wrote…
Done.
missing the extra "the"
There was a problem hiding this comment.
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 55 at r1 (raw file):
Previously, wynged (Eric Wassail) wrote…
missing the extra "the"
Done.
BACKGROUND:
DESCRIPTION:
If edge passes check of several WeightModifier objects - lowest factor is chosen.
TESTING:
FUTURE WORK:
REQUIRED:
CHANGELOG.md.COMMENTS:
This change is