-
Notifications
You must be signed in to change notification settings - Fork 484
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
Architecture and functional changes to gltf repo to divide into components #39
Conversation
Merging in changes from HoloToolkit (kurtie@microsoft.com) to add UWP support
Beginning of work to separate out code into multiple projects. Moved Unity into it's own folder, and created base projects for non-Unity dll
…ayer Moved files that should go in C# layer from Unity layer
…ling [+] Added color class to store color data and provide some constant values [~] Switched over classes to use System.Numerics [+] Added tests to verify that JSON parses correctly [~] Added basic stream functionality [~] Made load byte buffer an overload of the loader [~] Reworked the loader API [~] Added todo's to get working in Unity
[~] Added stream loader option for IRandomAccessStream [~] Made UWP project use proper files [~] Added Newtonsoft.JSON nuget for UWP project [~] Added UWP unit test
…ionality working again [-] Removed async code from non-UWP version [+] Added AssetCache to Unity layer which keeps track of loaded assets needed to construct a scene [-] Removed all data caching from base layer [~] Added IEnumerator functionality back to Unity layer [~] Moved BuildMeshAttributes to GLTFLoader [~] Renamed Unity's GLTFLoader to GLTFUnityLoader [-] Removed System.Numerics and replaces it with basic storage classes [~] Created helper function to convert the basic storage classes
[~] reenabled material caching [~] Fixed various bugs that caused files not to load [~] Hooked up first step stream support for unity layer [-] Removed loading API's from C# layer [~] Made all build targets AnyCPU (unit test UWP requires x86 or x64 so there is a custom for that) [~] Readded glb support [~] Fixed disabled tests [~] Made serialize and deserialize api's work with TextReader and TextWriter
Wow, this is a huge PR! Kind of hard to review something that touches every file in the project. Could you explain some of these to me? Specifically:
|
Hey Stephen,
I emailed Robert about the change last night, we had also talked a bit before about the changes that I am proposing, whatever is the best way to review the changes I am up for. The architecture changes are separating them out into a dll that simple does schema parsing, and having the scene construction be what is in the Unity layer (and all the memory for scene construction living there too). The data structure now for scene construction is in the unity layer.
I can separate my changes into a three (or more) PR’s:
1. The change of moving the parsing code to its own C# project/dll
2. The proposed naming changes we would like to make
3. The additional changes made on top of it
Does that sound good?
The reasoning behind these changes are:
1. We wanted to be able to handle the parsing of the glTF JSON in an engine abstract layer
2. We wanted to be able to control when things are loaded/parsed
The changes I am planning on making still are:
1. Adding some more API’s to the Unity layer to allow more modular scene construction (so that we can request that the library just builds a mesh, but then we have our own layer that builds the materials)
2. Doing an optimization pass
3. Work on extras serialization
…-Blake
From: Steven Vergenz [mailto:notifications@github.com]
Sent: Saturday, August 19, 2017 10:59 AM
To: KhronosGroup/UnityGLTF <UnityGLTF@noreply.github.com>
Cc: Blake Gross <blgross@microsoft.com>; Author <author@noreply.github.com>
Subject: Re: [KhronosGroup/UnityGLTF] Architecture and functional changes to gltf repo to divide into components (#39)
Wow, this is a huge PR! Kind of hard to review something that touches every file in the project. Could you explain some of these to me? Specifically:
1. Scene construction and schema parsing were already separated, weren't they? All the construction work happened in GLTFLoader, and the schema classes were only for de/serializing and storage.
2. If I understand your second bullet, the schema classes are now strictly de/serializing, and the data is stored in Unity? So there's no intermediate data structure anymore?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FKhronosGroup%2FUnityGLTF%2Fpull%2F39%23issuecomment-323538298&data=02%7C01%7Cblgross%40microsoft.com%7Cfb5863c2a5a54a8636cd08d4e72bf6e2%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636387623394729198&sdata=BvkuPbl8KHAnT3vPdkR7OkbV6uD17nMX1yOzZXAZwQI%3D&reserved=0>, or mute the thread<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAdqpN_7PHJfnFzABYoFOEE7ua5-YfdYEks5sZyJhgaJpZM4O8Oq9&data=02%7C01%7Cblgross%40microsoft.com%7Cfb5863c2a5a54a8636cd08d4e72bf6e2%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636387623394729198&sdata=DjaxiBd2fC88ATwnGujo5rylpmtYpmubNQG0RCvGFDw%3D&reserved=0>.
|
Thanks for making this PR Blake! There's a lot of good stuff in here. Excited to see changes that bring UWP support. I'm a big fan of splitting out the JSON serialization/deserialization into a separate DLL. There's really no need to have JSON serialization/deserialization in the Unity project and a lot of benefit to those using C# in separating it from the Unity loader. @bghgary commented in our email exchange about using glTF-CSharp-Loader for serialization/deserialization. It has some benefits like generating its parser from the glTF JSON Schema, but it's also considerably slower because it generates classes and then uses JSON.NET's reflection based parsing. I did some testing on the reflection based parsing in #10 and I don't think we should return to that method. However, we could modify glTF-CSharp-Loader to use the manual JSON.NET serialization/deserialization that we have in UnityGLTF. I'm starting to think that this is the best path forward. It would give a huge speed boost to anyone already using the C# loader, UnityGLTF could focus solely on Unity related things and we could avoid duplicated efforts. I'm happy to put in the work to make this happen if we decide this is a good path forward. |
Modifying the glTF CSharp Loader to support manual serialization/deserialization sounds fine to me. So would the work flow then be:
|
[/] Created a .NET schema parsing project and a Unity project to have as separation between parsing and scene construction. This involved moving around files to various directories
[-] Removed all data storage from schema layer. Moved the data storage to be in unity layer
[+] Added in support to load all data from file streams
[+] Added support for building on the UWP platform
[/] Added unit test for .NET schema parser
[/] Refactored API names and moved position of API's to make logic clearer