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

Enable cancelling localization sessions #247

Merged
merged 14 commits into from Oct 22, 2019

Conversation

chrisfromwork
Copy link
Contributor

@chrisfromwork chrisfromwork commented Oct 17, 2019

This review addresses: #242

In this review:

  1. ILocalizationSession is extended to have a Cancel function defined. This is beneficial because it eases cleanup when a network connection is lost.
  2. SpatialLocalizationSession was created as a base class. This base class contains cancellation token sources for tearing down localization on cancel.
  3. Dispatcher and DispatcherUnity were created to ease scheduling tasks on the ui thread.
  4. We now lock when accessing the current localization session in SpatialCoordinateSystemManager.
  5. SpatialCoordinateSystemManager now has a flag that states whether localization is running.
  6. SpatialAlignmentVisual now displays to the user when localization is running.

Breaking Change Details:

Notes:

This review introduces breaking changes to ISpatialLocalizationSession. Contributors felt there was a need to be able to cancel sessions, which led to this change.

Migration Instructions:

  1. Update custom implementations of ISpatialLocalizationSession to support a Cancel function.

@chrisfromwork chrisfromwork added the breaking change Pull request contains a breaking change and requires additional information. label Oct 17, 2019
Copy link
Contributor

@andreiborodin andreiborodin left a comment

Choose a reason for hiding this comment

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

:shipit:

private ITrackingObserver trackingObserver = null;

private readonly object localizationLock = new object();
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to prefer a separate lock object? If this is only protecting currentLocalizationSession, would we lock that object directly instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

locking on a null object throws exceptions. Since we want to populate currentLocalizationSession within the lock, it seems safer to lock on a separate, non-null object.

@chrisfromwork chrisfromwork merged commit fc5e31e into microsoft:master Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Pull request contains a breaking change and requires additional information.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants