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

MGLSource should not maintain parallel state #7375

Description

@1ec5

MGLSource and its subclasses, particularly MGLShapeSource, currently hang onto objects passed into their initializers in order to return them in their properties’ getters. This is a workaround for #6584, but it’s problematic because MGLSource objects don’t necessarily know the canonical value of these properties. For example, if you get a source via -[MGLStyle sourceWithIdentifier:], the properties are all unset because the source was initialized via -[MGLSource initWithRawSource:]. You can then proceed to modify the source’s contents behind the backs of any other MGLShapeSource instances that wrap that source:

let one = MGLShapeSource(identifier: "one", shape: MGLPointFeature(), options: nil)
mapView.style.addSource(one)
// a short while later…
let uno = mapView.style.source(withIdentifier: "one")
uno.shape = someOtherFeature
// finally…
assert(one.shape is MGLPointFeature)
// 💣💥

/cc @boundsj

Activity

  1. 1ec5 commented on Dec 11, 2016

    @1ec5
    ContributorAuthor

    This is essentially blocked by #7376, because otherwise we can continue to expose an -[MGLShapeSource setShape:] but we’d have to eliminate -[MGLShapeSource shape]. A write-only property is theoretically possible but unheard of in Objective-C and Swift. (The closest thing I can think of is -[NSView setNeedsDisplay:], which isn’t a property and which no one thinks is a good design.)

  2. 1ec5 commented on Dec 12, 2016

    @1ec5
    ContributorAuthor

    #7377 will eliminate MGLShapeSource.geoJSONData and make MGLShapeSource.URL’s getter computed. It will also eliminate all the parallel state in MGLRasterSource and MGLVectorSource. However, MGLShapeSource.shape remains.

  3. boundsj commented on Dec 12, 2016

    @boundsj
    Contributor

    I updated the example in the original description above:
    s/let uno = mapView.style.layer(withIdentifier: "one")/let uno = mapView.style.source(withIdentifier: "one")

  4. boundsj commented on Dec 12, 2016

    @boundsj
    Contributor

    This gets into #7376 and the memory concerns noted there but would it make sense for MGLStyle to keep references to the source and layer objects in use? Then, asking the style for a source would/could always return the same source instance.

  5. 1ec5 commented on Dec 13, 2016

    @1ec5
    ContributorAuthor

    It's definitely possible and it'd solve several problems associated with these wrappers. We already do that for MGLOpenGLStyleLayers, which are entirely managed by the style. I wanted to avoid doing that for sources and layers in general, since there may be ways for the style to change behind MGLStyle's back, whereas a custom layer can only come from application code.

  6. jfirebaugh commented on Dec 14, 2016

    @jfirebaugh
    Contributor

    I'm open to adding a void* peer member to mbgl::Style::Source, mbgl::Style::Layer, etc., as a place where SDKs could stash a pointer to the SDK-level object, so that they can ensure that methods like getLayer() always returns the same object for a given identifier. Would that help?

  7. 1ec5 commented on Dec 14, 2016

    @1ec5
    ContributorAuthor

    The more I think about this, the more attractive @boundsj’s idea in #7375 (comment) looks. The only catch is that we have to be absolutely certain MGLStyle knows exactly when a source or layer goes away, such as due to polyline/polygon annotation removals.

    A void* peer member on Source and Layer wouldn’t be a complete solution, because ARC doesn’t track ownership in void* pointers – the pointer would immediately become stale without some other Objective-C object hanging onto the object. However, a general-purpose void* context member would probably be useful for associating (non-object) data with any mbgl object that the SDK might need to wrap.

  8. jfirebaugh commented on Dec 14, 2016

    @jfirebaugh
    Contributor

    What does ARC do if you use a type-erasure-based any type instead of void*? http://www.artima.com/cppsource/type_erasure2.html

  9. added this to the ios-future milestone on Dec 24, 2016
  10. removed this from the ios-future milestone on Nov 15, 2017
  11. 1ec5 commented on Dec 2, 2018

    @1ec5
    ContributorAuthor

    Type erasure via any would also potentially eliminate the edge cases caused by application code and mbgl effectively maintaining separate registries of sources (or layers).

    /ref #13492

  12. removed
    archivedArchived because of inactivity
    on Dec 2, 2018
  13. stale commented on May 31, 2019

    @stale

    This issue has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.

  14. 1ec5 commented on Aug 6, 2019

    @1ec5
    ContributorAuthor

    Still the biggest pain point with runtime styling in Objective-C and Swift.

  15. reopened this on Aug 6, 2019
  16. removed
    archivedArchived because of inactivity
    on Aug 6, 2019
  17. 1ec5 commented on Aug 6, 2019

    @1ec5
    ContributorAuthor

    A void* peer member on Source and Layer wouldn’t be a complete solution, because ARC doesn’t track ownership in void* pointers – the pointer would immediately become stale without some other Objective-C object hanging onto the object.

    This might not be a problem anymore if ARC can track ownership of Objective-C objects within C/C++ structs. The only unknown, then, is whether that also extends to void * or any pointers.

  18. stale commented on May 22, 2020

    @stale

    This issue has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions