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

Add an original size constant. #274

Merged
merged 1 commit into from Dec 2, 2014
Merged

Conversation

sjudd
Copy link
Collaborator

@sjudd sjudd commented Nov 21, 2014

I'm still defaulting to screen dimensions when given wrap_content for Views on the theory that, on average, downsampling to around the screen size and providing an image that can be displayed (isn't over the max gl texture size) and that won't blow through huge quantities of memory is preferable, despite the occasional issue with cropping (see #151). Although this is safer from a display/memory perspective, it's also less correct. You could definitely make a reasonable argument that WRAP_CONTENT should be treated as SIZE_ORIGINAL.

All that said, I did included fixes for #135, so if WRAP_CONTENT is set for only one of width/height, we will only substitute in the screen size for the single dimension. I'd expect that change will at least mitigate some of the issues people have raised in #151 and #263.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling 827fc08 on sjudd:original_size into b6763ae on bumptech:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling 827fc08 on sjudd:original_size into b6763ae on bumptech:master.

/**
* Indicates that we want the resource in its original unmodified width and/or height.
*/
int SIZE_ORIGINAL = Integer.MIN_VALUE;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about View.WRAP_CONTENT as a value? Did you want to make it different deliberately?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the central question here:

Do we treat wrap_content as size original, or do we treat it as something else?

The primary difference I can see is that wrap_content will at most resolve to the screen width and height, whereas the original image may be substantially larger than the screen.

If I want the original bytes, probably to do something other than display them (share/upload etc), I'd use size original. If I want to show the image in roughly its original resolution, I'd use wrap_content and load it into a view.

The primary benefit of using the screen size for wrap_content is that it allows us to downsample large images which decreases memory usage and avoids issues with the maximum gl texture size (2048x2048 on older phones). That said, using the screen size will result in slight differences when cropping images with wrap_content, particularly with unusual aspect ratios or views that are limited in size by their parents.

If we do want to keep these concepts separate, I think the warning is no longer necessary, we're trying to claim that this is correct behavior. If we don't, then I should probably change this so that size original is wrap content and stop paying any attention to screen size.

sjudd added a commit that referenced this pull request Dec 2, 2014
Add an original size constant.
@sjudd sjudd merged commit 3cf5a30 into bumptech:master Dec 2, 2014
@sjudd sjudd deleted the original_size branch March 1, 2015 17:50
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.

None yet

3 participants