[Dotnet] port of ScanningAndDetecting3DObjects #435
Conversation
stephen-hawley
changed the title
|
|
||
| I occasionally refactored the Swift code where I thought it helped legibility or reduced repetition. Usually this was in the form of extracting a function. | ||
|
|
||
| One area where there is considerably variance from the Swift code is in the "Utility_Extensions" folder. Xamarin.iOS uses OpenTK vector and matrix classes for manipulating 3D objects and transforms. The OpenTK classes have different APIs and |
| // Output is: "(1, 0, 0, 0)\n(0, 1, 0, 0)\n(0, 0, 1, 0)\n(2, 3, 4, 1)" | ||
| ``` | ||
|
|
||
| As you can see, the position is encoded in the bottom row's first 3 elements. This might be transposed (sideways) to your expectations. The functions `SCNMatrix4 ToSCNMatrix4(this NMatrix4 self)` and `NMatrix4 ToNMatrix4(this SCNMatrix4 self)` use the `M{col}{row}` fields to assure the values are proper, so that's fine, but there _is_ a small performance cost to changing the representation, so avoid needless conversions. |
There was a problem hiding this comment.
The managed SCNMatrix4 type will change in .NET (xamarin/xamarin-macios#13695), so maybe we'll want to wait with completing this sample until the sample can be updated according to those changes?
| internal new void Dispose () | ||
| { | ||
| Dispose (true); | ||
| GC.SuppressFinalize (this); | ||
| } | ||
|
|
There was a problem hiding this comment.
Implementing a new Dispose method shouldn't needed, this is what our does:
So I think this should be removed (it also happens in at least one other file).
There was a problem hiding this comment.
I think I'm just going to remove these. They were sketchy anyway. At least one was marked as being put in for debugging.
| disposed = true; | ||
| if (disposing) | ||
| { | ||
| base.Dispose (); |
There was a problem hiding this comment.
The Dispose pattern is wrong, the Dispose (bool) method should call the same base method, so:
base.Dispose (disposing);
This effectively changes the whole method, and the comment in the README about Xamarin's weird Dispose pattern is also wrong (the original author got the Dispose pattern wrong).
This method would become:
if (!disposed)
{
disposed = true;
NSNotificationCenter.DefaultCenter.RemoveObserver (notificationObserverHandle);
Origin?.Dispose ();
BoundingBox?.Dispose ();
GhostBoundingBox?.Dispose ();
}
base.Dispose (disposing);This occurs in other files as well.
|
|
||
| void ScanningStateChanged (NSNotification notification) | ||
| { | ||
| var scanState = notification.UserInfo? [Scan.StateUserInfoKey] as SimpleBox<Scan.ScanState>; |
There was a problem hiding this comment.
I think this one would be a good candidate for an is operator!
| case Axis.Y: return new [] { sides [BoundingBoxSide.PositionName.Top], sides [BoundingBoxSide.PositionName.Bottom] }; | ||
| case Axis.Z: return new [] { sides [BoundingBoxSide.PositionName.Front], sides [BoundingBoxSide.PositionName.Back] }; | ||
| } | ||
| throw new ArgumentOutOfRangeException ("axis"); |
There was a problem hiding this comment.
| throw new ArgumentOutOfRangeException ("axis"); | |
| throw new ArgumentOutOfRangeException (nameof (axis)); |
|
|
||
| foreach (var result in hitResults) | ||
| { | ||
| var hitAxis = result.Node.ParentNode as ObjectOriginAxis; |
There was a problem hiding this comment.
Maybe another good place to use is operator
| // Perform a hit test against the feature point cloud. | ||
| var result = sceneView.SmartHitTest (ViewController.Instance.ScreenCenter); | ||
| if (result is null) | ||
| { |
There was a problem hiding this comment.
Not sure if worth going through many of these, but mono guidelines recommend if () { as well as the else statement
|
|
||
| void ScanningStateChanged (NSNotification notification) | ||
| { | ||
| var state = notification.UserInfo [Scan.StateUserInfoKey] as SimpleBox<Scan.ScanState>; |
|
|
||
| void ScanningStateChanged (NSNotification notification) | ||
| { | ||
| var state = notification.UserInfo? [Scan.StateUserInfoKey] as SimpleBox<Scan.ScanState>; |
| Opacity = (nfloat) newOpacity; | ||
| } | ||
| } | ||
| } |
| { | ||
| DetectedObject?.RemoveFromParentNode (); | ||
|
|
||
| if (sceneView.Session.Configuration as ARWorldTrackingConfiguration is not null) |
There was a problem hiding this comment.
| if (sceneView.Session.Configuration as ARWorldTrackingConfiguration is not null) | |
| if (sceneView.Session.Configuration as ARWorldTrackingConfiguration) |
Think this is redundant
| startOverButton = new UIBarButtonItem ("Restart", UIBarButtonItemStyle.Plain, restartButtonTapped); | ||
|
|
||
| var navigationItem = new UINavigationItem ("Start"); | ||
| navigationItem.LeftBarButtonItem = backButton; |
There was a problem hiding this comment.
Can these be added to the constructor?
|
Microsoft support for Xamarin will end on May 1, 2024 for all Xamarin SDKs. In preparation for this, all issues and PRs in this repository are being closed. |
This is a huge sample, thanks for your patience.