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

Deprecate remote spatial mapping classes #39

Closed angelaHillier closed 8 years ago

angelaHillier commented 8 years ago

The preferred method for getting spatial mapping data into Unity is to use the Windows Dev Portal for capturing a 3D model of the room and then loading the meshes from that model. We're currently updating the Spatial Mapping course to show this new flow and will no longer need the following classes in HoloToolkit for sending meshes to Unity: RemoteMapping.prefab MeshSaver.cs RemoteMappingManager.cs RemoteMeshSource.cs RemoteMeshTarget.cs SimpleMeshSerializer.cs

jwittner commented 8 years ago

Do we have a clear deprecation process?

angelaHillier commented 8 years ago

Nothing official. We were thinking of marking things as depecrated in code and in the notes, then removing them from the repository after a few months. For this particular feature, I've been debating about keeping it around, or to at least move it into a separate sample repository, as it does demonstrate some basic networking and mesh serialization that we've referred many of our forum users to. Either way, I need to do some code re-write to bring in the ObjectSurfaceObserver which is meant to replace all of the code above. I'll start on that within the next week.

jwittner commented 8 years ago

Totally agree that phasing them out is a great idea, though in this case like you said there may be value in keeping it around. Maybe there's a conversation to be had around treating this project as a core for developing applications on vs treating it as a repo of useful code to learn from and leverage pieces of.

Maybe we can ramp up the sophistication of our branching when we start to do deprecation? Like release branches with SemVer style tagging? That way we have a place to merge bug fixes without pulling in breaking changes. This is more or less important depending on the outcome of my question above on how we'd like to treat the project.

riverar commented 8 years ago

Hey @angelaHillier, is this repo in sync with the academy? Seems like the spatial mapping asset in H230 is out of sync with the toolkit's asset (e.g. there's an Object Surface Observer).

angelaHillier commented 8 years ago

@riverar, No, it's out of date. I'm starting the update now. We need to remove the Spatial Mapping components (SpatialMappingCollider and SpatialMappingRenderer), since they are now a part of Unity. Add in the ObjectSurfaceObserver, and either move/remove or deprecate the Remote Spatial Mapping pieces. I'm going to be tackling these items this week, so hopefully by next week everything will be aligned again.

riverar commented 8 years ago

@angelaHillier So I've been playing with this more. Do you know how many triangles/meter HoloLens captures for its "srmesh.obj" artifact? I ask because there doesn't seem to be a way to discover or configure this value in the portal. And without the classes you're deprecating, we're doomed (as least as staging in Unity is concerned) with the default granularity. I understand we can continue using these classes today but is there a plan to re-enable this functionality in the future? Or perhaps I missed something fundamental?

NeerajW commented 8 years ago

@riverar you should be able to configure this using the SurfaceData struct in the RequestMeshAsync and pass in the triangles per cubic meter value that you want the processed data to be. But I also could not find which values are supported at the basic scanning level. Perhaps something we can fix in documentation.

riverar commented 8 years ago

@NeerajW Yup and that works great at runtime. But when gathering a mesh for local use within Unity, it seems you're stuck with the low-poly SRMesh.obj from the Device Portal (per academy docs). You can use the Observer to gather data and serialize to disk but we're then recreating the classes that just got deprecated.

Not suggesting any changes be made at this time, just trying to understand how the mesh-for-local-use scenario will play out now.

angelaHillier commented 8 years ago

You should still be able to set TrianglesPerCubicMeter for the live meshes, as I'll just be updating HoloToolkit to match what is currently available in the Spatial Mapping (230) course, and we play with triangle count there. But you're correct, you will be stuck with what the device portal records for local meshes.

I've been holding off on this work for a while, because I still feel it is valuable to keep the remote mapping stuff around, it's just a lot of code that isn't really necessary..... I think it might be better moved into a sample, instead of being part of the HoloToolkit (partly due to all of the set-up that must be done with the firewall to get the remote mapping to work).

riverar commented 8 years ago

@angelaHillier Thanks, seems like it's definitely still useful for those that want more control over local mesh generation (as it supported (de)serialization of mesh data on disk)! I'm going to poke at some of the Device Portal internals to see if there's something we can tweak there

angelaHillier commented 8 years ago

@riverar Ideally, the windows device portal should expose options to save the room at different levels of detail. I definitely don't want to remove code if people are actually using it. I'll hold off on deprecating/ removing this code, but might try to move it to a special folder while making the other spatial mapping updates (so all that extra code doesn't get too confusing for new users). We started a discussion today about how we might be able to use this code to save meshes on the HoloLens (we could load a saved room if we save off a world anchor), which would make these components more useful. We'd like to save the file as a .obj too, so Unity can load it for us.

Subere23 commented 8 years ago

I use this feature regularly. I find it very useful for iterating without having to go back and forth from unity to HoloLens.

hereswilson commented 8 years ago

@angelaHillier looking forward to being able to save the room mesh as .obj, but until then are remoteMapping/RemoteMeshTarget/RemoteMeshSource the only way to get a room file from the hololens to Unity? If so what is the correct implementation of that, meaning do both the editor and hololens need manager,source, and target? Also does it only work for an editor in play mode or would it be possible to be modified for a standalone PC unity app?

riverar commented 8 years ago

@hereswilson The current implementation relies on a mesh saved via Device Portal. Saves an .obj you can drop into Unity.

hereswilson commented 8 years ago

@riverar that's what I thought. Trying to be able to let the person using the hololens save the room mesh as an obj from inside a unity app. I was able to save it as a .room, but it doesn't work as easily to display that file in a separate Unity app.

riverar commented 8 years ago

@hereswilson Yep, agree. It's disjointed right now. I may investigate building a .obj serializer for that, if they haven't already built one.

CameronVetter commented 8 years ago

Is the plan for this to bring ObjectSurfaceObserver into the Holotoolkit? This is way easier to work with compared to FileSurfaceObserver.... I read through this whole thread and don't understand what the plan is to move forward on this... I'd like to make sure I'm not working with things that are about to be deprecated...

CameronVetter commented 8 years ago

BTW If desired I could do a PR to integrate ObjectSurfaceObserver into the Holotoolkit. I have it working fine locally.

riverar commented 8 years ago

Here's my understanding:

Current HoloTookit snapshot (uses RemoteMesh[...] and [...]MeshSerializer code)

Academy snapshot (https://github.com/Microsoft/HolographicAcademy/commit/7a858ad06a618e6c88a4c71c0c646fedc5aeb76d)

In my projects, it was simple enough to nuke the old code and integrate the diff from the academy repo. @angelaHillier indicated a greater plan to perhaps keep this stuff around, so have been holding off submitting PRs. She probably has a push coming to clean this up any second now. 🍬

angelaHillier commented 8 years ago

Hey everyone, Sorry for the delay. We were swamped with hackathon and live academies last week, so I didn't have the chance to submit an update.

We are aware that the 'HoloToolkit' used in our Academy courses is different from this one. We're trying to figure out a good way to differentiate between them (probably rename the folders in the Academy courses), as the HoloToolkit has changed a lot since its initial inception (yeah! Community is awesome!) and it's hard to keep them in sync. I do try to keep the spatial mapping bits aligned though....

Here is the current plan: Bring over ObjectSurfaceObserver and make changes to SpatialMappingManager to support it.

Move (but not remove) the remote mapping components. There have been enough people who have reached out to say this is useful, so we won't deprecate it.

The basic SpatialMapping prefab will include the ObjectSurfaceObserver instead of the FileSurfaceObserver.

You should expect to see a PR from me for the inclusion of the SurfaceObserver within the next two days.

Thanks! ~Angela

angelaHillier commented 8 years ago

I have a PR out with changes related to this issue. Basically, the RemoteMapping stuff will be sticking around, but the following changes are going in: 1) All scripts related to saving/loading/sending meshes over the network are moving to a new folder (SpatialMapping\Scripts\RemoteMapping). 2) The FileSurfaceObserver was removed from the SpatialMapping prefab (which now uses the ObjectSurfaceObserver for loading room models instead). 3) The RemoteMapping prefab now includes the FileSurfaceObserver. 4) Files saved via remote mapping will not be loaded automatically by the SpatialMappingManager. You can still do this in code, or press the 'L' key in Editor to load. Also, when running in the editor, it won't automatically switch to the network source (press the 'N' key to receive meshes over the network and then say 'send meshes' as before). 5) A RemoteMapping test scene was added and documentation updated to help with usage.

jwittner commented 8 years ago

Was this fixed by #151?

angelaHillier commented 8 years ago

Yes check-in #151 addresses this. We decided to move 'RemoteMapping' into a separate folder instead of deprecating it. All functionality is still there, but the SpatialMappingManager will not use these components directly (so you have to manually press the 'N' key while running in the Unity Editor to enable network meshes, say 'Send Meshes' to send them, press the 'S' key to save them, and then press the 'L' key to load a saved mesh from file). I am considering adding automatic loading of these files if the ObjectSurfaceObserver does not have a valid file when running in the Editor.