Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue 22 - ARSCNViewDelegate Handling Content Updates #25

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix issue 22 - ARSCNViewDelegate Handling Content Updates #25

wants to merge 3 commits into from

Conversation

farandal
Copy link

For review,

This is the solution that worked for me
#22

Cheers!

@farandal
Copy link
Author

@boundsj

Copy link
Contributor

@boundsj boundsj left a comment

Choose a reason for hiding this comment

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

Thanks!

In addition to the changes I requested, it'd be great if you added some example usages of these new protocol methods in the demo app.

@@ -7,6 +7,11 @@ import CoreLocation
@objc optional func node(for annotation: Annotation) -> SCNNode?
@objc optional func session(_ session: ARSession, cameraDidChangeTrackingState camera: ARCamera)

@objc optional func MBRenderer(_ renderer: SCNSceneRenderer, time: TimeInterval)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use s/MBRender/renderer in all of these new methods.

@@ -85,6 +90,19 @@ extension AnnotationManager: ARSCNViewDelegate {
delegate?.session?(session, cameraDidChangeTrackingState: camera)
}


public func renderer(_ renderer: SCNSceneRenderer, updateAtTime time: TimeInterval) {
delegate?.MBRenderer!(renderer, time: time)
Copy link
Contributor

@boundsj boundsj Oct 25, 2017

Choose a reason for hiding this comment

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

This will cause a crash if the delegate is not nil but does not implement this method. This and all other calls should not force unwrap and should use optional chaining instead. MBRenderer should be changed to renderer here at the call site, too, after you make the change noted above.

@farandal
Copy link
Author

Hello @boundsj,

I have changed the implementation approach.
I personally don't like very much the Idea that the AnnotationManager implements the ARSCNViewDelegates as part of the MapboxARKit, as I cannot implement the ARSCNViewDelegates later in the View. What I did additionally to your review, is basically delegate different methods that notify the Anchor detection updates depending on the type of the the Anchor, in this case ARAnchor and MBARAnchor. Maybe there is a better way to implement this, but for now, this works for me.

@farandal
Copy link
Author

class ViewController: UIViewController, ARSCNViewDelegate, AnnotationManagerDelegate {
...

func renderer(_ renderer: SCNSceneRenderer, addedNode: SCNNode, arAnchor: ARAnchor) {
    //print("Added Node Notification, Native ARSCNViewDelegate delegated from Anotation Manager for ARAnchors")
}

func renderer(_ renderer: SCNSceneRenderer, addedNode: SCNNode, mbAnchor: MBARAnchor) {
    print("Added Node Notification, delegated from MB Anotation Manager for MBARAnchors")
}

...
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants