Repository navigation
Fix crash on some cases of path groups. - #379
Conversation
As a result of looking again at the algorithm that we use to combine animated paths I decided it needed a lot more documentation and to be rewritten to make sure that we handle all cases.
| // Get enumerators for the key frames in each of the Animatable<PathGeometry>s. | ||
| var enumerators = pathData.Select(EnumerateKeyFrames).ToArray(); | ||
|
|
||
| // Start enumerating. |
There was a problem hiding this comment.
// Start enumerating. [](start = 12, length = 21)
Why are we skipping the first keyframe in each path? Is it because we don't need the starting values if the path is animated? #Closed
There was a problem hiding this comment.
We're not skipping anything. We are simply moving to the first item in the enumeration. Enumerators have to have MoveNext() called on them to get to the first item. If it didn't work that way there would be no way to handle empty enumerations.
In reply to: 518930487 [](ancestors = 518930487)
| for (var i = 0; i < enumerators.Length; i++) | ||
| { | ||
| var enumerator = enumerators[i]; | ||
| if (!enumerator.Current.isCompleted && |
There was a problem hiding this comment.
!enumerator.Current.isCompleted && [](start = 24, length = 34)
I don't think this is necessary - the enumerators will happily output forever. #Closed
| var frames = groupsByFrame.OrderBy(kvp => kvp.Key).Select(kvp => (frame: kvp.Key, geometries: kvp.Value)).ToArray(); | ||
| var previousGeometries = frames[0].geometries; | ||
| var success = true; | ||
| result = |
There was a problem hiding this comment.
result = [](start = 12, length = 8)
Add comment explaining what this assignment is doing. #Closed
| if (frameIndex > 0) | ||
| { | ||
| previousKeyFrame = records[frameIndex - 1].Geometries[pathIndex]; | ||
| } |
There was a problem hiding this comment.
Maybe add another comment here explaining why we only look back one frame for the "previous" key frame but might look ahead by multiple frames for the "next" one-- just for extra clarity. #Closed
|
|
||
| // Fill in the null with the value from the closest key frame. | ||
| records[frameIndex].Geometries[pathIndex] = | ||
| new KeyFrame<PathGeometry>(currentFrameNumber, closestKeyFrame.Value, HoldEasing.Instance); |
There was a problem hiding this comment.
HoldEasing.Instance [](start = 98, length = 19)
Should this be whatever the preferred easing of the record is instead? #Closed
There was a problem hiding this comment.
No, the easing doesn't matter here - the KeyFrame is just a convenient way to store a frame number an PathGeometry. When we create the Animatable we'll use the PreferredEasing only.
In reply to: 520098584 [](ancestors = 520098584)
| /// Takes a group of possibly-animated paths and returns an animatable | ||
| /// of PathGeometryGroups. Returns true if it succeeds without issues. | ||
| /// Even if false is returned a best-effort animatable is returned.</summary> | ||
| /// <returns><c>true</c> iff the grouping worked perfectly.</returns> |
There was a problem hiding this comment.
Do you think it's worth it to add something about returning false in the return section? Not sure how likely it would be that someone would read only the return section and not the summary, but worth considering. #Resolved
There was a problem hiding this comment.
| foreach (var (frame, geometries) in frames.Skip(1)) | ||
| /// <summary> | ||
| /// From a list of animatable paths, produce a record for each distinct key frame number. | ||
| /// Each record consists of the frame number a list of geometries for that frame number |
There was a problem hiding this comment.
number [](start = 46, length = 6)
number, #Closed
| success = false; | ||
| indices[i] = -1; | ||
| } | ||
| } |
There was a problem hiding this comment.
There was a problem hiding this comment.
| /// of PathGeometryGroups. Returns true if it succeeds without issues. | ||
| /// Even if false is returned a best-effort animatable is returned.</summary> | ||
| /// <returns><c>true</c> iff the grouping worked perfectly.</returns> | ||
| internal static bool TryGroupPaths( |
There was a problem hiding this comment.
TryGroupPaths [](start = 29, length = 13)
The naming here threw me until I read the comments, expected null with a false return to match other common "Try" methods. Not sure what the local idiom is, but if this isn't consistent consider a more suggestive name, maybe leave out the "Try"? Or make the bool an out param with a helpful name? #Closed
There was a problem hiding this comment.
| // Get the key frame for the current frame number. | ||
| var keyFrame = records[frameIndex].Geometries[pathIndex]; | ||
| if (keyFrame is null) | ||
| { |
There was a problem hiding this comment.
Consider moving this block into its own method, to make this one more readable. It's over three screens in my CodeFlow view. #Closed
ecb9169
As a result of looking again at the algorithm that we use to combine animated paths I decided it needed a lot more documentation and to be rewritten to make sure that we handle all cases.