AndrewHartAR / ARKit-CoreLocation

Combines the high accuracy of AR with the scale of GPS data.
MIT License
5.46k stars 739 forks source link

Investigate requiring iOS 11 #160

Open intere opened 5 years ago

intere commented 5 years ago

Consider setting the minimum iOS version to iOS 11 for ARCL.

Open Questions:

intere commented 5 years ago

@halmueller has volunteered to work this issue.

halmueller commented 5 years ago

I'm more confused after looking into this than I was before I started.

I see that the Deployment Target for the demo app is 9.0.

The library has 3 targets: ARCL, Pods-ARKit+CoreLocation, and Pods-ARKit+CoreLocation-ARCLTests. ARCL has a Deployment Target of 9.0. Pods-ARKit+CoreLocation and Pods-ARKit+CoreLocation-ARCLTests have Deployment Targets of 11.0.

It looks to me like ARCL exists specifically to allow linking from a final app targeting 9.0. The other two targets already target 11.0.

Note this beginning of POIViewController.swift:

import ARCL
import ARKit
import MapKit
import SceneKit
import UIKit

@available(iOS 11.0, *)
/// Displays Points of Interest in ARCL
class POIViewController: UIViewController {

What would it take to change the Deployment Target for ARCL to 11.0 and still keep the Deployment Target for the demo app at 9.0?

A recompile after changing ARCL's DT to 11.0 causes this error on compile, on the import ARCL line:

Compiling for iOS 9.0, but module 'ARCL' has a minimum deployment target of iOS 11.0: /Users/hal/Library/Developer/Xcode/DerivedData/ARKit+CoreLocation-ddqtwxizbgbfcxatnrgepsqmzzsj/Build/Products/Debug-iphoneos/ARCL/ARCL.framework/Modules/ARCL.swiftmodule/arm64.swiftmodule

How about wrapping the ARCL/ARKit imports in an @available()? Well, you can't do that for import statements.

Instead, how about this modified version:

#if canImport(ARCL)
import ARCL
import ARKit
#endif
import MapKit
import SceneKit
import UIKit

Same error: module 'ARCL' has a minimum deployment target of iOS 11.0.

One more try:

#if canImport(ARKit)
import ARCL
import ARKit
#endif
import MapKit
import SceneKit
import UIKit

and that's the same error, one more time.

Stepping back: I think the core ARCL code is already built for 11.0. Then there's a wrapper layer that exists solely to allow linking by pre-11.0 apps.

I'd still like to see us move up to 12.0 (for USDZ support), or maybe even 13.0, depending on the timeline we'll see announced tomorrow (September 10). But I think we're already at 11.0, and that no mods are required right now.

Pilot-Marc commented 5 years ago

I'm 100% in favor of bumping the minimum supported iOS to 11, which is when Apple introduced ARKit and SafeAreaInsets. I'm hesitant to bump it to iOS 12 unless there's evidence that an iOS 11 bug makes ARKit, SceneKit, or CLLocation unusable. And I'm not in favor of limiting it to iOS 13, as many implementations of ARCL (like mine) do not use USDZ.

intere commented 5 years ago

I think we can probably bump it to iOS 11. The reason it's been left at iOS 9 is so that apps that support that far back don't have to force their user base to use that platform. I think it's safe to bump to iOS 11 and leave it there and reevaluate again in the future.

halmueller commented 5 years ago

I dug into this some more. I now disagree with the proposal to require 11. We should leave iOS version requirements alone for now.

The core library already requires iOS 11. The original author went to some great pains to wrap everything in iOS 9-compatible code. But all of the iOS 11 parts are already wrapped in @available blocks.

Note the targets: ARCL requires 9. But Pods-ARKit+CoreLocation requires 11.

I don't completely understand the tricks that Mr. Dent used to define the OS compatibility. But what we have right now is a framework that's linkable back to iOS 9 yet takes advantage of iOS 11+ if it's there. There's nothing IMO to be gained by changing that.

Why might we want to raise the core library's minimum OS in the future? Only if we need to add an internal feature that's restricted to ARKit newer than 11.0. I believe (although I haven't tested this) that I could externally add an iOS 12+ feature (such as a creating a node from a USDZ model) to an ARCL view, without changing ARCL.

I think we should close this issue. "works as intended", "already implemented", or something. More doc wouldn't hurt though.

Pilot-Marc commented 4 years ago

I'm struggling with a change to SceneLocationManager that requires iOS 11. Why does this file not have a blank if iOS11 around it?

halmueller commented 4 years ago

There's nothing in the existing SceneLocationManager that really depends on ARKit, so no conditionals are needed. The import of ARKit is only needed to get the definition of SCNVector3, which comes from SceneKit.

That said, though, I'm swinging back to saying drop the conditional compilation/linking. When the library was created, iOS 10 was current and iOS 11 (ARKit) was in beta. Supporting back to iOS 9 made sense in case someone wanted to use ARCL in an app that had to have support for iOS 9. The conditionals impose a bit of cognitive friction (like the need to have this conversation at all). I don't think they're solving a problem that anyone still has, 2 1/2 years after the first commit to the project.

In the interest of simplifying, I'd like to see us cut 1.2.2 as soon as possible, and include in there an announcement that future releases will require iOS 13.

Pilot-Marc commented 4 years ago

My vote is stick with a minimum of iOS 11. That's where Apple made a number of significant changes. It's easy to support back to 11, much harder (and of far lesser value) to go be beyond that.

intere commented 4 years ago

Let’s leave it as is for the 1.0 baseline, but drop it for 2.0. Removing support for an entire OS release sounds like a full version change to me.