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

[core][ios][osx][android] fix icons with non-integer width/height #3561

Merged
merged 2 commits into from
Jan 20, 2016

Conversation

ansis
Copy link
Contributor

@ansis ansis commented Jan 15, 2016

ref #3031
ref #2198

For example, an icon that has:

  • a pixel width of 10
  • a pixel ratio of 3
  • a scaled with of 3.333

is now supported.

SpriteImage is now constructed with the pixel width of the image instead of the width.

this works in android with @tobrun's https://github.com/mapbox/mapbox-gl-native/tree/3031-test-branch-android
@1ec5 I think this should fix #2198 - what's the best way to test that?

@jfirebaugh
Copy link
Contributor

Approach here looks sound. I would like to continue by changing the SpriteImage constructor signature to:

SpriteImage(PremultipliedImage&&, float pixelRatio, bool sdf = false)

And replacing these SpriteImage members:

    const uint16_t width;
    const uint16_t height;
    const uint16_t pixelWidth;
    const uint16_t pixelHeight;
    const std::string data;

With:

const PremultipliedImage image;

@1ec5
Copy link
Contributor

1ec5 commented Jan 15, 2016

I think this should fix #2198 - what's the best way to test that?

Change this line to:

CGRect rect = CGRectMake(0, 0, 20, 15.5);

@1ec5 1ec5 added iOS Mapbox Maps SDK for iOS Android Mapbox Maps SDK for Android crash labels Jan 15, 2016
@1ec5 1ec5 added this to the ios-v3.1.0 milestone Jan 15, 2016
@1ec5
Copy link
Contributor

1ec5 commented Jan 15, 2016

Can you add a line to the Android and iOS sections of the changelog, too? Lots of folks are eager to see this fixed. 👏

@bleege
Copy link
Contributor

bleege commented Jan 15, 2016

Tagging myself here so that we can cherry pick this over to release-android-3.1.0 branch when this lands.

@ansis
Copy link
Contributor Author

ansis commented Jan 16, 2016

@jfirebaugh SpriteImage and PremultipliedImage changes implemented in 9cef5a7

@@ -1227,14 +1228,19 @@ void JNICALL nativeAddAnnotationIcon(JNIEnv *env, jobject obj, jlong nativeMapVi

jbyte* pixelData = env->GetByteArrayElements(jpixels, nullptr);
jsize size = env->GetArrayLength(jpixels);
std::string pixels(reinterpret_cast<char*>(pixelData), size);
env->ReleaseByteArrayElements(jpixels, pixelData, JNI_ABORT);
Copy link
Contributor

Choose a reason for hiding this comment

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

This line needs to be moved to after the std::copy. (Actually I'm reading now that GetByteArrayRegion is a better API to use: http://developer.android.com/training/articles/perf-jni.html.)

@ansis
Copy link
Contributor Author

ansis commented Jan 19, 2016

@jfirebaugh thanks for finding all the bugs, they should be fixed now

@jfirebaugh
Copy link
Contributor

👍

@ansis ansis force-pushed the fix-sprite-non-integer-width branch from 02f3b92 to f1fce01 Compare January 20, 2016 01:53
ref #3031
ref #2198

For example, an icon that has:
- a pixel width of 10
- a pixel ratio of 3
- a scaled with of 3.333
is now supported.
the SpriteImage constructor signature changes from

SpriteImage(
        uint16_t width, uint16_t height, float pixelRatio,
        std::string&& data, bool sdf = false);

to

SpriteImage(PremultipliedImage&&, float pixelRatio, bool sdf = false)
@ansis ansis force-pushed the fix-sprite-non-integer-width branch from f1fce01 to d34f8eb Compare January 20, 2016 02:25
@ansis ansis merged commit d34f8eb into master Jan 20, 2016
@ansis ansis removed the in progress label Jan 20, 2016
@ansis ansis deleted the fix-sprite-non-integer-width branch January 20, 2016 02:53
@bleege
Copy link
Contributor

bleege commented Jan 20, 2016

Should this be cherry picked into the release-android-v3.1.0 branch?

@ansis
Copy link
Contributor Author

ansis commented Jan 20, 2016

@bleege I think so. at least the first commit (26faa6a).

@tobrun tobrun mentioned this pull request Jan 20, 2016
@tobrun
Copy link
Member

tobrun commented Jan 20, 2016

I verified that this fixed the initial issue of having a rectangle image as marker icon

@bleege
Copy link
Contributor

bleege commented Jan 20, 2016

Cool! @tobrun confirmed that he tested from master and both 26faa6a and d34f8eb made it in there. I'll cherry pick both to release-android-v3.1.0 for today's release at #3537 .

Thanks everyone!

@bleege
Copy link
Contributor

bleege commented Jan 20, 2016

FYI... I fudged the cherry pick process for the first commit 26faa6a and ended up creating a new commit after resolving the changes instead of doing a git cherry-pick --continue hence 52fd9d7b505559972654a9f7af05d6be9c244f73. I did it the proper way for the second commit however. Discussed with @zugaldia this morning and we decided to leave this as is (and not deal with force pushes and other crazy git commands to be named later) and deal with the pain when the merge of release-android-v3.1.0 into master after release.

@bleege bleege mentioned this pull request Jan 20, 2016
12 tasks
@danswick
Copy link
Contributor

Huge! Our mobile SDK users will be pumped when this lands. 👏

@bleege
Copy link
Contributor

bleege commented Jan 21, 2016

@danswick FYI... we had to pull this from Android 3.1.0 today due to C++ Pain. It's now scheduled for 4.0.0.

@afathman
Copy link

When is the expected release of 4.0.0?

@bleege
Copy link
Contributor

bleege commented Jan 25, 2016

@afathman We're still planning the specifics around 4.0.0, but in the interim the fix for this issues is available immediately for use via the nighlty 4.0.0-SNAPSHOT builds.

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

Successfully merging this pull request may close these issues.

MGLAnnotationImage with scale greater than 1.0 causes crash.
7 participants