Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Floating-point logical sprite image dimensions #3008

Closed
wants to merge 1 commit into from

Conversation

1ec5
Copy link
Contributor

@1ec5 1ec5 commented Nov 12, 2015

On HiDPI screens, the logical dimensions of a sprite image can be non-integral.

Fixes #2198.

/cc @incanus @friedbunny

@1ec5 1ec5 added iOS Mapbox Maps SDK for iOS crash labels Nov 12, 2015
@1ec5 1ec5 self-assigned this Nov 12, 2015
@1ec5 1ec5 added this to the ios-v3.0.0 milestone Nov 12, 2015
@friedbunny
Copy link
Contributor

Awesome that you tracked this down, fix works. Adding some decimals in iosapp's marker image code is enough to confirm:

diff --git a/ios/app/MBXViewController.mm b/ios/app/MBXViewController.mm
index 6380406..61d39b8 100644
--- a/ios/app/MBXViewController.mm
+++ b/ios/app/MBXViewController.mm
@@ -368,7 +368,7 @@ static UIColor *const kTintColor = [UIColor colorWithRed:0.120 green:0.550 blue:

     if ( ! image)
     {
-        CGRect rect = CGRectMake(0, 0, 20, 15);
+        CGRect rect = CGRectMake(0, 0, 20.5, 15.5);

         UIGraphicsBeginImageContextWithOptions(rect.size, NO, [[UIScreen mainScreen] scale]);

@jfirebaugh
Copy link
Contributor

I'm suspicious of the float ⇢ integer conversions that will be happening after this change here and here. We should probably review our handling of physical/logical dimensions at a higher level. Always using integer pixel dimensions plus float pixel ratio makes the most sense to me, but I haven't thought about it a lot.

@1ec5
Copy link
Contributor Author

1ec5 commented Nov 12, 2015

The upshot is that without floating point logical dimensions (expressed in "points" in iOS), you can't have an odd width or height after multiplying by the pixel ratio.

On HiDPI screens, the logical dimensions of a sprite image can be non-integral.

Fixes #2198.
@1ec5 1ec5 force-pushed the 1ec5-annotation-image-float-2198 branch from ec2796a to 281f366 Compare November 12, 2015 19:08
@1ec5
Copy link
Contributor Author

1ec5 commented Nov 12, 2015

The padding in allocateImage() would seem to inoculate this particular change from any ill effects. SpriteAtlas::getPosition() is only used to render backgrounds, fills, and lines, so it doesn’t come into play when assigning images to markers. I realize that introducing the concept of non-integral dimensions to just SpriteImage is problematic in the long run. However, this seems like a workable stopgap fix for #2198 while we consider more disruptive changes such as eliminating the concept of pixel density outside the renderer.

@1ec5 1ec5 removed this from the ios-v3.0.0 milestone Nov 17, 2015
tobrun added a commit that referenced this pull request Nov 25, 2015
[android] #2386 - Add vector drawable and rippleDrawable for attribution icon

[android] #2736 - Replace bitmap assets with vector drawable
@bleege
Copy link
Contributor

bleege commented Nov 30, 2015

Also related to Android via #3031

@jfirebaugh
Copy link
Contributor

I'm working on a different fix along the lines of #3008 (comment), and build on the new PremultipliedImage class.

@jfirebaugh jfirebaugh closed this Dec 2, 2015
@jfirebaugh jfirebaugh deleted the 1ec5-annotation-image-float-2198 branch December 2, 2015 00:12
@bleege
Copy link
Contributor

bleege commented Dec 4, 2015

@jfirebaugh Sounds good! Do you have a ticket or branch that we can link to so that we make sure this gets addressed / tickets closed in the iOS and Android bindings when it's ready?

@jfirebaugh
Copy link
Contributor

@bleege: #3164.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
crash iOS Mapbox Maps SDK for iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants