-
Notifications
You must be signed in to change notification settings - Fork 8.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix misalignment of Sixel image slices #17724
Conversation
const auto srcHeight = srcCellSize.height; | ||
const auto dstWidth = srcWidth * dstCellSize.width / srcCellSize.width; | ||
const auto dstHeight = srcHeight * dstCellSize.height / srcCellSize.height; | ||
const auto dstHeight = dstCellSize.height; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a little optimization that I noticed when investigating the rounding problems. The srcHeight
is always srcCellSize.height
, so the dstHeight
should always be dstCellSize.height
.
I'm assuming the audit failure is because the build server has a new version of the compiler. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, that's subtle. Thanks!
Don't worry about audit mode - #17723 |
Summary of the Pull Request
When we have a series of image slices of differing widths, which also
don't align with the cell boundaries, we can get rounding errors in the
scaling which makes the different slices appear misaligned.
This PR fixes the issue by removing the 4 pixel width alignment that was
enforced in the
ImageSlice
class, since that's not actually necessarywhen the pixels themselves are already 4 bytes in size. And without
that, the widths should be correctly aligned with the cell boundaries.
References and Relevant Issues
The initial Sixel implementation was added in PR #17421.
Validation Steps Performed
I've confirmed that this fixes the rendering glitches reported in
#17711, and all my existing Sixel tests still work as expected.
PR Checklist