Skip to content

Commit

Permalink
Fixes from PR comments;
Browse files Browse the repository at this point in the history
  • Loading branch information
katehryhorenko committed Jan 21, 2025
1 parent 73ed8d6 commit 69cbe69
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 75 deletions.
120 changes: 63 additions & 57 deletions Elements/src/Model.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
using Elements.Geometry.Solids;
using Elements.GeoJSON;
using System.IO;
using System.Text;

namespace Elements
{
Expand All @@ -21,6 +20,45 @@ namespace Elements
/// </summary>
public class Model
{
private class GatherSubElementsResult
{
/// <summary>
/// List of elements collected from the object.
/// </summary>
public List<Element> Elements { get; } = new List<Element>();
/// <summary>
/// List of shared objects collected from the object.
/// </summary>
public List<SharedObject> SharedObjects { get; } = new List<SharedObject>();
/// <summary>
/// List of elements collected from the shared object's properties.
///
/// If shared object is marked as JsonIgnore (e.g. RepresentationInstance), it will not be
/// serialized to JSON, but it's properties will be collected here so they can be used
/// during gltf serialization.
/// </summary>
public List<Element> ElementsFromSharedObjectProperties { get; } = new List<Element>();

public void MergeSubResult(GatherSubElementsResult gatherResult, bool hasJsonIgnore, bool isTypeRelatedToSharedObjects)
{
if (isTypeRelatedToSharedObjects)
{
ElementsFromSharedObjectProperties.AddRange(gatherResult.ElementsFromSharedObjectProperties);
}
else
{
Elements.AddRange(gatherResult.Elements);
}
// do not save shared objects marked with JsonIgnore
if (!hasJsonIgnore)
{
SharedObjects.AddRange(gatherResult.SharedObjects);
Elements.AddRange(gatherResult.ElementsFromSharedObjectProperties);
}
ElementsFromSharedObjectProperties.AddRange(gatherResult.ElementsFromSharedObjectProperties);
}
}

/// <summary>The origin of the model.</summary>
[JsonProperty("Origin", NullValueHandling = Newtonsoft.Json.NullValueHandling.Ignore)]
[Obsolete("Use Transform instead.")]
Expand All @@ -37,7 +75,6 @@ public class Model

/// <summary>A collection of SharedObjects keyed by their identifiers.</summary>
[JsonProperty("SharedObjects", Required = Required.Default)]
[System.ComponentModel.DataAnnotations.Required]
public System.Collections.Generic.IDictionary<Guid, SharedObject> SharedObjects { get; set; } = new System.Collections.Generic.Dictionary<Guid, SharedObject>();

/// <summary>
Expand Down Expand Up @@ -128,8 +165,8 @@ public void AddElement(Element element, bool gatherSubElements = true, bool upda
// to the elements dictionary first. This will ensure that
// those elements will be read out and be available before
// an attempt is made to deserialize the element itself.
var (subElements, sharedObjects) = RecursiveGatherSubElements(element, out var elementsToIgnore);
foreach (var e in subElements)
var gatherSubElementsResult = RecursiveGatherSubElements(element);
foreach (var e in gatherSubElementsResult.Elements)
{
if (!this.Elements.ContainsKey(e.Id))
{
Expand All @@ -143,15 +180,15 @@ public void AddElement(Element element, bool gatherSubElements = true, bool upda
}
}

foreach (var sharedObject in sharedObjects)
foreach (var sharedObject in gatherSubElementsResult.SharedObjects)
{
if (!SharedObjects.ContainsKey(sharedObject.Id))
{
SharedObjects.Add(sharedObject.Id, sharedObject);
}
}

foreach (var e in elementsToIgnore)
foreach (var e in gatherSubElementsResult.ElementsFromSharedObjectProperties)
{
if (!SubElementsFromSharedObjects.ContainsKey(e.Id))
{
Expand Down Expand Up @@ -466,24 +503,21 @@ public static Model FromJson(string json, bool forceTypeReload = false)
return FromJson(json, out _, forceTypeReload);
}

private (List<Element> elements, List<SharedObject> sharedObjects) RecursiveGatherSubElements(object obj, out List<Element> sharedObjectSubElements)
private GatherSubElementsResult RecursiveGatherSubElements(object obj)
{
// A dictionary created for the purpose of caching properties
// that we need to recurse, for types that we've seen before.
var props = new Dictionary<Type, List<PropertyInfo>>();

return RecursiveGatherSubElementsInternal(obj, props, out sharedObjectSubElements);
return RecursiveGatherSubElementsInternal(obj, props);
}

private (List<Element> elements, List<SharedObject> sharedObjects) RecursiveGatherSubElementsInternal(object obj, Dictionary<Type, List<PropertyInfo>> properties, out List<Element> sharedObjectSubElements)
private GatherSubElementsResult RecursiveGatherSubElementsInternal(object obj, Dictionary<Type, List<PropertyInfo>> properties)
{
var elements = new List<Element>();
sharedObjectSubElements = new List<Element>();
var sharedObjects = new List<SharedObject>();
GatherSubElementsResult result = new GatherSubElementsResult();

if (obj == null)
{
return (elements, sharedObjects);
return result;
}

var e = obj as Element;
Expand All @@ -492,7 +526,7 @@ public static Model FromJson(string json, bool forceTypeReload = false)
// Do nothing. The Element has already
// been added. This assumes that that the sub-elements
// have been added as well and we don't need to continue.
return (elements, sharedObjects);
return result;
}

// This explicit loop is because we have mappings marked as internal so it's elements won't be automatically serialized.
Expand All @@ -501,17 +535,17 @@ public static Model FromJson(string json, bool forceTypeReload = false)
foreach (var map in e.Mappings ?? new Dictionary<string, MappingBase>())
{
if (!Elements.ContainsKey(map.Value.Id))
{ elements.Add(map.Value); }
{ result.Elements.Add(map.Value); }
}
}

var sharedObject = obj as SharedObject;

// if this shared object is already in the list, we don't need to process and add it again
if (sharedObject != null)
{
if (SharedObjects.ContainsKey(sharedObject.Id))
{
return (elements, sharedObjects);
return result;
}
}

Expand All @@ -522,7 +556,7 @@ public static Model FromJson(string json, bool forceTypeReload = false)
// could be elements.
if (!t.IsClass || t == typeof(string))
{
return (elements, sharedObjects);
return result;
}

List<PropertyInfo> constrainedProps;
Expand All @@ -539,8 +573,7 @@ public static Model FromJson(string json, bool forceTypeReload = false)
properties.Add(t, constrainedProps);
}

var elementsFromProperties = new List<Element>();
var sharedObjectsFromProperties = new List<SharedObject>();
bool isTypeRelatedToSharedObjects = IsTypeRelatedToSharedObjects(t);
foreach (var p in constrainedProps)
{
try
Expand All @@ -558,14 +591,8 @@ public static Model FromJson(string json, bool forceTypeReload = false)
{
foreach (var item in elems)
{
var (subElements, subSharedObjects) = RecursiveGatherSubElementsInternal(item, properties, out var elementsFromSharedObjectProperties);
elementsFromProperties.AddRange(subElements);
// do not save shared objects marked with JsonIgnore
if (!hasJsonIgnore)
{
sharedObjectsFromProperties.AddRange(subSharedObjects);
}
sharedObjectSubElements.AddRange(elementsFromSharedObjectProperties);
var subElements = RecursiveGatherSubElementsInternal(item, properties);
result.MergeSubResult(subElements, hasJsonIgnore, isTypeRelatedToSharedObjects);
}
continue;
}
Expand All @@ -575,53 +602,32 @@ public static Model FromJson(string json, bool forceTypeReload = false)
{
foreach (var value in dict.Values)
{
var (subElements, subSharedObjects) = RecursiveGatherSubElementsInternal(value, properties, out var elementsFromSharedObjectProperties);
elementsFromProperties.AddRange(subElements);
// do not save shared objects marked with JsonIgnore
if (!hasJsonIgnore)
{
sharedObjectsFromProperties.AddRange(subSharedObjects);
}
sharedObjectSubElements.AddRange(elementsFromSharedObjectProperties);
var subElements = RecursiveGatherSubElementsInternal(value, properties);
result.MergeSubResult(subElements, hasJsonIgnore, isTypeRelatedToSharedObjects);
}
continue;
}

var (gatheredSubElements, gatheredSubSharedObjects) = RecursiveGatherSubElementsInternal(pValue, properties, out var elementsFromPropertyToIgnore);
elementsFromProperties.AddRange(gatheredSubElements);
// do not save shared objects marked with JsonIgnore
if (!hasJsonIgnore)
{
sharedObjectsFromProperties.AddRange(gatheredSubSharedObjects);
}
sharedObjectSubElements.AddRange(elementsFromPropertyToIgnore);
var gatheredSubElements = RecursiveGatherSubElementsInternal(pValue, properties);
result.MergeSubResult(gatheredSubElements, hasJsonIgnore, isTypeRelatedToSharedObjects);
}
catch (Exception ex)
{
throw new Exception($"The {p.Name} property or one of its children was not valid for introspection. Check the inner exception for details.", ex);
}
}
if (IsTypeRelatedToSharedObjects(t))
{
sharedObjectSubElements.AddRange(elementsFromProperties);
}
else
{
elements.AddRange(elementsFromProperties);
}
sharedObjects.AddRange(sharedObjectsFromProperties);

if (e != null)
{
elements.Add(e);
result.Elements.Add(e);
}

if (sharedObject != null)
{
sharedObjects.Add(sharedObject);
result.SharedObjects.Add(sharedObject);
}

return (elements, sharedObjects);
return result;
}

/// <summary>
Expand Down
26 changes: 8 additions & 18 deletions Elements/src/Serialization/JSON/JsonInheritanceConverter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,12 +185,12 @@ public override void WriteJson(Newtonsoft.Json.JsonWriter writer, object value,
// Operate on all identifiable Elements with a path less than Entities.xxxxx
// This will get all properties.
var element = value as Element;
if (element != null && !WritingTopLevelElement(writer.Path) && !ElementwiseSerialization)
if (element != null && !PathIsTopLevel(writer.Path, "Elements") && !ElementwiseSerialization)
{
var ident = element;
writer.WriteValue(ident.Id);
}
else if (value is SharedObject sharedObject && !WritingTopLevelSharedObject(writer.Path) && !ElementwiseSerialization)
else if (value is SharedObject sharedObject && !PathIsTopLevel(writer.Path, "SharedObjects") && !ElementwiseSerialization)
{
var ident = sharedObject;
writer.WriteValue(ident.Id);
Expand Down Expand Up @@ -275,20 +275,10 @@ private void RemovePropertiesSameAsInSharedObject(Element element, JObject jObje
}
}

private static bool WritingTopLevelElement(string path)
private static bool PathIsTopLevel(string path, string propertyName)
{
var parts = path.Split('.');
if (parts.Length == 2 && parts[0] == "Elements" && Guid.TryParse(parts[1], out var _))
{
return true;
}
return false;
}

private static bool WritingTopLevelSharedObject(string path)
{
var parts = path.Split('.');
if (parts.Length == 2 && parts[0] == "SharedObjects" && Guid.TryParse(parts[1], out var _))
if (parts.Length == 2 && parts[0] == propertyName && Guid.TryParse(parts[1], out var _))
{
return true;
}
Expand Down Expand Up @@ -350,7 +340,7 @@ public override object ReadJson(Newtonsoft.Json.JsonReader reader, System.Type o
{
// The serialized value is an identifier, so the expectation is
// that the element with that id has already been deserialized.
if (typeof(Element).IsAssignableFrom(objectType) && !WritingTopLevelElement(reader.Path) && reader.Value != null)
if (typeof(Element).IsAssignableFrom(objectType) && !PathIsTopLevel(reader.Path, "Elements") && reader.Value != null)
{
var id = Guid.Parse(reader.Value.ToString());
if (!Elements.ContainsKey(id))
Expand All @@ -361,7 +351,7 @@ public override object ReadJson(Newtonsoft.Json.JsonReader reader, System.Type o
return Elements[id];
}

if (typeof(SharedObject).IsAssignableFrom(objectType) && !WritingTopLevelSharedObject(reader.Path) && reader.Value != null)
if (typeof(SharedObject).IsAssignableFrom(objectType) && !PathIsTopLevel(reader.Path, "SharedObjects") && reader.Value != null)
{
var id = Guid.Parse(reader.Value.ToString());
if (!SharedObjects.ContainsKey(id))
Expand Down Expand Up @@ -405,7 +395,7 @@ public override object ReadJson(Newtonsoft.Json.JsonReader reader, System.Type o

// Write the id to the cache so that we can retrieve it next time
// instead of de-serializing it again.
if (typeof(Element).IsAssignableFrom(objectType) && WritingTopLevelElement(reader.Path))
if (typeof(Element).IsAssignableFrom(objectType) && PathIsTopLevel(reader.Path, "Elements"))
{
var ident = (Element)obj;
if (!Elements.ContainsKey(ident.Id))
Expand All @@ -414,7 +404,7 @@ public override object ReadJson(Newtonsoft.Json.JsonReader reader, System.Type o
}
}

if (typeof(SharedObject).IsAssignableFrom(objectType) && WritingTopLevelSharedObject(reader.Path))
if (typeof(SharedObject).IsAssignableFrom(objectType) && PathIsTopLevel(reader.Path, "SharedObjects"))
{
var ident = (SharedObject)obj;
if (!SharedObjects.ContainsKey(ident.Id))
Expand Down

0 comments on commit 69cbe69

Please sign in to comment.