-
Notifications
You must be signed in to change notification settings - Fork 7.1k
remove fill blending for bilinear affine #8098
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
base: main
Are you sure you want to change the base?
Conversation
bool_mask = mask < 1 | ||
float_img[bool_mask] = fill_img.expand_as(float_img)[bool_mask] |
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 only equivalent to the 'bilinear' block if mask
was a boolean mask
, right?
Is that the case? And if it is, why was there even a distinction between bilinear and nearest mode in the first place?
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 only equivalent to the 'bilinear' block if
mask
was a booleanmask
, right?
Yes.
Is that the case?
No.
And if it is, why was there even a distinction between bilinear and nearest mode in the first place?
I would need to dig, but maybe @vfdev-5 knows? My guess is that the blend strategy we implemented was to have a smooth transition from fill to image. And that works well if we fill with zeros, but creates a shadow artifact if we use a different fill color with bilinear interpolation. See #8083.
Meaning, this PR is BC breaking, but I consider it a bug fix since the blending behavior seems to be "nice to have" while the shadow is an actual issue.
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.
I think it was an incorrect implementation. At the moment of PR (#2904), I thought it could be ok as assumption to linearly interpolate around the boundaries: #2904 (comment)
Fixes #8083 and addresses the corresponding part in #6517.
cc @vfdev-5