[asimage] Make the two ellipse fill spans mirror each other - #23133
Open
tekinertekin wants to merge 1 commit into
Open
[asimage] Make the two ellipse fill spans mirror each other#23133tekinertekin wants to merge 1 commit into
tekinertekin wants to merge 1 commit into
Conversation
asim_ellips2() fills a tilted ellipse by scanline rather than by flood fill: each iteration of the walk emits two horizontal spans, one on the row above the centre and one on the row below, exploiting the shape's 180-degree rotational symmetry. The two spans must therefore be mirror images about the centre, and they were not. The upper span ended at x2-1 while the lower one started at x-x2-1, which mirrors to x2+1 -- a two-column disagreement that went in both directions at once. Two columns end up unpainted inside the shape on the upper rows, and two columns outside it get painted on the lower rows. The gap is visible for every tilted angle and grows with the tilt; at 45 degrees with rx=21, ry=9 it leaves eight unfilled pixels in the interior, including the one the report points at. The straight cases are unaffected because angles of 0, 90, 180 and 270 return early to asim_straight_ellips(). Naming the span end makes the symmetry explicit and lets each branch of the walk set it from its own geometry. Only the line > yr branch needs a different value: it writes the anti-aliased outline at x2+2, so the fill may reach x2+1. The other two branches keep the previous end, so their output does not change. Measured against the interior of the shape's own outline, over 610 combinations of angle and radii: unfilled interior pixels drop from 7698 to 30 and painted exterior pixels from 7496 to 1102, with no combination getting worse on either count. Unfilled ellipses and the straight-ellipse path render bit-identically before and after, at every angle and radius tried. Refs root-project#23120 Assisted-by: Claude (Anthropic); the change was AI-assisted, then reviewed, measured and verified by the author.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This Pull request:
The function asim_ellips2() creates a tilted ellipse using a scanning technique, rather than through flood fill. On each step of the walk, two spans are created, one above and one below the center, based on the rotational symmetry of the shape. Thus, these spans should have been reflections around the center, but they weren't. While the upper span terminated at x2 - 1, the lower span began at x - x2 - 1, which mirrors to x2 + 1.
Changes or fixes:
Two columns remain unpainted inside the shape on the upper rows, while two columns outside it become painted on the lower rows. The gap is apparent for every tilt angle and increases with the angle; at 45 degrees for rx=21, ry=9 there are eight unpainted pixels inside the shape, including the one that is mentioned in the report. The straight cases are not affected since 0, 90, 180, and 270 degrees exit early in asim_straight_ellips().
Using an explicit name for the span end exposes the symmetry and allows each walk branch to set the end from its own parameters. Only one of them needs a different end: it draws the anti-aliased outline at x2+2, so the fill can go to x2+1. The other two branches retain the previous end, so the output of theirs is unchanged.
Measured against the interior of the shape itself using its outline, over 610 combinations of angle and radii: number of interior pixels that were not painted decreases from 7698 to 30, number of exterior pixels that were painted decreases from 7496 to 1102; neither of the combinations gets worse on any of these criteria. Unfilled ellipses and straight-ellipse path paint identically before and after the change, for every tested angle and radius.
Before and after
DrawEllips2(32, 32, 21, 9, 45, "#FF2277CC", -1), the reproducer from the issue. The red cells in the magnified crop are the pixels the fill leaves empty; that region is scaled 22x with a pixel grid.masterThe gap is not specific to 45 degrees. At 85 degrees the same radii leave 30 pixels empty instead of 8:
masterThe tests already exist
graf2d/asimage/test/tasimage_ellipse_draw.cxx, added in #23119, already covers this, including the assertion for the pixel named in the issue:masterTASImage.FilledEllipsOpaqueTASImage.FilledEllipsHighAlphaTASImage.FilledEllipsSemiTransparentTASImage.FilleEllipsLowAlphaAll four fail on the same assertion, the one added for this bug:
The corner, centre and fill-count assertions pass both before and after; only that one flips. The fill count also moves closer to the expected area, 699 to 689 against an expected 691, rather than further from it.
I could not run the gtest binary itself, since this build has
asimage=OFF, so I reimplemented the four assertions against asim_ellips2() directly in a standalone harness over the vendored draw.c. CI will be the real check.Assisted-by: Claude (Anthropic); the change was AI-assisted, then reviewed, measured and verified by the author.
Checklist:
This PR fixes #23120