From 6af19178808f3b3896c2bc6a14c144ef341c848e Mon Sep 17 00:00:00 2001 From: eeropomell Date: Sat, 7 Dec 2024 01:42:24 +0200 Subject: [PATCH 01/10] add clarifying comments to ModelWidget --- Assets/Scripts/Widgets/ModelWidget.cs | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/Assets/Scripts/Widgets/ModelWidget.cs b/Assets/Scripts/Widgets/ModelWidget.cs index c8c74ef424..7407659a64 100644 --- a/Assets/Scripts/Widgets/ModelWidget.cs +++ b/Assets/Scripts/Widgets/ModelWidget.cs @@ -13,9 +13,11 @@ // limitations under the License. using System; +using System.Collections.Generic; using UnityEngine; using System.Linq; using System.Threading.Tasks; +using UnityEditor; namespace TiltBrush { @@ -29,6 +31,18 @@ public class ModelWidget : MediaWidget [SerializeField] private float m_MaxBloat; private Model m_Model; + + + // What is Subtree? + // e.g. if we have 3d model with 3 chairs with the hierarchy below, + // then when the model is broken apart, we create a separate ModelWidget for each Chair1,Chair2,Chair3 + // e.g for Chair1, Subtree = "Root/Chair1" + /* + Root (empty node) + Chair1 (mesh) + Chair2 (mesh) + Chair3 (mesh) + */ private string m_Subtree; public string Subtree { @@ -245,7 +259,7 @@ void LoadModel() m_ModelInstance = Instantiate(m_Model.m_ModelParent); m_ModelInstance.gameObject.SetActive(true); m_ModelInstance.parent = this.transform; - + Coords.AsLocal[m_ModelInstance] = TrTransform.identity; float maxExtent = 2 * Mathf.Max(m_Model.m_MeshBounds.extents.x, Mathf.Max(m_Model.m_MeshBounds.extents.y, m_Model.m_MeshBounds.extents.z)); @@ -322,13 +336,16 @@ public bool HasSubModels() return false; } + // Update the transform hierarchy of this ModelWidget to only contain m_Subtree + // e.g if Subtree = "CarBody/Floor/Wheel1", then this method will update the transform hierarchy to contain nodes + // starting at CarBody/Floor/Wheel1 public void SyncHierarchyToSubtree(string previousSubtree = null) { if (string.IsNullOrEmpty(Subtree)) return; // Walk the hierarchy and find the matching node Transform oldRoot = m_ObjModelScript.transform; Transform node = oldRoot; - + // We only want to walk the new part of the hierarchy string subpathToTraverse; if (!string.IsNullOrEmpty(previousSubtree)) @@ -355,9 +372,9 @@ public void SyncHierarchyToSubtree(string previousSubtree = null) bool excludeChildren = false; if (subpathToTraverse.EndsWith(".mesh")) { - subpathToTraverse = subpathToTraverse.Substring(0, subpathToTraverse.Length - 5); + subpathToTraverse = subpathToTraverse.Substring(0, subpathToTraverse.Length - ".mesh".Length); excludeChildren = true; - } + } if (node.name == subpathToTraverse) { // We're already at the right node @@ -366,7 +383,7 @@ public void SyncHierarchyToSubtree(string previousSubtree = null) } else { - // node will be null if not found + // - node will be null if not found node = node.Find(subpathToTraverse); } From ad3fa0d53afc86ab021e626f133ba2a885d0eba2 Mon Sep 17 00:00:00 2001 From: eeropomell Date: Sat, 7 Dec 2024 01:45:47 +0200 Subject: [PATCH 02/10] add initial version for a map from unique id to each node in a Model --- Assets/Scripts/Model.cs | 54 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/Assets/Scripts/Model.cs b/Assets/Scripts/Model.cs index 38a7d8b19a..a64510a2e2 100644 --- a/Assets/Scripts/Model.cs +++ b/Assets/Scripts/Model.cs @@ -248,6 +248,18 @@ public IExportableMaterial GetExportableMaterial(Material material) return m_ImportMaterialCollector.GetExportableMaterial(material); } + public struct UidToNodeMapItem + { + public Transform nodeTransform; + public string nodeRealName; + } + + // initialized when model is loaded + // - initialization uses GetInstanceID() to get unique id for each node in the model + // - this dictionary is currently used for finding subtrees when breaking models apart, in ModelWidget.cs + // - unique identifiers for nodes are needed because node names in e.g., glTF aren't unique + public Dictionary UidToNodeMap; + public Model(Location location) { m_Location = location; @@ -752,6 +764,8 @@ private async Task StartCreatePrefab(GameObject go) // If we pulled this from Icosa, it's going to be a gltf file. Task t = LoadGltf(warnings); await t; + + } else if (ext == ".fbx" || ext == ".obj") { @@ -856,6 +870,8 @@ public void EndCreatePrefab(GameObject go, List warnings) UnityEngine.Object.Destroy(m_ModelParent.gameObject); } m_ModelParent = go.transform; + + InitializeUidToNodeMap(m_ModelParent); // !!! Add to material dictionary here? @@ -864,6 +880,44 @@ public void EndCreatePrefab(GameObject go, List warnings) } + + + public string GetNodeRealNameFromID(int uid) + { + if (UidToNodeMap.ContainsKey(uid)) + { + return UidToNodeMap[uid].nodeRealName; + } + return null; + } + + private void InitializeUidToNodeMap(Transform rootNode) + { + // the immediate children of rootNode are the root nodes of the model + + UidToNodeMap = new Dictionary(); + + void ProcessNode(Transform node) + { + UidToNodeMapItem uidToNodeMapItem = new UidToNodeMapItem(); + uidToNodeMapItem.nodeTransform = node; + uidToNodeMapItem.nodeRealName = node.name; + node.name = node.gameObject.GetInstanceID().ToString(); + UidToNodeMap[node.gameObject.GetInstanceID()] = uidToNodeMapItem; + + foreach (Transform child in node) + { + ProcessNode(child); + } + } + + foreach (Transform child in rootNode) + { + ProcessNode(child); + } + } + + public void UnloadModel() { if (m_builder != null) From 5de976f64c39401ca7d2fd9f039ec8d98468d264 Mon Sep 17 00:00:00 2001 From: eeropomell Date: Sat, 7 Dec 2024 01:49:54 +0200 Subject: [PATCH 03/10] switch from default string concat to StringBuilder as it's faster --- Assets/Scripts/Commands/BreakModelApartCommand.cs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/Assets/Scripts/Commands/BreakModelApartCommand.cs b/Assets/Scripts/Commands/BreakModelApartCommand.cs index 0ee683fde4..19f5e49bbe 100644 --- a/Assets/Scripts/Commands/BreakModelApartCommand.cs +++ b/Assets/Scripts/Commands/BreakModelApartCommand.cs @@ -14,6 +14,7 @@ using System.Collections.Generic; using System.Linq; +using System.Text; using UnityEngine; namespace TiltBrush @@ -167,13 +168,16 @@ public BreakModelApartCommand(ModelWidget initialWidget, BaseCommand parent = nu private static string GetHierarchyPath(Transform root, Transform obj) { - string path = "/" + obj.name; + StringBuilder stringBuilder = new StringBuilder(); + stringBuilder.Insert(0, "/" + obj.name); + while (obj.transform.parent != root) { obj = obj.transform.parent; - path = "/" + obj.name + path; + stringBuilder.Insert(0, "/" + obj.name); } - return path; + + return stringBuilder.ToString(); } protected override void OnRedo() From a14492b11fd345d4bc882f4a99bf278f2e6482d6 Mon Sep 17 00:00:00 2001 From: eeropomell Date: Sat, 7 Dec 2024 01:54:58 +0200 Subject: [PATCH 04/10] remove unnecessary using directive --- Assets/Scripts/Widgets/ModelWidget.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/Assets/Scripts/Widgets/ModelWidget.cs b/Assets/Scripts/Widgets/ModelWidget.cs index 7407659a64..f70d664276 100644 --- a/Assets/Scripts/Widgets/ModelWidget.cs +++ b/Assets/Scripts/Widgets/ModelWidget.cs @@ -17,7 +17,6 @@ using UnityEngine; using System.Linq; using System.Threading.Tasks; -using UnityEditor; namespace TiltBrush { From 219270c98e221957e97a437b15bf3bd67822c246 Mon Sep 17 00:00:00 2001 From: eeropomell Date: Sat, 7 Dec 2024 02:53:24 +0200 Subject: [PATCH 05/10] [CI BUILD DEV] [CI BUILD] From d8aa0c7c2cf139c8cce5d6a96a47d944cb59c4b0 Mon Sep 17 00:00:00 2001 From: eeropomell Date: Sat, 7 Dec 2024 16:33:34 +0200 Subject: [PATCH 06/10] add profile markers for profiling Model.cs [CI BUILD DEV] --- Assets/Scripts/Model.cs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Assets/Scripts/Model.cs b/Assets/Scripts/Model.cs index a64510a2e2..62eb6fbe57 100644 --- a/Assets/Scripts/Model.cs +++ b/Assets/Scripts/Model.cs @@ -21,6 +21,7 @@ using System.Linq; using System.Threading.Tasks; using TiltBrushToolkit; +using Unity.Profiling; using Unity.VectorGraphics; using Debug = UnityEngine.Debug; using UObject = UnityEngine.Object; @@ -852,9 +853,12 @@ private void CalcBoundsNonGltf(GameObject go) } } + + static ProfilerMarker _endCreatePrefabPerfMarker = new ProfilerMarker("Model.EndCreatePrefab"); public void EndCreatePrefab(GameObject go, List warnings) { + if (go == null) { m_LoadError = m_LoadError ?? new LoadError("Bad data"); @@ -871,7 +875,11 @@ public void EndCreatePrefab(GameObject go, List warnings) } m_ModelParent = go.transform; + _endCreatePrefabPerfMarker.Begin(); + InitializeUidToNodeMap(m_ModelParent); + + _endCreatePrefabPerfMarker.End(); // !!! Add to material dictionary here? From ea48923d894ceba71310b999c98fc0a3cf62a014 Mon Sep 17 00:00:00 2001 From: eeropomell Date: Sun, 8 Dec 2024 02:12:28 +0200 Subject: [PATCH 07/10] Simplify unique node name generation logic This commit improves developer experience as the original node name is kept visible, and the UID is appended to the end [CI BUILD DEV] [CI BUILD] --- Assets/Scripts/Model.cs | 61 ++++++++++++++--------------------------- 1 file changed, 21 insertions(+), 40 deletions(-) diff --git a/Assets/Scripts/Model.cs b/Assets/Scripts/Model.cs index 62eb6fbe57..2ba6d315cc 100644 --- a/Assets/Scripts/Model.cs +++ b/Assets/Scripts/Model.cs @@ -248,18 +248,8 @@ public IExportableMaterial GetExportableMaterial(Material material) EnsureCollectorExists(); return m_ImportMaterialCollector.GetExportableMaterial(material); } + - public struct UidToNodeMapItem - { - public Transform nodeTransform; - public string nodeRealName; - } - - // initialized when model is loaded - // - initialization uses GetInstanceID() to get unique id for each node in the model - // - this dictionary is currently used for finding subtrees when breaking models apart, in ModelWidget.cs - // - unique identifiers for nodes are needed because node names in e.g., glTF aren't unique - public Dictionary UidToNodeMap; public Model(Location location) { @@ -854,8 +844,6 @@ private void CalcBoundsNonGltf(GameObject go) } - static ProfilerMarker _endCreatePrefabPerfMarker = new ProfilerMarker("Model.EndCreatePrefab"); - public void EndCreatePrefab(GameObject go, List warnings) { @@ -875,11 +863,16 @@ public void EndCreatePrefab(GameObject go, List warnings) } m_ModelParent = go.transform; - _endCreatePrefabPerfMarker.Begin(); - - InitializeUidToNodeMap(m_ModelParent); + #if DEVELOPMENT_BUILD || UNITY_EDITOR + ProfilerMarker generateUniqueNamesPerfMarker = new ProfilerMarker("Model.GenerateUniqueNames"); + generateUniqueNamesPerfMarker.Begin(); + #endif + + GenerateUniqueNames(m_ModelParent); - _endCreatePrefabPerfMarker.End(); + #if DEVELOPMENT_BUILD || UNITY_EDITOR + generateUniqueNamesPerfMarker.End(); + #endif // !!! Add to material dictionary here? @@ -887,41 +880,29 @@ public void EndCreatePrefab(GameObject go, List warnings) DisplayWarnings(warnings); } - - - public string GetNodeRealNameFromID(int uid) - { - if (UidToNodeMap.ContainsKey(uid)) - { - return UidToNodeMap[uid].nodeRealName; - } - return null; - } + - private void InitializeUidToNodeMap(Transform rootNode) + // This method is called when the model has been loaded and the node tree is available + // This method is necessary because (1) nodes in e.g glTF files don't need to have unique names + // and (2) there's code in at least ModelWidget that searches for specific nodes using node names + private static void GenerateUniqueNames(Transform rootNode) { - // the immediate children of rootNode are the root nodes of the model - - UidToNodeMap = new Dictionary(); - void ProcessNode(Transform node) + void SetUniqueNameForNode(Transform node) { - UidToNodeMapItem uidToNodeMapItem = new UidToNodeMapItem(); - uidToNodeMapItem.nodeTransform = node; - uidToNodeMapItem.nodeRealName = node.name; - node.name = node.gameObject.GetInstanceID().ToString(); - UidToNodeMap[node.gameObject.GetInstanceID()] = uidToNodeMapItem; - + // GetInstanceID returns a unique ID for every GameObject during a runtime session + node.name += " uid: " + node.gameObject.GetInstanceID(); + foreach (Transform child in node) { - ProcessNode(child); + SetUniqueNameForNode(child); } } foreach (Transform child in rootNode) { - ProcessNode(child); + SetUniqueNameForNode(child); } } From 0d2e987dc791642d87222e1cbb5ea9f64e172d0f Mon Sep 17 00:00:00 2001 From: eeropomell Date: Sun, 8 Dec 2024 13:03:09 +0200 Subject: [PATCH 08/10] apply code formatting --- Assets/Scripts/Model.cs | 119 ++++++++++++++++++++++++---------------- 1 file changed, 73 insertions(+), 46 deletions(-) diff --git a/Assets/Scripts/Model.cs b/Assets/Scripts/Model.cs index 2ba6d315cc..e84eb79d94 100644 --- a/Assets/Scripts/Model.cs +++ b/Assets/Scripts/Model.cs @@ -29,7 +29,6 @@ namespace TiltBrush { - public class Model { public struct Location @@ -60,6 +59,7 @@ public static Location File(string relativePath) path = relativePath.Substring(0, lastIndex); fragment = relativePath.Substring(lastIndex + 1); } + return new Location { type = Type.LocalFile, @@ -87,6 +87,7 @@ public string AbsolutePath { return null; } + switch (type) { case Type.LocalFile: @@ -94,6 +95,7 @@ public string AbsolutePath case Type.IcosaAssetId: return path.Replace("\\", "/"); } + return null; } } @@ -102,7 +104,11 @@ public string RelativePath { get { - if (type == Type.LocalFile) { return path; } + if (type == Type.LocalFile) + { + return path; + } + throw new Exception("Invalid relative path request"); } } @@ -113,12 +119,19 @@ public string AssetId { get { - if (type == Type.IcosaAssetId) { return id; } + if (type == Type.IcosaAssetId) + { + return id; + } + throw new Exception("Invalid Icosa asset id request"); } } - public Type GetLocationType() { return type; } + public Type GetLocationType() + { + return type; + } public override int GetHashCode() { @@ -136,6 +149,7 @@ public override string ToString() { str = $"{type}:{path}"; } + return str; } @@ -145,6 +159,7 @@ public override bool Equals(object obj) { return false; } + return this == (Location)obj; } @@ -197,8 +212,10 @@ public LoadError(string message, string detail = null) this.message = message; this.detail = detail; } + public readonly string message; // Human-readable short message - public readonly string detail; // Maybe non-human-readable details + + public readonly string detail; // Maybe non-human-readable details // maybe? public bool transient; // true if we know for sure that this error is transient } @@ -208,6 +225,7 @@ public LoadError(string message, string detail = null) /// m_LoadError != null implies m_Valid == false private LoadError? m_LoadError; + public LoadError? Error => m_LoadError; // How many widgets are using this model? @@ -248,7 +266,6 @@ public IExportableMaterial GetExportableMaterial(Material material) EnsureCollectorExists(); return m_ImportMaterialCollector.GetExportableMaterial(material); } - public Model(Location location) @@ -256,7 +273,10 @@ public Model(Location location) m_Location = location; } - public Location GetLocation() { return m_Location; } + public Location GetLocation() + { + return m_Location; + } /// A helper class which allows import to run I/O on a background thread before producing Unity /// GameObject(s). Usage: @@ -289,11 +309,7 @@ abstract class ModelBuilder /// It's unclear if the intent is that the user should continue calling TryEndAsyncLoad /// until it returns true, or if they should stop calling TryEndAsyncLoad. etc. Probably /// we should remove this. - public bool IsValid - { - get; - protected set; - } + public bool IsValid { get; protected set; } public ModelBuilder(string localPath) { @@ -318,13 +334,15 @@ public void CancelAsyncLoad() if (m_root != null) { foreach (var mesh in m_root.GetComponentsInChildren() - .Select(x => x.sharedMesh)) + .Select(x => x.sharedMesh)) { UObject.Destroy(mesh); } + UObject.Destroy(m_root); m_root = null; } + m_stateReader.Close(); } @@ -334,7 +352,7 @@ public void CancelAsyncLoad() /// ImportMaterialCollector - non-null upon successful completion. /// Raises an exception on unsuccessful completion. public bool TryEndAsyncLoad(out GameObject root, - out ImportMaterialCollector importMaterialCollector) + out ImportMaterialCollector importMaterialCollector) { // Three things happen in this function. // 1: It waits to try and get the result of reading the model on a background thread @@ -346,7 +364,10 @@ public bool TryEndAsyncLoad(out GameObject root, if (m_meshEnumerator == null) { IDisposable state; - if (!m_stateReader.TryGetResult(out state)) { return false; } + if (!m_stateReader.TryGetResult(out state)) + { + return false; + } IEnumerable enumerable; m_root = DoUnityThreadWork(state, out enumerable, out m_ImportMaterialCollector); @@ -359,6 +380,7 @@ public bool TryEndAsyncLoad(out GameObject root, { return false; } + m_ImportMaterialCollector = new ImportMaterialCollector( Path.GetDirectoryName(m_localPath), uniqueSeed: m_localPath @@ -366,6 +388,7 @@ public bool TryEndAsyncLoad(out GameObject root, m_meshEnumerator = enumerable.GetEnumerator(); m_root.SetActive(false); } + // Yield until the limiter unblocks. // Multiple calls to TryGetResult above are harmless. if (sm_Limiter.IsBlocked()) @@ -387,6 +410,7 @@ public bool TryEndAsyncLoad(out GameObject root, stopwatch.Stop(); return true; } + if (stopwatch.ElapsedTicks > numTicks) { stopwatch.Stop(); @@ -437,13 +461,14 @@ protected override IDisposable DoBackgroundThreadWork() { return ImportGltf.BeginImport(m_localPath, loader, options); } + return NewGltfImporter.BeginImport(m_localPath); } protected override GameObject DoUnityThreadWork(IDisposable state__, - out IEnumerable meshEnumerable, - out ImportMaterialCollector - importMaterialCollector) + out IEnumerable meshEnumerable, + out ImportMaterialCollector + importMaterialCollector) { GameObject rootObject = null; if (m_fromIcosa) @@ -467,6 +492,7 @@ out ImportMaterialCollector importMaterialCollector = (ImportMaterialCollector)result.materialCollector; } } + IsValid = rootObject != null; meshEnumerable = null; importMaterialCollector = null; @@ -485,9 +511,11 @@ out ImportMaterialCollector // EndImport doesn't try to use the loadImages functionality of UriLoader anyway. // It knows it's on the main thread, so chooses to use Unity's fast loading. rootObject = state.root; - importMaterialCollector = new ImportMaterialCollector(assetLocation, uniqueSeed: m_localPath); + importMaterialCollector = + new ImportMaterialCollector(assetLocation, uniqueSeed: m_localPath); } } + IsValid = rootObject != null; return rootObject; } @@ -505,7 +533,6 @@ GameObject LoadUsd(List warnings) GameObject LoadPly(List warningsOut) { - try { var reader = new PlyReader(m_Location.AbsolutePath); @@ -522,7 +549,6 @@ GameObject LoadPly(List warningsOut) Debug.LogException(ex); return null; } - } GameObject LoadSvg(List warningsOut, out SVGParser.SceneInfo sceneInfo) @@ -663,6 +689,7 @@ public bool TryLoadModel() { return false; } + isValid = m_builder.IsValid; } catch (ObjectDisposedException ex) @@ -705,12 +732,11 @@ public async Task LoadModelAsync() { Task t = StartCreatePrefab(null); await t; - } + public void LoadModel() { StartCreatePrefab(null); - } /// Either synchronously load a GameObject hierarchy and convert it to a "prefab" @@ -750,13 +776,11 @@ private async Task StartCreatePrefab(GameObject go) EndCreatePrefab(go, warnings); } else if (m_Location.GetLocationType() == Location.Type.IcosaAssetId || - ext == ".gltf2" || ext == ".gltf" || ext == ".glb") + ext == ".gltf2" || ext == ".gltf" || ext == ".glb") { // If we pulled this from Icosa, it's going to be a gltf file. Task t = LoadGltf(warnings); await t; - - } else if (ext == ".fbx" || ext == ".obj") { @@ -782,7 +806,6 @@ private async Task StartCreatePrefab(GameObject go) m_LoadError = new LoadError("Unknown format", ext); } } - } public void CalcBoundsGltf(GameObject go) @@ -804,6 +827,7 @@ public void CalcBoundsGltf(GameObject go) b.Encapsulate(bounds); } } + m_MeshBounds = b; if (first) { @@ -833,20 +857,20 @@ private void CalcBoundsNonGltf(GameObject go) { b.Encapsulate(bc.bounds); } + UnityEngine.Object.Destroy(bc); } + m_MeshBounds = b; if (first) { // There was no geometry Debug.LogErrorFormat("No usable geometry in model. LoadModel({0})", go.name); } - } - + public void EndCreatePrefab(GameObject go, List warnings) { - if (go == null) { m_LoadError = m_LoadError ?? new LoadError("Bad data"); @@ -861,39 +885,37 @@ public void EndCreatePrefab(GameObject go, List warnings) { UnityEngine.Object.Destroy(m_ModelParent.gameObject); } + m_ModelParent = go.transform; - - #if DEVELOPMENT_BUILD || UNITY_EDITOR + +#if DEVELOPMENT_BUILD || UNITY_EDITOR ProfilerMarker generateUniqueNamesPerfMarker = new ProfilerMarker("Model.GenerateUniqueNames"); generateUniqueNamesPerfMarker.Begin(); - #endif - +#endif + GenerateUniqueNames(m_ModelParent); - - #if DEVELOPMENT_BUILD || UNITY_EDITOR + +#if DEVELOPMENT_BUILD || UNITY_EDITOR generateUniqueNamesPerfMarker.End(); - #endif +#endif // !!! Add to material dictionary here? m_Valid = true; DisplayWarnings(warnings); - } - - + // This method is called when the model has been loaded and the node tree is available // This method is necessary because (1) nodes in e.g glTF files don't need to have unique names // and (2) there's code in at least ModelWidget that searches for specific nodes using node names private static void GenerateUniqueNames(Transform rootNode) { - void SetUniqueNameForNode(Transform node) { // GetInstanceID returns a unique ID for every GameObject during a runtime session node.name += " uid: " + node.gameObject.GetInstanceID(); - + foreach (Transform child in node) { SetUniqueNameForNode(child); @@ -905,7 +927,7 @@ void SetUniqueNameForNode(Transform node) SetUniqueNameForNode(child); } } - + public void UnloadModel() { @@ -914,6 +936,7 @@ public void UnloadModel() m_builder.CancelAsyncLoad(); m_builder = null; } + m_Valid = false; m_LoadError = null; if (m_ModelParent != null) @@ -921,10 +944,11 @@ public void UnloadModel() // Procedurally created meshes need to be explicitly destroyed - you can't just destroy // the MeshFilter that references them. foreach (var mesh in m_ModelParent.GetComponentsInChildren() - .Select(x => x.sharedMesh)) + .Select(x => x.sharedMesh)) { UObject.Destroy(mesh); } + UObject.Destroy(m_ModelParent.gameObject); m_ModelParent = null; } @@ -952,6 +976,7 @@ public IEnumerator LoadFullyCoroutine(string reason) { yield return null; } + break; default: m_LoadError = new LoadError($"Unknown load type {type}"); @@ -976,7 +1001,7 @@ private void DisplayWarnings(List warnings) public bool IsCached() { return m_Location.GetLocationType() == Location.Type.IcosaAssetId && - Directory.Exists(m_Location.AbsolutePath); + Directory.Exists(m_Location.AbsolutePath); } public void RefreshCache() @@ -994,6 +1019,7 @@ public MeshFilter[] GetMeshes() { throw new InvalidOperationException(); } + return m_ModelParent.GetComponent().m_MeshChildren; } @@ -1006,6 +1032,7 @@ public string GetExportName() case Model.Location.Type.IcosaAssetId: return AssetId; } + return "Unknown"; } @@ -1034,4 +1061,4 @@ public void EnsureCollectorExists() } } } -} // namespace TiltBrush; +} // namespace TiltBrush; \ No newline at end of file From 7ac28f7dec79ceb406209ebb08ba90126a21f45a Mon Sep 17 00:00:00 2001 From: Andy Baker Date: Tue, 10 Dec 2024 18:08:52 +0000 Subject: [PATCH 09/10] Undo formatting changes --- Assets/Scripts/Model.cs | 93 +++++++++------------------ Assets/Scripts/Widgets/ModelWidget.cs | 8 +-- 2 files changed, 33 insertions(+), 68 deletions(-) diff --git a/Assets/Scripts/Model.cs b/Assets/Scripts/Model.cs index e84eb79d94..c3cc07d0c5 100644 --- a/Assets/Scripts/Model.cs +++ b/Assets/Scripts/Model.cs @@ -29,8 +29,10 @@ namespace TiltBrush { + public class Model { + public struct Location { public enum Type @@ -59,7 +61,6 @@ public static Location File(string relativePath) path = relativePath.Substring(0, lastIndex); fragment = relativePath.Substring(lastIndex + 1); } - return new Location { type = Type.LocalFile, @@ -87,7 +88,6 @@ public string AbsolutePath { return null; } - switch (type) { case Type.LocalFile: @@ -95,7 +95,6 @@ public string AbsolutePath case Type.IcosaAssetId: return path.Replace("\\", "/"); } - return null; } } @@ -104,11 +103,7 @@ public string RelativePath { get { - if (type == Type.LocalFile) - { - return path; - } - + if (type == Type.LocalFile) { return path; } throw new Exception("Invalid relative path request"); } } @@ -119,19 +114,12 @@ public string AssetId { get { - if (type == Type.IcosaAssetId) - { - return id; - } - + if (type == Type.IcosaAssetId) { return id; } throw new Exception("Invalid Icosa asset id request"); } } - public Type GetLocationType() - { - return type; - } + public Type GetLocationType() { return type; } public override int GetHashCode() { @@ -149,7 +137,6 @@ public override string ToString() { str = $"{type}:{path}"; } - return str; } @@ -159,7 +146,6 @@ public override bool Equals(object obj) { return false; } - return this == (Location)obj; } @@ -212,10 +198,8 @@ public LoadError(string message, string detail = null) this.message = message; this.detail = detail; } - public readonly string message; // Human-readable short message - - public readonly string detail; // Maybe non-human-readable details + public readonly string detail; // Maybe non-human-readable details // maybe? public bool transient; // true if we know for sure that this error is transient } @@ -225,7 +209,6 @@ public LoadError(string message, string detail = null) /// m_LoadError != null implies m_Valid == false private LoadError? m_LoadError; - public LoadError? Error => m_LoadError; // How many widgets are using this model? @@ -267,11 +250,7 @@ public IExportableMaterial GetExportableMaterial(Material material) return m_ImportMaterialCollector.GetExportableMaterial(material); } - - public Model(Location location) - { - m_Location = location; - } + public Model(Location location) { m_Location = location; } public Location GetLocation() { @@ -309,7 +288,11 @@ abstract class ModelBuilder /// It's unclear if the intent is that the user should continue calling TryEndAsyncLoad /// until it returns true, or if they should stop calling TryEndAsyncLoad. etc. Probably /// we should remove this. - public bool IsValid { get; protected set; } + public bool IsValid + { + get; + protected set; + } public ModelBuilder(string localPath) { @@ -334,15 +317,13 @@ public void CancelAsyncLoad() if (m_root != null) { foreach (var mesh in m_root.GetComponentsInChildren() - .Select(x => x.sharedMesh)) + .Select(x => x.sharedMesh)) { UObject.Destroy(mesh); } - UObject.Destroy(m_root); m_root = null; } - m_stateReader.Close(); } @@ -352,7 +333,7 @@ public void CancelAsyncLoad() /// ImportMaterialCollector - non-null upon successful completion. /// Raises an exception on unsuccessful completion. public bool TryEndAsyncLoad(out GameObject root, - out ImportMaterialCollector importMaterialCollector) + out ImportMaterialCollector importMaterialCollector) { // Three things happen in this function. // 1: It waits to try and get the result of reading the model on a background thread @@ -364,10 +345,7 @@ public bool TryEndAsyncLoad(out GameObject root, if (m_meshEnumerator == null) { IDisposable state; - if (!m_stateReader.TryGetResult(out state)) - { - return false; - } + if (!m_stateReader.TryGetResult(out state)) { return false; } IEnumerable enumerable; m_root = DoUnityThreadWork(state, out enumerable, out m_ImportMaterialCollector); @@ -380,7 +358,6 @@ public bool TryEndAsyncLoad(out GameObject root, { return false; } - m_ImportMaterialCollector = new ImportMaterialCollector( Path.GetDirectoryName(m_localPath), uniqueSeed: m_localPath @@ -388,7 +365,6 @@ public bool TryEndAsyncLoad(out GameObject root, m_meshEnumerator = enumerable.GetEnumerator(); m_root.SetActive(false); } - // Yield until the limiter unblocks. // Multiple calls to TryGetResult above are harmless. if (sm_Limiter.IsBlocked()) @@ -410,7 +386,6 @@ public bool TryEndAsyncLoad(out GameObject root, stopwatch.Stop(); return true; } - if (stopwatch.ElapsedTicks > numTicks) { stopwatch.Stop(); @@ -461,14 +436,13 @@ protected override IDisposable DoBackgroundThreadWork() { return ImportGltf.BeginImport(m_localPath, loader, options); } - return NewGltfImporter.BeginImport(m_localPath); } protected override GameObject DoUnityThreadWork(IDisposable state__, - out IEnumerable meshEnumerable, - out ImportMaterialCollector - importMaterialCollector) + out IEnumerable meshEnumerable, + out ImportMaterialCollector + importMaterialCollector) { GameObject rootObject = null; if (m_fromIcosa) @@ -492,7 +466,6 @@ out ImportMaterialCollector importMaterialCollector = (ImportMaterialCollector)result.materialCollector; } } - IsValid = rootObject != null; meshEnumerable = null; importMaterialCollector = null; @@ -511,11 +484,9 @@ out ImportMaterialCollector // EndImport doesn't try to use the loadImages functionality of UriLoader anyway. // It knows it's on the main thread, so chooses to use Unity's fast loading. rootObject = state.root; - importMaterialCollector = - new ImportMaterialCollector(assetLocation, uniqueSeed: m_localPath); + importMaterialCollector = new ImportMaterialCollector(assetLocation, uniqueSeed: m_localPath); } } - IsValid = rootObject != null; return rootObject; } @@ -533,6 +504,7 @@ GameObject LoadUsd(List warnings) GameObject LoadPly(List warningsOut) { + try { var reader = new PlyReader(m_Location.AbsolutePath); @@ -549,6 +521,7 @@ GameObject LoadPly(List warningsOut) Debug.LogException(ex); return null; } + } GameObject LoadSvg(List warningsOut, out SVGParser.SceneInfo sceneInfo) @@ -689,7 +662,6 @@ public bool TryLoadModel() { return false; } - isValid = m_builder.IsValid; } catch (ObjectDisposedException ex) @@ -732,11 +704,12 @@ public async Task LoadModelAsync() { Task t = StartCreatePrefab(null); await t; - } + } public void LoadModel() { StartCreatePrefab(null); + } /// Either synchronously load a GameObject hierarchy and convert it to a "prefab" @@ -776,7 +749,7 @@ private async Task StartCreatePrefab(GameObject go) EndCreatePrefab(go, warnings); } else if (m_Location.GetLocationType() == Location.Type.IcosaAssetId || - ext == ".gltf2" || ext == ".gltf" || ext == ".glb") + ext == ".gltf2" || ext == ".gltf" || ext == ".glb") { // If we pulled this from Icosa, it's going to be a gltf file. Task t = LoadGltf(warnings); @@ -806,6 +779,7 @@ private async Task StartCreatePrefab(GameObject go) m_LoadError = new LoadError("Unknown format", ext); } } + } public void CalcBoundsGltf(GameObject go) @@ -827,7 +801,6 @@ public void CalcBoundsGltf(GameObject go) b.Encapsulate(bounds); } } - m_MeshBounds = b; if (first) { @@ -857,16 +830,15 @@ private void CalcBoundsNonGltf(GameObject go) { b.Encapsulate(bc.bounds); } - UnityEngine.Object.Destroy(bc); } - m_MeshBounds = b; if (first) { // There was no geometry Debug.LogErrorFormat("No usable geometry in model. LoadModel({0})", go.name); } + } public void EndCreatePrefab(GameObject go, List warnings) @@ -885,7 +857,6 @@ public void EndCreatePrefab(GameObject go, List warnings) { UnityEngine.Object.Destroy(m_ModelParent.gameObject); } - m_ModelParent = go.transform; #if DEVELOPMENT_BUILD || UNITY_EDITOR @@ -928,7 +899,6 @@ void SetUniqueNameForNode(Transform node) } } - public void UnloadModel() { if (m_builder != null) @@ -936,7 +906,6 @@ public void UnloadModel() m_builder.CancelAsyncLoad(); m_builder = null; } - m_Valid = false; m_LoadError = null; if (m_ModelParent != null) @@ -944,11 +913,10 @@ public void UnloadModel() // Procedurally created meshes need to be explicitly destroyed - you can't just destroy // the MeshFilter that references them. foreach (var mesh in m_ModelParent.GetComponentsInChildren() - .Select(x => x.sharedMesh)) + .Select(x => x.sharedMesh)) { UObject.Destroy(mesh); } - UObject.Destroy(m_ModelParent.gameObject); m_ModelParent = null; } @@ -976,7 +944,6 @@ public IEnumerator LoadFullyCoroutine(string reason) { yield return null; } - break; default: m_LoadError = new LoadError($"Unknown load type {type}"); @@ -1001,7 +968,7 @@ private void DisplayWarnings(List warnings) public bool IsCached() { return m_Location.GetLocationType() == Location.Type.IcosaAssetId && - Directory.Exists(m_Location.AbsolutePath); + Directory.Exists(m_Location.AbsolutePath); } public void RefreshCache() @@ -1019,7 +986,6 @@ public MeshFilter[] GetMeshes() { throw new InvalidOperationException(); } - return m_ModelParent.GetComponent().m_MeshChildren; } @@ -1032,7 +998,6 @@ public string GetExportName() case Model.Location.Type.IcosaAssetId: return AssetId; } - return "Unknown"; } @@ -1061,4 +1026,4 @@ public void EnsureCollectorExists() } } } -} // namespace TiltBrush; \ No newline at end of file +} // namespace TiltBrush; diff --git a/Assets/Scripts/Widgets/ModelWidget.cs b/Assets/Scripts/Widgets/ModelWidget.cs index f70d664276..b44b9cf956 100644 --- a/Assets/Scripts/Widgets/ModelWidget.cs +++ b/Assets/Scripts/Widgets/ModelWidget.cs @@ -30,8 +30,8 @@ public class ModelWidget : MediaWidget [SerializeField] private float m_MaxBloat; private Model m_Model; - - + + // What is Subtree? // e.g. if we have 3d model with 3 chairs with the hierarchy below, // then when the model is broken apart, we create a separate ModelWidget for each Chair1,Chair2,Chair3 @@ -258,7 +258,7 @@ void LoadModel() m_ModelInstance = Instantiate(m_Model.m_ModelParent); m_ModelInstance.gameObject.SetActive(true); m_ModelInstance.parent = this.transform; - + Coords.AsLocal[m_ModelInstance] = TrTransform.identity; float maxExtent = 2 * Mathf.Max(m_Model.m_MeshBounds.extents.x, Mathf.Max(m_Model.m_MeshBounds.extents.y, m_Model.m_MeshBounds.extents.z)); @@ -344,7 +344,7 @@ public void SyncHierarchyToSubtree(string previousSubtree = null) // Walk the hierarchy and find the matching node Transform oldRoot = m_ObjModelScript.transform; Transform node = oldRoot; - + // We only want to walk the new part of the hierarchy string subpathToTraverse; if (!string.IsNullOrEmpty(previousSubtree)) From c944cab220a247635821fd04e920328780c4554a Mon Sep 17 00:00:00 2001 From: Andy Baker Date: Tue, 10 Dec 2024 18:13:36 +0000 Subject: [PATCH 10/10] fix a mistaken formatting fix --- Assets/Scripts/Model.cs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/Assets/Scripts/Model.cs b/Assets/Scripts/Model.cs index c3cc07d0c5..f6537790e0 100644 --- a/Assets/Scripts/Model.cs +++ b/Assets/Scripts/Model.cs @@ -32,7 +32,6 @@ namespace TiltBrush public class Model { - public struct Location { public enum Type @@ -250,13 +249,13 @@ public IExportableMaterial GetExportableMaterial(Material material) return m_ImportMaterialCollector.GetExportableMaterial(material); } - public Model(Location location) { m_Location = location; } - - public Location GetLocation() + public Model(Location location) { - return m_Location; + m_Location = location; } + public Location GetLocation() { return m_Location; } + /// A helper class which allows import to run I/O on a background thread before producing Unity /// GameObject(s). Usage: /// BeginAsyncLoad()