Skip to content
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

We shouldn't assume that the bounds we get will be pixel aligned, #345

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pineapplespatula
Copy link
Contributor

This change prevents us from trying to crop pixmaps we recieve down to non pixel aligned boundaries

This is a more targeted( but higher risk ) fix for issue adobe-photoshop/generator-assets#350

and is an alternate to the more heavy handed approach in adobe-photoshop/generator-assets#406

…s change prevents us from trying to crop pixmaps we recieve down to non pixel aligned boundaries
@mcilroyc
Copy link
Contributor

mcilroyc commented May 3, 2016

@chadrolfs - this PR indeed seems to fix the clipping behavior in issue 350. But the resultant 1x/2x asset dimensions are 30x34 and 60x66 respectively. The images look correct, given the pixel alignment of the test file... but it seems wrong that the 2x asset is not exactly twice the height. Thoughts? See output zip
issue-350-test.zip
(note that the behavior is the same between this PR and the alternate g-a PR 406)

@chadrolfs
Copy link
Contributor

I would agree that the x2 seems wrong from a straight dimensions standpoint, especially from user perspective; 34 @ 2x != 66. Looking at the output file though, the shape size seems correct and there isn't pixel clipping as seen in the bug. It looks like the transparent pixels get chopped resulting in the -2 px discrepancy. So, while I'd say the output clipping is correct, the dimensions may not be what a user would expect.

@mcilroyc
Copy link
Contributor

mcilroyc commented May 3, 2016

I stepped through this for the test file and realized that for the 2x asset, visibleOutputHeight is calculated to be 68 even though the pixmap ultimately returns 66. The end result is 66, but the discrepancy makes me uneasy with this implementation.

@mcilroyc
Copy link
Contributor

mcilroyc commented May 6, 2016

I don't see that the newest commit resolved the discrepancy.
http://cl.ly/3H023i2R2r0m

  1. visibleOutputHeight is still 68
  2. pixelAlignedInputHeight is probably not a good name, if you are now scaling that value
  3. finalOutputHeight is 64?
  4. The resultant image on disk is 66

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants