Skip to content

Commit aab90e1

Browse files
Copilottig
andauthored
Fixes #4463 - Adds LineCanvas.GetRegion() - 10x performance regression in RenderLineCanvas (#4464)
* Initial plan * Revert xunit maxParallelThreads to default to fix performance issue Co-authored-by: tig <[email protected]> * Fix 10x performance regression in RenderLineCanvas Found via binary search of 51 commits that commit 0270183 introduced the slowdown by calling context.AddDrawnRectangle() for every cell in the LineCanvas (thousands of times per frame). Fixed by reporting the entire LineCanvas.Bounds once instead of per-cell. Test results: - Before: 1m 33s (10x slower than v2_release) - After: 9s (same as v2_release) Fixes the performance issue in AllViews_Draw_Does_Not_Layout test. Co-authored-by: tig <[email protected]> * Add comment clarifying LineCanvas coordinate system Addresses code review feedback about coordinate system conversion. LineCanvas.Bounds is already in screen-relative coordinates since cells are stored with screen positions (used with Driver.Move). Co-authored-by: tig <[email protected]> * Build non-rectangular Region from LineCanvas cells efficiently Instead of reporting entire LineCanvas.Bounds (which includes empty space), efficiently build a Region that encompasses only the actual line cells. Algorithm groups contiguous horizontal cells into spans per row, avoiding per-cell overhead while accurately representing the non-rectangular shape. Performance maintained: 10s (same as v2_release) All tests pass: 5695 drawing tests, 200 LineCanvas tests, 33 transparency tests Co-authored-by: tig <[email protected]> * Address code review feedback - improve robustness - Remove redundant empty check (caller already verifies) - Add safety check for empty row groups to prevent IndexOutOfRangeException - Update documentation to clarify precondition Tests still pass with 9s performance. Co-authored-by: tig <[email protected]> * Optimize BuildRegionFromLineCells - eliminate redundant sort Remove redundant OrderBy since X values are already sorted from the initial OrderBy().ThenBy() operation. Also remove unreachable empty check since GroupBy guarantees non-empty groups. Performance maintained at 9s. Co-authored-by: tig <[email protected]> * Final code review fixes - improve comments and add defensive check - Clarify comment about sorting behavior - Add defensive empty check for edge cases - Performance maintained at 10s Co-authored-by: tig <[email protected]> * Move BuildRegionFromCells to LineCanvas class Moved the BuildRegionFromCells method from View.Drawing.cs to LineCanvas where it logically belongs as a public static method. Updated View.Drawing.cs to call Drawing.LineCanvas.BuildRegionFromCells. Performance maintained at 9s. All tests pass. Co-authored-by: tig <[email protected]> * Update documentation for BuildRegionFromCells - Clarify that method handles empty cellMap gracefully - Improve comment explaining the defensive empty check - Performance maintained at 10s Co-authored-by: tig <[email protected]> * Rename BuildRegionFromCells to GetRegion Renamed the method as requested to have a more concise name. Performance maintained at 10s. All tests pass. Co-authored-by: tig <[email protected]> * Add GetCellMapWithRegion optimization for single-pass iteration Implemented optimized method that builds both cell map and Region in a single pass through the canvas bounds, eliminating the need for separate GetCellMap() and GetRegion() calls. Benefits: - Reduces from 2 iterations to 1 (GetCellMap then GetRegion -> single pass) - Eliminates sorting overhead (O(n log n) -> O(n)) - X values already sorted due to inner loop order - Builds Region incrementally per row as cells are discovered Performance maintained at 10s. All tests pass: - AllViews_Draw_Does_Not_Layout: 10s - 5695 drawing tests passed - 200 LineCanvas tests passed - 33 transparency tests passed Co-authored-by: tig <[email protected]> * Add comprehensive unit tests for GetRegion and GetCellMapWithRegion Added 15 new unit tests covering: GetRegion tests: - Empty cell map handling - Single cell regions - Horizontal and vertical lines - L-shaped regions - Discontiguous cells creating separate spans - Intersecting lines GetCellMapWithRegion tests: - Empty canvas handling - Single horizontal/vertical lines - Intersecting lines with proper intersection handling - Complex shapes (boxes) - Consistency with separate GetCellMap + GetRegion calls - Negative coordinate handling - Exclusion region support All 215 LineCanvas tests pass. Performance maintained at 9s. Co-authored-by: tig <[email protected]> * Fix code style violations per CONTRIBUTING.md - Replace var with explicit types (Dictionary<Point, Cell?>, LineCanvas, Region, etc.) - Replace new TypeName() with target-typed new () where type is declared - Fix foreach loop to use explicit IGrouping<int, Point> type - Use StraightLine instead of var in foreach All style violations fixed per CONTRIBUTING.md coding conventions. Tests still pass: 215 LineCanvas tests, 9s performance maintained. Co-authored-by: tig <[email protected]> * Code cleanup. * Upgrades Transparent Scenario to demonstrate LineCanvas transparency. "dotnet" is drawn using LineCanvas. * Comment out debug assertions in View drawing logic The debug assertions in the `View` class, which validated the relationships between `Margin`, `Border`, `Padding`, and their parent objects, as well as their `SubViewNeedsDraw` and `NeedsDraw` states, have been commented out. These assertions were conditionally executed when the current object and its `SuperView` were not of type `Adornment`. The code has been retained as comments for potential future reference or debugging purposes, but the assertions are no longer active in the current implementation. --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: tig <[email protected]> Co-authored-by: Tig <[email protected]>
1 parent 3ae28bb commit aab90e1

File tree

5 files changed

+658
-69
lines changed

5 files changed

+658
-69
lines changed

Examples/UICatalog/Scenarios/Transparent.cs

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,11 +219,102 @@ protected override bool OnDrawingContent (DrawContext? context)
219219
return false;
220220
}
221221

222+
protected override bool OnRenderingLineCanvas ()
223+
{
224+
// Draw "dotnet" using LineCanvas
225+
Point screenPos = ViewportToScreen (new Point (7, 16));
226+
DrawDotnet (LineCanvas, screenPos.X, screenPos.Y, LineStyle.Single, GetAttributeForRole (VisualRole.Normal));
227+
228+
return false;
229+
}
230+
222231
/// <inheritdoc />
223232
protected override bool OnClearingViewport () { return false; }
224233

225234
/// <inheritdoc />
226235
protected override bool OnMouseEvent (MouseEventArgs mouseEvent) { return false; }
236+
237+
238+
/// <summary>
239+
/// Draws "dotnet" text using LineCanvas. The 'd' is 8 cells high.
240+
/// </summary>
241+
/// <param name="canvas">The LineCanvas to draw on</param>
242+
/// <param name="x">Starting X position</param>
243+
/// <param name="y">Starting Y position</param>
244+
/// <param name="style">Line style to use</param>
245+
/// <param name="attribute">Optional attribute for the lines</param>
246+
private void DrawDotnet (LineCanvas canvas, int x, int y, LineStyle style = LineStyle.Single, Attribute? attribute = null)
247+
{
248+
int currentX = x;
249+
int letterHeight = 8;
250+
int letterSpacing = 2;
251+
252+
// Letter 'd' - lowercase, height 8
253+
// Vertical stem on right (goes up full 8 cells)
254+
canvas.AddLine (new (currentX + 3, y), letterHeight, Orientation.Vertical, style, attribute);
255+
// Top horizontal
256+
canvas.AddLine (new (currentX, y + 3), 4, Orientation.Horizontal, style, attribute);
257+
// Left vertical (only bottom 5 cells, leaving top 3 for ascender space)
258+
canvas.AddLine (new (currentX, y + 3), 5, Orientation.Vertical, style, attribute);
259+
// Bottom horizontal
260+
canvas.AddLine (new (currentX, y + 7), 4, Orientation.Horizontal, style, attribute);
261+
currentX += 4 + letterSpacing;
262+
263+
// Letter 'o' - height 5 (x-height)
264+
int oY = y + 3; // Align with x-height (leaving 3 cells for ascenders)
265+
// Top
266+
canvas.AddLine (new (currentX, oY), 4, Orientation.Horizontal, style, attribute);
267+
// Left
268+
canvas.AddLine (new (currentX, oY), 5, Orientation.Vertical, style, attribute);
269+
// Right
270+
canvas.AddLine (new (currentX + 3, oY), 5, Orientation.Vertical, style, attribute);
271+
// Bottom
272+
canvas.AddLine (new (currentX, oY + 4), 4, Orientation.Horizontal, style, attribute);
273+
currentX += 4 + letterSpacing;
274+
275+
// Letter 't' - height 7 (has ascender above x-height)
276+
int tY = y + 1; // Starts 1 cell above x-height
277+
// Vertical stem
278+
canvas.AddLine (new (currentX + 1, tY), 7, Orientation.Vertical, style, attribute);
279+
// Top cross bar (at x-height)
280+
canvas.AddLine (new (currentX, tY + 2), 3, Orientation.Horizontal, style, attribute);
281+
// Bottom horizontal (foot)
282+
canvas.AddLine (new (currentX + 1, tY + 6), 2, Orientation.Horizontal, style, attribute);
283+
currentX += 3 + letterSpacing;
284+
285+
// Letter 'n' - height 5 (x-height)
286+
int nY = y + 3;
287+
// Left vertical
288+
canvas.AddLine (new (currentX, nY), 5, Orientation.Vertical, style, attribute);
289+
// Top horizontal
290+
canvas.AddLine (new (currentX + 1, nY), 3, Orientation.Horizontal, style, attribute);
291+
// Right vertical
292+
canvas.AddLine (new (currentX + 3, nY), 5, Orientation.Vertical, style, attribute);
293+
currentX += 4 + letterSpacing;
294+
295+
// Letter 'e' - height 5 (x-height)
296+
int eY = y + 3;
297+
// Top
298+
canvas.AddLine (new (currentX, eY), 4, Orientation.Horizontal, style, attribute);
299+
// Left
300+
canvas.AddLine (new (currentX, eY), 5, Orientation.Vertical, style, attribute);
301+
// Right
302+
canvas.AddLine (new (currentX + 3, eY), 3, Orientation.Vertical, style, attribute);
303+
// Middle horizontal bar
304+
canvas.AddLine (new (currentX, eY + 2), 4, Orientation.Horizontal, style, attribute);
305+
// Bottom
306+
canvas.AddLine (new (currentX, eY + 4), 4, Orientation.Horizontal, style, attribute);
307+
currentX += 4 + letterSpacing;
308+
309+
// Letter 't' - height 7 (has ascender above x-height) - second 't'
310+
int t2Y = y + 1;
311+
// Vertical stem
312+
canvas.AddLine (new (currentX + 1, t2Y), 7, Orientation.Vertical, style, attribute);
313+
// Top cross bar (at x-height)
314+
canvas.AddLine (new (currentX, t2Y + 2), 3, Orientation.Horizontal, style, attribute);
315+
// Bottom horizontal (foot)
316+
canvas.AddLine (new (currentX + 1, t2Y + 6), 2, Orientation.Horizontal, style, attribute);
317+
}
227318
}
228319

229320
}

Terminal.Gui/Drawing/LineCanvas/LineCanvas.cs

Lines changed: 151 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,7 @@ public void Clear ()
179179
}
180180
}
181181
// Safe as long as the list is not modified while the span is in use.
182-
ReadOnlySpan<IntersectionDefinition> intersects = CollectionsMarshal.AsSpan(intersectionsBufferList);
182+
ReadOnlySpan<IntersectionDefinition> intersects = CollectionsMarshal.AsSpan (intersectionsBufferList);
183183
Cell? cell = GetCellForIntersects (intersects);
184184
// TODO: Can we skip the whole nested looping if _exclusionRegion is null?
185185
if (cell is { } && _exclusionRegion?.Contains (x, y) is null or false)
@@ -192,6 +192,136 @@ public void Clear ()
192192
return map;
193193
}
194194

195+
/// <summary>
196+
/// Evaluates the lines and returns both the cell map and a Region encompassing the drawn cells.
197+
/// This is more efficient than calling <see cref="GetCellMap"/> and <see cref="GetRegion"/> separately
198+
/// as it builds both in a single pass through the canvas bounds.
199+
/// </summary>
200+
/// <returns>A tuple containing the cell map and the Region of drawn cells</returns>
201+
public (Dictionary<Point, Cell?> CellMap, Region Region) GetCellMapWithRegion ()
202+
{
203+
Dictionary<Point, Cell?> map = new ();
204+
Region region = new ();
205+
206+
List<IntersectionDefinition> intersectionsBufferList = [];
207+
List<int> rowXValues = [];
208+
209+
// walk through each pixel of the bitmap, row by row
210+
for (int y = Bounds.Y; y < Bounds.Y + Bounds.Height; y++)
211+
{
212+
rowXValues.Clear ();
213+
214+
for (int x = Bounds.X; x < Bounds.X + Bounds.Width; x++)
215+
{
216+
intersectionsBufferList.Clear ();
217+
foreach (StraightLine line in _lines)
218+
{
219+
if (line.Intersects (x, y) is { } intersect)
220+
{
221+
intersectionsBufferList.Add (intersect);
222+
}
223+
}
224+
// Safe as long as the list is not modified while the span is in use.
225+
ReadOnlySpan<IntersectionDefinition> intersects = CollectionsMarshal.AsSpan (intersectionsBufferList);
226+
Cell? cell = GetCellForIntersects (intersects);
227+
228+
if (cell is { } && _exclusionRegion?.Contains (x, y) is null or false)
229+
{
230+
map.Add (new (x, y), cell);
231+
rowXValues.Add (x);
232+
}
233+
}
234+
235+
// Build Region spans for this completed row
236+
if (rowXValues.Count <= 0)
237+
{
238+
continue;
239+
}
240+
241+
// X values are already sorted (inner loop iterates x in order)
242+
int spanStart = rowXValues [0];
243+
int spanEnd = rowXValues [0];
244+
245+
for (int i = 1; i < rowXValues.Count; i++)
246+
{
247+
if (rowXValues [i] == spanEnd + 1)
248+
{
249+
// Continue the span
250+
spanEnd = rowXValues [i];
251+
}
252+
else
253+
{
254+
// End the current span and add it to the region
255+
region.Combine (new Rectangle (spanStart, y, spanEnd - spanStart + 1, 1), RegionOp.Union);
256+
spanStart = rowXValues [i];
257+
spanEnd = rowXValues [i];
258+
}
259+
}
260+
261+
// Add the final span for this row
262+
region.Combine (new Rectangle (spanStart, y, spanEnd - spanStart + 1, 1), RegionOp.Union);
263+
}
264+
265+
return (map, region);
266+
}
267+
268+
/// <summary>
269+
/// Efficiently builds a <see cref="Region"/> from line cells by grouping contiguous horizontal spans.
270+
/// This avoids the performance overhead of adding each cell individually while accurately
271+
/// representing the non-rectangular shape of the lines.
272+
/// </summary>
273+
/// <param name="cellMap">Dictionary of points where line cells are drawn. If empty, returns an empty Region.</param>
274+
/// <returns>A Region encompassing all the line cells, or an empty Region if cellMap is empty</returns>
275+
public static Region GetRegion (Dictionary<Point, Cell?> cellMap)
276+
{
277+
// Group cells by row for efficient horizontal span detection
278+
// Sort by Y then X so that within each row group, X values are in order
279+
IEnumerable<IGrouping<int, Point>> rowGroups = cellMap.Keys
280+
.OrderBy (p => p.Y)
281+
.ThenBy (p => p.X)
282+
.GroupBy (p => p.Y);
283+
284+
Region region = new ();
285+
286+
foreach (IGrouping<int, Point> row in rowGroups)
287+
{
288+
int y = row.Key;
289+
// X values are sorted due to ThenBy above
290+
List<int> xValues = row.Select (p => p.X).ToList ();
291+
292+
// Note: GroupBy on non-empty Keys guarantees non-empty groups, but check anyway for safety
293+
if (xValues.Count == 0)
294+
{
295+
continue;
296+
}
297+
298+
// Merge contiguous x values into horizontal spans
299+
int spanStart = xValues [0];
300+
int spanEnd = xValues [0];
301+
302+
for (int i = 1; i < xValues.Count; i++)
303+
{
304+
if (xValues [i] == spanEnd + 1)
305+
{
306+
// Continue the span
307+
spanEnd = xValues [i];
308+
}
309+
else
310+
{
311+
// End the current span and add it to the region
312+
region.Combine (new Rectangle (spanStart, y, spanEnd - spanStart + 1, 1), RegionOp.Union);
313+
spanStart = xValues [i];
314+
spanEnd = xValues [i];
315+
}
316+
}
317+
318+
// Add the final span for this row
319+
region.Combine (new Rectangle (spanStart, y, spanEnd - spanStart + 1, 1), RegionOp.Union);
320+
}
321+
322+
return region;
323+
}
324+
195325
// TODO: Unless there's an obvious use case for this API we should delete it in favor of the
196326
// simpler version that doesn't take an area.
197327
/// <summary>
@@ -227,7 +357,7 @@ public Dictionary<Point, Rune> GetMap (Rectangle inArea)
227357
}
228358
}
229359
// Safe as long as the list is not modified while the span is in use.
230-
ReadOnlySpan<IntersectionDefinition> intersects = CollectionsMarshal.AsSpan(intersectionsBufferList);
360+
ReadOnlySpan<IntersectionDefinition> intersects = CollectionsMarshal.AsSpan (intersectionsBufferList);
231361

232362
Rune? rune = GetRuneForIntersects (intersects);
233363

@@ -442,14 +572,14 @@ private void ConfigurationManager_Applied (object? sender, ConfigurationManagerE
442572
}
443573

444574
// TODO: Remove these once we have all of the below ported to IntersectionRuneResolvers
445-
bool useDouble = AnyLineStyles(intersects, [LineStyle.Double]);
446-
bool useDashed = AnyLineStyles(intersects, [LineStyle.Dashed, LineStyle.RoundedDashed]);
447-
bool useDotted = AnyLineStyles(intersects, [LineStyle.Dotted, LineStyle.RoundedDotted]);
575+
bool useDouble = AnyLineStyles (intersects, [LineStyle.Double]);
576+
bool useDashed = AnyLineStyles (intersects, [LineStyle.Dashed, LineStyle.RoundedDashed]);
577+
bool useDotted = AnyLineStyles (intersects, [LineStyle.Dotted, LineStyle.RoundedDotted]);
448578

449579
// horiz and vert lines same as Single for Rounded
450-
bool useThick = AnyLineStyles(intersects, [LineStyle.Heavy]);
451-
bool useThickDashed = AnyLineStyles(intersects, [LineStyle.HeavyDashed]);
452-
bool useThickDotted = AnyLineStyles(intersects, [LineStyle.HeavyDotted]);
580+
bool useThick = AnyLineStyles (intersects, [LineStyle.Heavy]);
581+
bool useThickDashed = AnyLineStyles (intersects, [LineStyle.HeavyDashed]);
582+
bool useThickDotted = AnyLineStyles (intersects, [LineStyle.HeavyDotted]);
453583

454584
// TODO: Support ruler
455585
//var useRuler = intersects.Any (i => i.Line.Style == LineStyle.Ruler && i.Line.Length != 0);
@@ -727,10 +857,10 @@ private bool Has (HashSet<IntersectionType> intersects, ReadOnlySpan<Intersectio
727857
private static class CornerIntersections
728858
{
729859
// Names matching #region "Corner Conditions" IntersectionRuneType
730-
internal static readonly IntersectionType[] UpperLeft = [IntersectionType.StartRight, IntersectionType.StartDown];
731-
internal static readonly IntersectionType[] UpperRight = [IntersectionType.StartLeft, IntersectionType.StartDown];
732-
internal static readonly IntersectionType[] LowerRight = [IntersectionType.StartUp, IntersectionType.StartLeft];
733-
internal static readonly IntersectionType[] LowerLeft = [IntersectionType.StartUp, IntersectionType.StartRight];
860+
internal static readonly IntersectionType [] UpperLeft = [IntersectionType.StartRight, IntersectionType.StartDown];
861+
internal static readonly IntersectionType [] UpperRight = [IntersectionType.StartLeft, IntersectionType.StartDown];
862+
internal static readonly IntersectionType [] LowerRight = [IntersectionType.StartUp, IntersectionType.StartLeft];
863+
internal static readonly IntersectionType [] LowerLeft = [IntersectionType.StartUp, IntersectionType.StartRight];
734864
}
735865

736866
private class BottomTeeIntersectionRuneResolver : IntersectionRuneResolver
@@ -773,14 +903,18 @@ private abstract class IntersectionRuneResolver
773903
internal Rune _thickBoth;
774904
internal Rune _thickH;
775905
internal Rune _thickV;
776-
protected IntersectionRuneResolver () { SetGlyphs (); }
906+
907+
protected IntersectionRuneResolver ()
908+
{
909+
SetGlyphs ();
910+
}
777911

778912
public Rune? GetRuneForIntersects (ReadOnlySpan<IntersectionDefinition> intersects)
779913
{
780914
// Note that there aren't any glyphs for intersections of double lines with heavy lines
781915

782-
bool doubleHorizontal = AnyWithOrientationAndAnyLineStyle(intersects, Orientation.Horizontal, [LineStyle.Double]);
783-
bool doubleVertical = AnyWithOrientationAndAnyLineStyle(intersects, Orientation.Vertical, [LineStyle.Double]);
916+
bool doubleHorizontal = AnyWithOrientationAndAnyLineStyle (intersects, Orientation.Horizontal, [LineStyle.Double]);
917+
bool doubleVertical = AnyWithOrientationAndAnyLineStyle (intersects, Orientation.Vertical, [LineStyle.Double]);
784918

785919
if (doubleHorizontal)
786920
{
@@ -792,9 +926,9 @@ private abstract class IntersectionRuneResolver
792926
return _doubleV;
793927
}
794928

795-
bool thickHorizontal = AnyWithOrientationAndAnyLineStyle(intersects, Orientation.Horizontal,
929+
bool thickHorizontal = AnyWithOrientationAndAnyLineStyle (intersects, Orientation.Horizontal,
796930
[LineStyle.Heavy, LineStyle.HeavyDashed, LineStyle.HeavyDotted]);
797-
bool thickVertical = AnyWithOrientationAndAnyLineStyle(intersects, Orientation.Vertical,
931+
bool thickVertical = AnyWithOrientationAndAnyLineStyle (intersects, Orientation.Vertical,
798932
[LineStyle.Heavy, LineStyle.HeavyDashed, LineStyle.HeavyDotted]);
799933

800934
if (thickHorizontal)

0 commit comments

Comments
 (0)