microsoft / MixedRealityToolkit-Unity

This repository is for the legacy Mixed Reality Toolkit (MRTK) v2. For the latest version of the MRTK please visit https://github.com/MixedRealityToolkit/MixedRealityToolkit-Unity
https://aka.ms/mrtkdocs
MIT License
6k stars 2.12k forks source link

Bounding Box Refactor #5340

Closed thalbern closed 4 years ago

thalbern commented 5 years ago

Describe the bug

BoundingBox is one of the most commonly used components in MRTK but has over time grown into a huge monolithic class that is hard to maintain and understand for developers and users. BoundingBox refactor should

Refactor breakdown:

related #6260, #6261

Alexees commented 5 years ago

I just want to say something here:

This is just the first time out of 3 times I had to repair the BoundingBox, now actually contributing the change to the source. The first BoundingBox from the Hololens 2 years ago did not work very well when upside down and needed fixing. The second Box from MRDL just did not differentiate between Hololens Hand Movement and the MR Motion Controllers, and needed fixing. It then moved over officially and had those same problems of course and needed fixing. Now the current implementation is the closest to being fully functional.

So please please please take extra care whoever does this. This box has been broken long enough now and nobody cared.

Cameron-Micka commented 5 years ago

This certainly ranks at the top of my list of commonly used components that need some love. Looking forward to a refactor!

Troy-Ferrell commented 5 years ago

There are quite a few issues already filed on boundingbox so this is a great idea. #5111 , #5038 , #5006 , #4901 , #4755

Alexees commented 5 years ago

Wanted to add a couple of things that I realized while fixing bounds calculation:

Alexees commented 5 years ago

I'm working on my fourth point right now and it looks good. I'll report back when I have something to show

https://github.com/HoloSpaces/MixedRealityToolkit-Unity/tree/mrtk_betterBBLogic

Cameron-Micka commented 5 years ago

Thank you @alexees I agree that all of those bullet points should be addressed at some point. (And, have been pain points for developers.)

Alexees commented 5 years ago

Hey you guys,

I spent a couple of days to come up with an improved version of a BoundingBox rotation. Before going to 3D-fiying the logic, I wanted you to have a look, if what you see is what you would expect when using it. Features are:

The white circle represents the circular motion vector of one handle you can grab. The blue line with blue circle represents a motion controller with its beam and the cursor at the distance by the time of the handle grab. The green dot is the handle. The green line appears when the box overextends.

ezgif com-video-to-gif

thalbern commented 4 years ago

closing this now - open individual tickets for more work on bounds control - graduation is tracked here https://github.com/microsoft/MixedRealityToolkit-Unity/issues/7358