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

Implemented interfaces for Frame, Hand, Finger, Bone and Arm, along … #16

Closed
wants to merge 1 commit into from

Conversation

cherullo
Copy link

…with zero-copy implementations of TransformedCopy.
The objective was to eliminate most memory allocations that contribute to GC stutter, most of which happened on Frame.TransformedCopy.
This was achieved by creating classes which transform the original values on demand, instead of transforming and copying all the values before-hand. Instances of these classes are obtained calling TransformedShallowCopy.
Of course, these Transformed* classes have different performance characteristics, but in the demo scene, using the CapsuleHand, the aggregated performance was improved. Even thou TransformedShallowCopy is much faster than TransformedCopy, the following scripts are slower since the actual calculation is happening lazily. In the end it's a clear win.

I needed to check whether those implementations were correct, so I created an Asserter class which compares two IFrame instances (and all it's children objects). This uncovered a few bugs in the original implementations and the new Transformed classes. For example, the bug #6 reported early doesn't affect the new classes. Anyway, there is some new code in LeapProvider.Update to optionally compare Frames transformed by TransformedCopy and TransformedShallowCopy.

Once those interfaces were created, I also created Invalid versions of those objects. These implementations throw NotImplementedExceptions (should change to something more appropriate like IllegalOperationException) for every method, except IsValid and Equals. This allows a much more defensive coding style, where you MUST check IsValid before accessing an object, otherwise an exception is thrown. The static Invalid properties then were changed to return appropriate Invalid* singletons too, further reducing memory allocation.

Another huge source of memory allocations was in LeapImageRetriever. The _combinedTexture was being destroyed every frame just to be recreated. This was fixed too.

Looking at the Unity profiler, there is just a 44 byte allocation of a iterator in UpdateHandRepresentations now.

Remaining fixes and improvements:
FIX: Leap.Arm.TransformedCopy didn't normalize Direction.
FIX: Fixed FingerId propagation issue (#15)
IMP: TransformedCopy methods now receive Matrix by ref.
IMP: Methods FingerList.{Left, Front, Right}most don't allocate a temporary invalid Finger.
IMP: Methods HandList.{Left, Front, Right}most don't allocate a temporary invalid Finger.
IMP: CapsuleHand.updateCapsules performance improvements
IMP: LeapProvider.GetFixedFrame: only transform the returned frame.

…ith zero-copy implementations of TransformedCopy.
@codemercenary
Copy link

Holy macrel!

I'd like to get this merged, but I do have a request: Would you mind breaking it up a bit more? Perhaps submitting the bugfixes in a separate PR, so we can understand and comment on them separately?

@codemercenary codemercenary self-assigned this Feb 26, 2016
@codemercenary codemercenary added this to the v4.0.1 milestone Feb 26, 2016
@codemercenary
Copy link

Nevermind, looks like the work is too tied together. We'll leave it as it is.

@@ -356,9 +372,11 @@ public override string ToString ()
* @returns The invalid Finger instance.
* @since 1.0
*/
public static Finger Invalid {

Choose a reason for hiding this comment

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

We actually deprecated the Invalid concept at this layer in the new API, you should be able to just remove all of this code once we update the wrappers here.

@Amarcolina
Copy link
Contributor

This PR should target the develop branch instead of the master branch. Develop branch is for developmental work, master branch is only for releases.

@cherullo
Copy link
Author

Let me apologize for doing things in an awkward manner. I didn't know that the UnityModules repo existed, let alone that it was publicly visible. So I didn't expect to ever create a pull request, I was prepared to keep my private copy of those scripts with our changes, and periodically merge with the official releases. I was about to blow my timebox, so I rushed it even more. Next time I'll create issues to discuss possible solutions instead of dropping a PR from out of nowhere.

Also, does the C# project have automated testing? I really needed the test code in LeapProvider and the Asserter class, it helped a lot in finding bugs, but it sure shouldn't be there.

Overall, I'm glad that this PR was received with such warmth, and I'll find some time to rewrite it.

@codemercenary
Copy link

Don't worry about it at all, it's easy enough to fix, though we will need to open a new PR.

The current system is three-layer API. LeapC is on the bottom, then LeapCSharp (which is what this PR touches), and then at the top are the Unity wrappers. Originally, LeapC didn't exist, and LeapCSharp was generated code. We've always known LeapCSharp is horrible but until recently we couldn't do anything about it.

So, we spent the last two months rewriting the underlying API just so that we could change the wrapper. We actually did fix a lot of issues in LeapCSharp, but our focus was just in getting LeapC completely stable. We were about to blow our timebox for the Orion launch, so we focused all of our efforts on LeapC. LeapCSharp had bugs, but it was at least functional.

Four engineers were dedicated to this project for two months for the sole purpose of allowing us to correcting these specific garbage collection problems. And then, along comes this pull request. Holy guacamole. This is amazing.

THANK YOU for doing this, if you hadn't, it might have been another month before this stuff got out there. If this isn't a testament to the power of open source, I don't know what is.

@codemercenary
Copy link

Also, we do have unit tests. They're in a monolithic repository, though, and so I can't show you them directly. I'm working on splitting up the monolith now, when it's done I'll make that stuff open source as well.

@codemercenary
Copy link

Closing this as superceded by #21

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.

None yet

3 participants