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

[core][ios][Android] Check layer compatibility with source #15644

Merged
merged 8 commits into from
Sep 18, 2019
Merged

[core][ios][Android] Check layer compatibility with source #15644

merged 8 commits into from
Sep 18, 2019

Conversation

pozdnyakov
Copy link
Contributor

@pozdnyakov pozdnyakov commented Sep 17, 2019 •
edited
Loading

Fixes #15636

@pozdnyakov pozdnyakov added the Core The cross-platform C++ core, aka mbgl label Sep 17, 2019
@pozdnyakov pozdnyakov self-assigned this Sep 17, 2019
@pozdnyakov pozdnyakov marked this pull request as ready for review September 17, 2019 16:06
@pozdnyakov pozdnyakov requested a review from a team September 17, 2019 16:06
@pozdnyakov pozdnyakov added iOS Mapbox Maps SDK for iOS bug labels Sep 17, 2019
@julianrex
Copy link
Contributor

@pozdnyakov I rebased a local copy of your branch, branched and pushed: https://github.com/mapbox/mapbox-gl-native/tree/jrex/check-source-compatibility

These are speculative fixes for the 3 failing iOS tests:

06ee040 addresses:

  • testRemovingLayerBeforeAddingSameLayer
  • testRemovingSourceInUse

ed7321e addresses:

  • MGLDocumentationExampleTests.testMGLHillshadeStyleLayer()

This last one was because it was creating a RasterSource instead of a RasterDEMSource - so I think your PR & the tests have unearthed a genuine bug 👍.

@frederoni Can you take a look at the C++/Obj-C nature of ed7321e - I'm not super keen that it's passing C++ objects BUT this is an internal method (and not exposed publicly), so I think we should be ok here.

@julianrex julianrex added this to the release-ristretto milestone Sep 18, 2019
@pozdnyakov pozdnyakov force-pushed the mikhail_check_source_compatibility branch from 62561a3 to 102dd4a Compare September 18, 2019 07:37
@pozdnyakov
Copy link
Contributor Author

@julianrex Thank you! I've reset this PR to your branch!

alexshalamov
alexshalamov reviewed Sep 18, 2019
View reviewed changes
platform/darwin/src/MGLRasterTileSource.mm
@@ -33,15 +33,16 @@ - (instancetype)initWithIdentifier:(NSString *)identifier configurationURL:(NSUR
}

- (instancetype)initWithIdentifier:(NSString *)identifier configurationURL:(NSURL *)configurationURL tileSize:(CGFloat)tileSize {
auto source = [self pendingSourceWithIdentifier:identifier configurationURL:configurationURL tileSize:tileSize];
NSString *configurationURLString = configurationURL.mgl_URLByStandardizingScheme.absoluteString;
auto source = [self pendingSourceWithIdentifier:identifier urlOrTileset:configurationURLString.UTF8String tileSize:tileSize];
Copy link
Contributor

Choose a reason for hiding this comment

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

should tileSize be rounded?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good spot. That was there before, I'll re-add.

src/mbgl/style/style_impl.cpp Show resolved Hide resolved
src/mbgl/style/sources/vector_source.cpp Outdated Show resolved Hide resolved
@pozdnyakov pozdnyakov added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Sep 18, 2019
@pozdnyakov
Copy link
Contributor Author

Working on an alternate approach atm

@pozdnyakov pozdnyakov force-pushed the mikhail_check_source_compatibility branch from 102dd4a to cec8599 Compare September 18, 2019 11:57
@pozdnyakov pozdnyakov removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Sep 18, 2019
@frederoni
Copy link
Contributor

Can you take a look at the C++/Obj-C nature of ed7321e

I share your concerns since Obj-C has a concrete type for url but it's clear by the type of the arg.
LGTM 👍

@pozdnyakov pozdnyakov changed the title [core] Check layer compatibility with source [core][ios] Check layer compatibility with source Sep 18, 2019
alexshalamov
alexshalamov approved these changes Sep 18, 2019
View reviewed changes
Copy link
Contributor

@alexshalamov alexshalamov left a comment

Choose a reason for hiding this comment

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

lgtm

@pozdnyakov pozdnyakov changed the title [core][ios] Check layer compatibility with source [core][ios][Android] Check layer compatibility with source Sep 18, 2019
@pozdnyakov pozdnyakov merged commit 5abb4ae into master Sep 18, 2019
@pozdnyakov pozdnyakov deleted the mikhail_check_source_compatibility branch September 18, 2019 13:22
@julianrex julianrex mentioned this pull request Sep 18, 2019
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alexshalamov alexshalamov alexshalamov approved these changes

@julianrex julianrex Awaiting requested review from julianrex

@1ec5 1ec5 Awaiting requested review from 1ec5

@frederoni frederoni Awaiting requested review from frederoni

Assignees

@pozdnyakov pozdnyakov

Labels
bug Core The cross-platform C++ core, aka mbgl iOS Mapbox Maps SDK for iOS
Projects
None yet
Milestone
release-ristretto
Development

Successfully merging this pull request may close these issues.

[core] Assert fires in LayerFactory::createBucket
4 participants
@pozdnyakov @julianrex @frederoni @alexshalamov