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

Directions (Routes) and AR Polylines #159

Merged
merged 33 commits into from
Feb 28, 2019

Conversation

intere
Copy link
Collaborator

@intere intere commented Feb 18, 2019

Attempt at fixing merge conflicts with #26.

The original update info is:

  • a lot of code refactored
  • added ability to display routes on AR view
  • added a PR template
  • added a .travis.yml to lint the pod
  • added a 'changelog.md' file to track library updates / changes

TODO

  • Factor LocationNode and AnnotationNode out of LocationAnnotationNode.swift and back into LocationNode.swift
  • Verify pod installation in an external project (I'll use my ARCL branch in GeoTrackKit for this)
  • Test the routing (I'll use my ARCL branch in GeoTrackKit for this)
  • Add a UI to test polyline routing in the example app

Updates

  • updates the swiftlint rules
    • addresses many swiftlint issues in code
  • makes many things more "swifty" (forEach function blocks instead of for loops for example)
  • updated the ARCL.podspec file to contain the swift_version
  • removed .swift_version file (Podspec is now responsible for it)
  • adds the ability to display a route in the AR view

Checklist

  • Appropriate label has been added to this PR (i.e., Bug, Enhancement, etc.).
  • Documentation has been added to all open, and public scoped methods and properties.
  • Changelog has been updated
  • Deferred Tests have have been added to all new features. (not a requirement, but helpful)
  • Image/GIFs have been added for all UI related changed.

Screenshots

First Demo: the polyline route demo:
route_polyline_demo

Second Demo: the (original) annotations
point_annotations

@intere intere added enhancement under construction not ready to be merged yet, still in progress labels Feb 18, 2019
@intere intere self-assigned this Feb 18, 2019
@intere intere mentioned this pull request Feb 19, 2019
@aaronbrethorst aaronbrethorst changed the title Polylines Not for Merge: Polylines Feb 19, 2019
Copy link
Collaborator

@aaronbrethorst aaronbrethorst left a comment

Choose a reason for hiding this comment

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

Generally looks good. I noticed a few stylistic things. I'll put time into functional testing on a device later today.

ARCL/Source/Nodes/LocationNode.swift Outdated Show resolved Hide resolved
ARCL/Source/Nodes/PolylineNode.swift Outdated Show resolved Hide resolved
ARCL/Source/Nodes/PolylineNode.swift Outdated Show resolved Hide resolved
@available(iOS 11.0, *)
public extension SceneLocationViewEstimateDelegate {
func didAddSceneLocationEstimate(sceneLocationView: SceneLocationView, position: SCNVector3, location: CLLocation) {
//
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need to exist?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The idea is that they are default implementations of the functions (essentially making them optional when conforming to the protocol). They don't have to exist, but by existing, they allow you to choose which functions you want to implement and not have no-op implementations for the ones you don't want to implement.

@available(iOS 11.0, *)
public extension SceneLocationViewDelegate {
func didAddSceneLocationEstimate(sceneLocationView: SceneLocationView, position: SCNVector3, location: CLLocation) {
//
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this need to exist?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The idea is that they are default implementations of the functions (essentially making them optional when conforming to the protocol). They don't have to exist, but by existing, they allow you to choose which functions you want to implement and not have no-op implementations for the ones you don't want to implement.

ARKit+CoreLocation/ViewController.swift Outdated Show resolved Hide resolved
ARKit+CoreLocation/ViewController.swift Outdated Show resolved Hide resolved
…print statements, though; not CocoaLumberjack).
…ng them in AR, however I haven't wired it up to the map yet.
@intere
Copy link
Collaborator Author

intere commented Feb 20, 2019

@aaronbrethorst - thanks for the feedback. I've responded and/or updated based on your feedback. I have started working on updating the UI and have hacked together the ability to specify a location, search for it, get directions and see those directions as a polyline in AR, however the settings UI is janky and I need to clean it up and polish it more.

@intere intere removed the under construction not ready to be merged yet, still in progress label Feb 22, 2019
@intere intere changed the title Not for Merge: Polylines Directions (Routes) and AR Polylines Feb 22, 2019
Copy link
Collaborator

@aaronbrethorst aaronbrethorst left a comment

Choose a reason for hiding this comment

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

looks 👍 awesome work!

@aaronbrethorst aaronbrethorst merged commit 8698199 into AndrewHartAR:master Feb 28, 2019
@intere intere deleted the polylines branch April 29, 2019 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants