Backport improvements from TechPizzaDev fork.#366
Backport improvements from TechPizzaDev fork.#366JimBobSquarePants merged 37 commits intoSixLabors:mainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR includes a comprehensive set of code quality improvements, refactoring, and test infrastructure enhancements. The changes focus on modernizing C# code patterns, improving performance, and adding new drawing robustness test capabilities.
- Modernization of C# syntax (range operators, simplified expressions, expression-bodied members)
- Performance optimizations (lazy initialization, reduced allocations, better memory usage patterns)
- Test infrastructure improvements (new test cases, Linux SkiaSharp support)
Reviewed Changes
Copilot reviewed 51 out of 51 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/.../DrawingRobustnessTests.cs | Adds new scaled GeoJSON test and enables previously skipped test |
| tests/.../TestImageProviderTests.cs | Moves variable declaration to inner scope |
| tests/.../Issue_28_108.cs | Replaces instance field with inline color conversion |
| tests/.../FillPatternBrushTests.cs | Removes unnecessary this. qualifiers |
| src/.../BaseGlyphBuilder.cs | Introduces duplicate paths and PathList fields causing confusion |
| src/.../ActiveEdgeList.cs | Removes exception throw and adds readonly modifier |
| src/.../ScanEdgeCollection.Build.cs | Extracts local method to private static method |
| src/.../InternalPath.cs | Fixes spelling typo and modernizes code patterns |
| src/.../PathCollection.cs | Implements lazy bounds calculation |
| src/.../ComplexPolygon.cs | Implements lazy initialization for bounds and internal paths |
| src/.../EllipsePolygon.cs | Refactors to inherit from Polygon, uses lazy flattening |
| src/.../CubicBezierLineSegment.cs | Implements lazy computation of line points |
| src/.../PolygonClipper/* | Adds new files and modernizes existing code |
| .github/workflows/build-and-test.yml | Adds release branch triggers and formatting fixes |
| Various project files | Adds Linux SkiaSharp native assets support |
Comments suppressed due to low confidence (1)
src/ImageSharp.Drawing/Shapes/Text/BaseGlyphBuilder.cs:15
- The existing
pathsfield (line 15) is never used and creates confusion with the newPathListproperty (line 30). Both fields serve the same purpose. The oldpathsfield should be removed, and all code should referencePathListinstead.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| PathF points = new(pathPoints.Length); | ||
| for (int i = 0; i < pathPoints.Length; i++) | ||
| { | ||
| points.Add(pathPoints[i]); | ||
| } | ||
| points.AddRange(pathPoints); |
There was a problem hiding this comment.
Creating a new PathF with initial capacity and then calling AddRange is inefficient. Since PathF now inherits from List<PointF> (based on diff in PolygonOffsetter.cs), you can construct it directly: PathF points = new(pathPoints) which will be more efficient.
| foreach (IPath p in this.paths) | ||
| { | ||
| RectangleF pBounds = p.Bounds; | ||
|
|
||
| minX = MathF.Min(minX, pBounds.Left); | ||
| maxX = MathF.Max(maxX, pBounds.Right); | ||
| minY = MathF.Min(minY, pBounds.Top); | ||
| maxY = MathF.Max(maxY, pBounds.Bottom); | ||
| } |
There was a problem hiding this comment.
This foreach loop immediately maps its iteration variable to another variable - consider mapping the sequence explicitly using '.Select(...)'.
| foreach (IPath path in this.paths) | ||
| { | ||
| RectangleF bounds = path.Bounds; | ||
| minX = Math.Min(bounds.Left, minX); | ||
| minY = Math.Min(bounds.Top, minY); | ||
| maxX = Math.Max(bounds.Right, maxX); | ||
| maxY = Math.Max(bounds.Bottom, maxY); | ||
| } |
There was a problem hiding this comment.
This foreach loop immediately maps its iteration variable to another variable - consider mapping the sequence explicitly using '.Select(...)'.
| const float opaque = 1f; | ||
|
|
||
| if (options.BlendPercentage != Opaque) | ||
| if (options.BlendPercentage != opaque) |
There was a problem hiding this comment.
Equality checks on floating point values can yield unexpected results.
| } | ||
|
|
||
| if (color.ToScaledVector4().W != Opaque) | ||
| if (color.ToScaledVector4().W != opaque) |
There was a problem hiding this comment.
Equality checks on floating point values can yield unexpected results.
| RectangleF bounds = path.Bounds; | ||
| minX = Math.Min(bounds.Left, minX); | ||
| minY = Math.Min(bounds.Top, minY); | ||
| maxX = Math.Max(bounds.Right, maxX); | ||
| maxY = Math.Max(bounds.Bottom, maxY); |
There was a problem hiding this comment.
Local scope variable 'bounds' shadows PathCollection.bounds.
| RectangleF bounds = path.Bounds; | |
| minX = Math.Min(bounds.Left, minX); | |
| minY = Math.Min(bounds.Top, minY); | |
| maxX = Math.Max(bounds.Right, maxX); | |
| maxY = Math.Max(bounds.Bottom, maxY); | |
| RectangleF pathBounds = path.Bounds; | |
| minX = Math.Min(pathBounds.Left, minX); | |
| minY = Math.Min(pathBounds.Top, minY); | |
| maxX = Math.Max(pathBounds.Right, maxX); | |
| maxY = Math.Max(pathBounds.Bottom, maxY); |
| this.srcVertices.Add(new VertexDistance(x, y, distance)); | ||
| } | ||
|
|
||
| private void CloseVertexPath(bool closed) |
There was a problem hiding this comment.
Local scope variable 'closed' shadows PolygonStroker.closed.
| => this.internalPaths; | ||
| { | ||
| this.InitInternalPaths(); | ||
| return this.internalPaths; |
There was a problem hiding this comment.
Variable this.internalPaths may be null at this access because it has a nullable type.
| return this.internalPaths; | |
| return this.internalPaths!; |
| internal struct VertexDistance | ||
| { | ||
| private const double Dd = 1.0 / Constants.Misc.VertexDistanceEpsilon; | ||
| public double X; |
There was a problem hiding this comment.
Field 'X' can be 'readonly'.
| { | ||
| private const double Dd = 1.0 / Constants.Misc.VertexDistanceEpsilon; | ||
| public double X; | ||
| public double Y; |
There was a problem hiding this comment.
Field 'Y' can be 'readonly'.
Prerequisites
Description
Note: Most of the work here was done by @TechPizzaDev
This PR introduces several improvements and refactorings across the drawing and shapes components of the codebase. The most significant changes include refactoring the
ComplexPolygonandEllipsePolygonclasses for better performance and maintainability, updating the handling of Bezier and Arc line segments, and making various code quality and style improvements. Below are the most important changes grouped by theme:Shapes and Geometry Refactoring
ComplexPolygonto lazily compute bounds and internal paths, reducing upfront computation and improving performance. Introduced helper methods for internal path initialization and bounds calculation.EllipsePolygonto inherit fromPolygonand removed redundant internal state. Updated transformation logic to work with the new structure, simplifying the class and aligning it with other shape types.PolygonStrokerclass designed to replacePolygonOffsetterLine Segment Improvements
CubicBezierLineSegmentandArcLineSegmentto computeEndPointandFlatten()lazily, improving efficiency and consistency. Added a convenience constructor and a method to retrieve control points for Bezier segments.Code Quality and Style
opaqueconstant, use of slice syntax).Miscellaneous Fixes
RichTextGlyphRendererby referencing the correct path list.