Skip to content
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

[Serialization] Fix diverging rules for editor and runtime serialization of fields and properties #1875

Merged
merged 11 commits into from Oct 16, 2023
Git LFS file not shown
@@ -1 +1 @@
52F819550E98D126A685C556595CB25A66BFC7CD97352FA5D127B4A759B52FDB
F5307F2607FE9E761088E06BACCDCB5ECE8739B25418A6B19421004D872C343D
Git LFS file not shown
Git LFS file not shown
Git LFS file not shown
Git LFS file not shown
@@ -1 +1 @@
E17AB0CB8BC5F55CA985C21612E1DA0AF77EC6EA6CDD6FAF9F76252B0217D226
F47741092523EC3BF4C9C1B87BEC8E1FAECC919E1F8319CE64A696F876E186CB
Git LFS file not shown
Git LFS file not shown
Git LFS file not shown
Expand Up @@ -28,7 +28,7 @@ public MyAsset()
// TODO: Re-enable non-pure collections here once we support them for serialization!
//MapItems2 = new MyDictionary();
MapItems3 = new MyDictionaryPure();
CustomObjectWithProtectedSet = new CustomObject { Name = "customObject" };
CustomObjectWithInternalSet = new CustomObject { Name = "customObject" };
}

[DataMember(0)]
Expand Down Expand Up @@ -65,7 +65,8 @@ public MyAsset()

public MyDictionaryPure MapItems3 { get; set; }

public object CustomObjectWithProtectedSet { get; protected set; }
[DataMember]
public CustomObject CustomObjectWithInternalSet { get; internal set; }
}

[DataContract("CustomObject")]
Expand Down
Expand Up @@ -27,5 +27,5 @@ MapItems3:
11000000110000001100000011000000~key2: 2
12000000120000001200000012000000~key3: 3
13000000130000001300000013000000~key4: 3
CustomObjectWithProtectedSet:
CustomObjectWithInternalSet:
Name: customObject
5 changes: 5 additions & 0 deletions sources/assets/Stride.Core.Assets/AssetItem.cs
Expand Up @@ -59,6 +59,7 @@ internal AssetItem([NotNull] UFile location, [NotNull] Asset asset, Package pack
/// </summary>
/// <value>The location.</value>
[NotNull]
[DataMember]
public UFile Location { get => location; internal set => location = value ?? throw new ArgumentNullException(nameof(value)); }

/// <summary>
Expand All @@ -84,6 +85,7 @@ internal AssetItem([NotNull] UFile location, [NotNull] Asset asset, Package pack
/// Gets the package where this asset is stored.
/// </summary>
/// <value>The package.</value>
[DataMember]
public Package Package { get; internal set; }

/// <summary>
Expand Down Expand Up @@ -187,6 +189,7 @@ public UFile FullPath
/// </summary>
/// <value>The asset.</value>
[NotNull]
[DataMember]
public Asset Asset { get => asset; internal set => asset = value ?? throw new ArgumentNullException(nameof(value)); }

/// <summary>
Expand All @@ -197,13 +200,15 @@ public UFile FullPath
/// By default, contains the last modified time of the asset from the disk. If IsDirty is also updated from false to true
/// , this time will get current time of modification.
/// </remarks>
[DataMember]
public DateTime ModifiedTime { get; internal set; }

private long version;

/// <summary>
/// Gets the asset version incremental counter, increased everytime the asset is edited.
/// </summary>
[DataMember]
public long Version { get => Interlocked.Read(ref version); internal set => Interlocked.Exchange(ref version, value); }

/// <summary>
Expand Down
4 changes: 2 additions & 2 deletions sources/assets/Stride.Core.Assets/AssetReference.cs
Expand Up @@ -33,14 +33,14 @@ public AssetReference(AssetId id, UFile location)
/// </summary>
/// <value>The unique identifier of the reference asset..</value>
[DataMember(10)]
public AssetId Id { get; }
public AssetId Id { get; init; }

/// <summary>
/// Gets or sets the location of the asset.
/// </summary>
/// <value>The location.</value>
[DataMember(20)]
public string Location { get; }
public string Location { get; init; }

public bool Equals(AssetReference other)
{
Expand Down
4 changes: 2 additions & 2 deletions sources/assets/Stride.Core.Assets/BasePart.cs
Expand Up @@ -72,10 +72,10 @@ public BasePart([NotNull] AssetReference basePartAsset, Guid basePartId, Guid in
public AssetReference BasePartAsset { get; set; }

[DataMember(20)]
public Guid BasePartId { get; }
public Guid BasePartId { get; init; }

[DataMember(30)]
public Guid InstanceId { get; }
public Guid InstanceId { get; init; }

[CanBeNull]
public IIdentifiable ResolvePart(PackageSession session)
Expand Down
13 changes: 2 additions & 11 deletions sources/assets/Stride.Core.Assets/Bundle.cs
Expand Up @@ -14,15 +14,6 @@ namespace Stride.Core.Assets
[DebuggerDisplay("Bundle [{Name}] Selectors[{Selectors.Count}] Dependencies[{Dependencies.Count}]")]
public sealed class Bundle
{
/// <summary>
/// Initializes a new instance of the <see cref="Bundle"/> class.
/// </summary>
public Bundle()
{
Selectors = new List<AssetSelector>();
Dependencies = new List<string>();
}

/// <summary>
/// Gets or sets the name of this bundle.
/// </summary>
Expand All @@ -33,13 +24,13 @@ public Bundle()
/// Gets the selectors used by this bundle.
/// </summary>
/// <value>The selectors.</value>
public List<AssetSelector> Selectors { get; private set; }
public List<AssetSelector> Selectors { get; } = new List<AssetSelector>();

/// <summary>
/// Gets the bundle dependencies.
/// </summary>
/// <value>The dependencies.</value>
public List<string> Dependencies { get; private set; }
public List<string> Dependencies { get; } = new List<string>();

/// <summary>
/// Gets the output group (used in conjonction with <see cref="ProjectBuildProfile.OutputGroupDirectories"/> to control where file will be put).
Expand Down
10 changes: 1 addition & 9 deletions sources/assets/Stride.Core.Assets/Selectors/TagSelector.cs
Expand Up @@ -13,19 +13,11 @@ namespace Stride.Core.Assets.Selectors
[DataContract("TagSelector")]
public class TagSelector : AssetSelector
{
/// <summary>
/// Initializes a new instance of the <see cref="TagSelector"/> class.
/// </summary>
public TagSelector()
{
Tags = new TagCollection();
}

/// <summary>
/// Gets the tags that will be used to select an asset.
/// </summary>
/// <value>The tags.</value>
public TagCollection Tags { get; private set; }
public TagCollection Tags { get; } = new TagCollection();

public override IEnumerable<string> Select(PackageSession packageSession, IContentIndexMap contentIndexMap)
{
Expand Down
20 changes: 5 additions & 15 deletions sources/assets/Stride.Core.Assets/SolutionPlatform.cs
Expand Up @@ -16,21 +16,12 @@ namespace Stride.Core.Assets
[DataContract("SolutionPlatform")]
public class SolutionPlatform : SolutionPlatformPart
{
/// <summary>
/// Initializes a new instance of the <see cref="SolutionPlatform"/> class.
/// </summary>
public SolutionPlatform()
{
PlatformsPart = new SolutionPlatformPartCollection();
DefineConstants = new List<string>();
}

/// <summary>
/// Gets the alternative names that will appear in the .sln file equivalent to this platform.
/// </summary>
/// <value>The alternative names.</value>
[DataMember(20)]
public SolutionPlatformPartCollection PlatformsPart { get; private set; }
public SolutionPlatformPartCollection PlatformsPart { get; } = new SolutionPlatformPartCollection();

/// <summary>
/// Gets or sets the type of the platform.
Expand Down Expand Up @@ -58,7 +49,7 @@ public SolutionPlatform()
/// </summary>
/// <value>The define constants.</value>
[DataMember(40)]
public List<string> DefineConstants { get; private set; }
public List<string> DefineConstants { get; } = new List<string>();

/// <summary>
/// Gets or sets a value indicating whether this <see cref="SolutionPlatform"/> is available on this machine.
Expand Down Expand Up @@ -274,16 +265,15 @@ public class SolutionConfiguration
/// </summary>
public SolutionConfiguration(string name)
{
if (name == null) throw new ArgumentNullException("name");
if (name == null) throw new ArgumentNullException(nameof(name));
Name = name;
Properties = new List<string>();
}

/// <summary>
/// Gets or sets the configuration name (e.g. Debug, Release)
/// </summary>
/// <value>The name.</value>
public string Name { get; private set; }
public string Name { get; init; }

/// <summary>
/// Gets or sets a value indicating whether this instance is a debug configuration.
Expand All @@ -295,6 +285,6 @@ public SolutionConfiguration(string name)
/// Gets the additional msbuild properties for a specific configuration (Debug or Release)
/// </summary>
/// <value>The msbuild configuration properties.</value>
public List<string> Properties { get; private set; }
public List<string> Properties { get; } = new List<string>();
}
}
1 change: 1 addition & 0 deletions sources/assets/Stride.Core.Assets/Yaml/YamlAssetPath.cs
Expand Up @@ -127,6 +127,7 @@ public YamlAssetPath([NotNull] IEnumerable<Element> elements)
/// <summary>
/// The elements constituting this path.
/// </summary>
[DataMember]
public IReadOnlyList<Element> Elements => elements;

/// <summary>
Expand Down
Expand Up @@ -10,7 +10,6 @@
using System.Runtime.CompilerServices;
using System.Text;
using Mono.Cecil;
using Stride.Core;

namespace Stride.Core.AssemblyProcessor
{
Expand Down Expand Up @@ -282,25 +281,33 @@ public static IEnumerable<SerializableItem> GetSerializableItems(TypeDefinition
var fields = new List<FieldDefinition>();
var properties = new List<PropertyDefinition>();

var fieldEnum = type.Fields.Where(x => (x.IsPublic || (x.IsAssembly && x.CustomAttributes.Any(a => a.AttributeType.FullName == "Stride.Core.DataMemberAttribute"))) && !x.IsStatic && !ignoredMembers.Contains(x));
foreach (var field in type.Fields)
{
if (field.IsStatic)
continue;

// If there is a explicit or sequential layout, sort by offset
if (type.IsSequentialLayout || type.IsExplicitLayout)
fieldEnum = fieldEnum.OrderBy(x => x.Offset);
if (ignoredMembers.Contains(field))
continue;

foreach (var field in fieldEnum)
{
fields.Add(field);
if (field.IsPublic || (field.IsAssembly && field.CustomAttributes.Any(a => a.AttributeType.FullName == "Stride.Core.DataMemberAttribute")))
Eideren marked this conversation as resolved.
Show resolved Hide resolved
fields.Add(field);
}

// If there is a explicit or sequential layout, sort by offset
if (type.IsSequentialLayout || type.IsExplicitLayout)
fields.Sort((x,y) => x.Offset.CompareTo(y.Offset));

foreach (var property in type.Properties)
{
if (IsAccessibleThroughAccessModifiers(property) == false)
continue;

// Need a non-static public get method
if (property.GetMethod == null || !property.GetMethod.IsPublic || property.GetMethod.IsStatic)
if (property.GetMethod.IsStatic)
continue;

// If it's a struct (!IsValueType), we need a public set method as well
if (property.PropertyType.IsValueType && (property.SetMethod == null || !(property.SetMethod.IsAssembly || property.SetMethod.IsPublic)))
// If it's a struct (!IsValueType), we need a set method as well
if (property.PropertyType.IsValueType && property.SetMethod == null)
continue;

// Only take virtual properties (override ones will be handled by parent serializers)
Expand Down Expand Up @@ -382,6 +389,27 @@ public static IEnumerable<SerializableItem> GetSerializableItems(TypeDefinition
}
}

static bool IsAccessibleThroughAccessModifiers(PropertyDefinition property)
{
var get = property.GetMethod;
var set = property.SetMethod;

if (get == null)
return false;

bool forced = property.CustomAttributes.Any(a => a.AttributeType.FullName == "Stride.Core.DataMemberAttribute");
Eideren marked this conversation as resolved.
Show resolved Hide resolved

if (forced && (get.IsPublic || get.IsAssembly))
return true;

// By default, allow access for get-only auto-property, i.e.: { get; } but not { get => }
// as the later may create side effects, and without a setter, we can't 'set' as a fallback for those exceptional cases.
if (get.IsPublic)
return set?.IsPublic == true || set == null && property.DeclaringType.Fields.Any(x => x.Name == $"<{property.Name}>k__BackingField");
Eideren marked this conversation as resolved.
Show resolved Hide resolved

return false;
}

internal static bool IsMemberIgnored(ICollection<CustomAttribute> customAttributes, ComplexTypeSerializerFlags flags, DataMemberMode dataMemberMode)
{
// Check for DataMemberIgnore
Expand Down
2 changes: 1 addition & 1 deletion sources/core/Stride.Core.Design/Settings/SettingsFile.cs
Expand Up @@ -23,6 +23,6 @@ public SettingsFile(SettingsProfile profile)
/// Gets the settings profile to serialize.
/// </summary>
[DataMemberCustomSerializer]
public SettingsProfile Settings { get; private set; }
public SettingsProfile Settings { get; init; }
}
}
Expand Up @@ -30,7 +30,7 @@ public FieldDescriptor(ITypeDescriptor typeDescriptor, FieldInfo fieldInfo, Stri

public override bool IsPublic => FieldInfo.IsPublic;

public override bool HasSet => true;
public override bool HasSet => !FieldInfo.IsInitOnly;

public override object Get(object thisObject)
{
Expand Down
Expand Up @@ -21,11 +21,9 @@ public PropertyDescriptor(ITypeDescriptor typeDescriptor, PropertyInfo propertyI

PropertyInfo = propertyInfo;

getMethod = propertyInfo.GetGetMethod(false) ?? propertyInfo.GetGetMethod(true);
if (propertyInfo.CanWrite && propertyInfo.GetSetMethod(!IsPublic) != null)
{
setMethod = propertyInfo.GetSetMethod(!IsPublic);
}
getMethod = propertyInfo.GetMethod;
setMethod = propertyInfo.SetMethod;

TypeDescriptor = typeDescriptor;
}

Expand Down