1

I'm trying to understand Swift better through this example. If in the following code the variable snapshotter is declared as a class variable the function startSnapshot does it's intended purpose. But as soon as the variable snapshotter is moved to a local variable the code stops working as snapshotter.onStyleLoaded.observeNext is never triggered.

I feel as if there should be a way to wait at the end of startSnapshot until all tasks have been finished, but I can't figure out how the waiting should be done.


import UIKit
import MapboxMaps

final class SnapshotterCoreGraphicsExample: UIViewController, NonMapViewExampleProtocol {
  private var snapshotView: UIImageView!
  private var cancelables = Set<AnyCancelable>()
  
  override func viewDidLoad() {
    super.viewDidLoad()
    
    // Add the `UIImageView` that will eventually render the snapshot.
    snapshotView = UIImageView(frame: CGRect.zero)
    snapshotView.translatesAutoresizingMaskIntoConstraints = false
    view.addSubview(snapshotView)
    
    NSLayoutConstraint.activate([
      snapshotView.centerXAnchor.constraint(equalTo: view.centerXAnchor),
      snapshotView.centerYAnchor.constraint(equalTo: view.centerYAnchor)
    ])
    
    startSnapshot()
  }
   
  private func startSnapshot() {
    // Configure the snapshot options, such as the size, scale and style to be used.
    // Here we use a scale (pixelRatio) that is different than the display's
    // scale (UIScreen.main.scale)
    let options = MapSnapshotOptions(size: CGSize(width: view.bounds.size.width, height: view.bounds.height), pixelRatio: 4)
    
    var snapshotter = Snapshotter(options: options)
    snapshotter.styleURI = .dark
    snapshotter.setCamera(to: CameraOptions(center: CLLocationCoordinate2D(latitude: 51.180885866921386, longitude: 16.26129435178828), zoom: 4))
    
    snapshotter.onStyleLoaded.observeNext { [weak self] _ in
      // Begin the snapshot after the style is loaded into the `Snapshotter`.
      // The `SnapshotOverlay` object contains references to the current
      // graphics context being used by the Snapshotter and provides closures to
      // perform coordinate conversion between map and screen coordinates.
      snapshotter.start { ( overlayHandler ) in
        let context = overlayHandler.context
        
        // Convert the map coordinates for Berlin and Kraków to points,
        // in order to correctly position the Core Graphics drawing code.
        let berlin = overlayHandler.pointForCoordinate(CLLocationCoordinate2D(latitude: 52.53, longitude: 13.38))
        let krakow = overlayHandler.pointForCoordinate(CLLocationCoordinate2D(latitude: 50.06, longitude: 19.92))
        
        // Draw a yellow line between Berlin and Kraków.
        context.setStrokeColor(UIColor.yellow.cgColor)
        context.setLineWidth(6.0)
        context.setLineCap(.round)
        context.move(to: berlin)
        context.addLine(to: krakow)
        context.strokePath()
      } completion: { [weak self] result in
        switch result {
        case .success(let image):
          self?.snapshotView.image = image
        case .failure(let error):
          print("Error generating snapshot: \(error)")
        }
      }
    }
  }
}

I have tried using a DispatchGroup, Semaphore and DispatchQueue but I seem to be missing something as what I achieve is either blocking the UI thread or nothing happens.

What is the correct way to handle this situation so that snapshotter can remain a local variable, and the style is loaded properly before taking the snapshot?

2

The issue here is that nothing holds your snapshotter when it just a local variable of a function. Observer is executed asynchronously, so as soon as function returns after setting it, the instance is deallocated, and nothing happens after that. To resolve that, you need something which will hold strong reference to the snapshotter as long as it needs to. For example, if you need snapshot updates to run as long as controller exist, you make it to be a property on instance of this controller.

Other options, as you’ve experienced, will only block execution flow.

3

First of all: Welcome to the forums! :smiley:

@vns already correctly explained the base issue, but I think it might be useful to dig a bit deeper into what actually seems to make this "weird" for you.

I have been in this situation in the past myself, so I am taking an educated guess what your line of thinking is, if I am wrong I apologize in advance.

When you write

you are "leading yourself" into an XY Problem.
I assume you don't want to make snapshotter a property because it requires the view's bounds and you do not get those before viewDidLoad is called. Furthermore, you don't need it afterwards anymore (I assume), so you feel like you'd "waste" things when making it a property. Ergo, you think "I want to wait until I get my bounds, then do the work and not keep left-overs".

That is, unfortunately, something that a view controller's viewDidLoad method does not give you. The method is synchronous and demands you return in a reasonable amount of time. It's part of the framework and when you override it, you have to obey this "semantic contract". Trying to change that is an unsolvable "X problem", that, while it would solve your "Y" is not feasible.

@vns already laid out the simplest solution: Make snapshotter a property (i.e. a member variable, if you're coming from other programming languages). I'd suggest to make it an optional, as indeed it has no meaningful value before viewDidLoad is called (as it needs the bounds, which do not exist before the view is loaded). That is fine and not an unusual solution.

Another approach would be using a Task, i.e. unstructured concurrency, but since your Snapshotter seems to already rely on closures for asynchronous work this would probably be a bit "dirty" for now. Also, you wrote "to understand Swift better", so I assume you are still learning and throwing in concurrency right away might be too much at once.
If you want to, I can give a simple example how I'd do it these days. For that, however, I'd like to know whether MapSnapshotOptions and Snapshotter are reference or value types (classes or structs).

2 Likes
4

Just learning swift but I'm comfortable with concurrency in other languages. MapSnapshotOptions is a struct and Snapshotter is a class.

I had a similar problem in real world where the "startSnapshot" as triggered concurrently and there I solved the issue by saving the "snapshotter" variable in a dict and when the "startSnapshot" was finished the value was removed from the dict. But it feels that it was a hack and there exists a clean solution.

5

BTW, I believe you missing storing cancellable:

snapshotter.onStyleLoaded.observeNext { [weak self] _ in
}
.store(in: &cancellables) // that one is missing

Which should prevent observer to be dropped.

In general, saving as property has nothing wrong or unclean. While saving in a dict is a bit odd and unclear way to deal with it (why on just optional?). That's all about managing lifetime of the instance you want to deal with. Asynchronous calls make function leave earlier and remove all the references it has locally, which is a default behaviour when you use automatic reference counting.

1 Like
6

Okay, this should not be too complex I guess, without having tried it, using a Task could roughly look like this:

  private func startSnapshot() {
    // being a value type, options is automatically Sendable, so I can let the Task's closure capture that later
    let options = MapSnapshotOptions(size: CGSize(width: view.bounds.size.width, height: view.bounds.height), pixelRatio: 4)
    
    Task { [weak self] in // note that this Task will be implicitly @MainActor as it inherits that from the method
      var snapshotter = Snapshotter(options: options)
      // ...
    
      snapshotter.onStyleLoaded.observeNext { [weak self] _ in
        // This becomes a little "callback hell" now, personally I'd try to give the snapshotter async methods now instead
        snapshotter.start { ( overlayHandler ) in
          let context = overlayHandler.context
          // ... I believe this would not be changed
        } completion: { [weak self] result in
        // This also looks okay so far
        }
      }
    }
  }

I am actually not super sure about where the [weak self] is needed anymore (I think this should be correct, but the compiler will let you know). It's definitely needed in the last closure, of course.

Depending on how the snapshotter and its internals work, you might also be able to use checked continuations or the like, but that goes beyond what I can assume for now.
The gist is that in principle this works in a similar way: The snapshotter is basically retained by the Task (which lives until its completion). Funnily enough, there's not any suspension points in the Task's closure (no await), which is perhaps unusual, but not bad. You don't have to have async method calls inside an asynchronous execution context. However, as said, I would look into making the Snapshotter have async methods in exchange (or addition) of the closures, that fits well in the Task then.

Edit: indentation...

7

While saving in a dict is a bit odd and unclear way to deal with it (why on just optional?).

for concurrent use

8

I don’t see any connection between using dictionary and concurrency, instead of optional. Plus, what concurrent use do you mean? Saving property will happen before any other access, and release will happen on the same queue (since the completion will update UI, it will be the same main queue). So far neither dictionary nor optional by itself are safe in concurrent access.

1 Like
9

I concur with @vns's remark about concurrency (hehe...).
I assume in a different context you had a situation where calls from multiple concurrency contexts started a snapshotter (which cannot happen here, as viewDidLoad is @MainActor), but as @vns said neither a dictionary nor an optional protect you here, that's what actors are for.
Concurrency or not, an optional in general means you have to be careful if the method that triggers its instantiation can be called multiple times, as subsequent calls would then naturally overwrite previous ones, but that is not an issue here, as viewDidLoad is only called once (unless you call it yourself somewhere, which you should never do).

Even if (in a different scenario) that can occur, this is not automatically a problem, as long as previous instances do nothing destructive in any potential escaping closures.
I'd say in such a context Tasks are a bit better (as you can e.g. cancel them, too), but nothing in your example would be "unclean" if you use an optional.

Conceptually, I'd say it models valid state information about the view controller: whether it is in the process of taking a snapshot (or at least "ready" for that).

10

I don’t see any connection between using dictionary and concurrency, instead of optional. Plus, what concurrent use do you mean?

Well not in the sample code provided, but in a real world code :) The "process" takes a few seconds and if the "snaphotter" variable is disposed during the course of the "process" then the "process" will be canceled. There can be several "processes" that run in parallel. Thus I hold the value of "snapshotter" in a dictionary tied with an ID of the request.