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

Add UrlReference attempt number 2 #564

Merged
merged 9 commits into from Dec 20, 2019
9 changes: 9 additions & 0 deletions sources/assets/Xenko.Core.Assets/AssetRegistry.cs
Expand Up @@ -11,6 +11,7 @@
using Xenko.Core.Serialization.Contents;
using Xenko.Core.VisualStudio;
using Xenko.Core.Yaml.Serialization;
using Xenko.Core.Serialization;

namespace Xenko.Core.Assets
{
Expand Down Expand Up @@ -289,6 +290,14 @@ public static IReadOnlyList<Type> GetAssetTypes(Type contentType)
lock (RegistryLock)
{
var currentType = contentType;
if (UrlReferenceHelper.IsGenericUrlReferenceType(currentType))
{
currentType = UrlReferenceHelper.GetTargetContentType(currentType);
}
else if (UrlReferenceHelper.IsUrlReferenceType(contentType))
{
return GetPublicTypes().Where(t => IsAssetType(t)).ToList();
}
List<Type> assetTypes;
return ContentToAssetTypes.TryGetValue(currentType, out assetTypes) ? new List<Type>(assetTypes) : new List<Type>();
}
Expand Down
@@ -0,0 +1,53 @@
using System;
using Xenko.Core;
using Xenko.Core.Annotations;
using Xenko.Core.IO;
using Xenko.Core.Reflection;
using Xenko.Core.Serialization;
using Xenko.Core.Yaml;
using Xenko.Core.Yaml.Events;
using Xenko.Core.Yaml.Serialization;

namespace Xenko.Core.Assets.Serializers
{
/// <summary>
/// A Yaml serializer for <see cref="UrlReference"/>
/// </summary>
[YamlSerializerFactory(YamlAssetProfile.Name)]
internal class UrlReferenceSerializer : AssetScalarSerializerBase
{
public override bool CanVisit(Type type)
{
return typeof(UrlReference).IsAssignableFrom(type);
}

public override object ConvertFrom(ref ObjectContext context, Scalar fromScalar)
{
if (!AssetReference.TryParse(fromScalar.Value, out var guid, out var location))
{
throw new YamlException(fromScalar.Start, fromScalar.End, "Unable to decode url reference [{0}]. Expecting format GUID:LOCATION".ToFormat(fromScalar.Value));
}

var urlReference = UrlReferenceHelper.CreateReference(context.Descriptor.Type, guid, location.FullPath);

return urlReference;
}

public override string ConvertTo(ref ObjectContext objectContext)
{
if (objectContext.Instance is UrlReference urlReference)
{
var attachedReference = AttachedReferenceManager.GetAttachedReference(urlReference);

if (attachedReference != null)
{
return $"{attachedReference.Id}:{urlReference.Url}";
}
}

throw new YamlException($"Unable to extract url reference from object [{objectContext.Instance}]");
}


}
}
4 changes: 3 additions & 1 deletion sources/core/Xenko.Core.Design/Module.cs
Expand Up @@ -2,9 +2,9 @@
// Distributed under the MIT license. See the LICENSE.md file in the project root for more information.

using System.ComponentModel;

using Xenko.Core.Mathematics;
using Xenko.Core.Reflection;
using Xenko.Core.Serialization;
using Xenko.Core.TypeConverters;

namespace Xenko.Core
Expand All @@ -16,6 +16,7 @@ public static void Initialize()
{
// Make sure that this assembly is registered
AssemblyRegistry.Register(typeof(Module).Assembly, AssemblyCommonCategories.Assets);

TypeDescriptor.AddAttributes(typeof(Color), new TypeConverterAttribute(typeof(ColorConverter)));
TypeDescriptor.AddAttributes(typeof(Color3), new TypeConverterAttribute(typeof(Color3Converter)));
TypeDescriptor.AddAttributes(typeof(Color4), new TypeConverterAttribute(typeof(Color4Converter)));
Expand All @@ -28,6 +29,7 @@ public static void Initialize()
TypeDescriptor.AddAttributes(typeof(Vector2), new TypeConverterAttribute(typeof(Vector2Converter)));
TypeDescriptor.AddAttributes(typeof(Vector3), new TypeConverterAttribute(typeof(Vector3Converter)));
TypeDescriptor.AddAttributes(typeof(Vector4), new TypeConverterAttribute(typeof(Vector4Converter)));
TypeDescriptor.AddAttributes(typeof(UrlReference), new TypeConverterAttribute(typeof(UrlReferenceConverter)));
}
}
}
Expand Up @@ -20,12 +20,13 @@ public static bool CanConvert([NotNull] Type sourceType, [NotNull] Type destinat
if (sourceType == null) throw new ArgumentNullException(nameof(sourceType));
if (destinationType == null) throw new ArgumentNullException(nameof(destinationType));

var context = new DestinationTypeDescriptorContext(destinationType);
// already same type or inherited (also works with interface), or
// implements IConvertible, or
// can convert from source type to target type
return destinationType.IsAssignableFrom(sourceType) ||
(typeof(IConvertible).IsAssignableFrom(sourceType) && Type.GetTypeCode(destinationType) != TypeCode.Object) ||
TypeDescriptor.GetConverter(sourceType).CanConvertTo(destinationType) || TypeDescriptor.GetConverter(destinationType).CanConvertFrom(sourceType);
TypeDescriptor.GetConverter(sourceType).CanConvertTo(destinationType) || TypeDescriptor.GetConverter(destinationType).CanConvertFrom(context, sourceType);
}

/// <summary>
Expand Down Expand Up @@ -69,10 +70,11 @@ public static bool TryConvert(object source, [NotNull] Type destinationType, out
return true;
}
// Try to convert using the target type converter
var context = new DestinationTypeDescriptorContext(destinationType);
converter = TypeDescriptor.GetConverter(destinationType);
if (converter.CanConvertFrom(sourceType))
if (converter.CanConvertFrom(context, sourceType))
{
target = converter.ConvertFrom(source);
target = converter.ConvertFrom(context, System.Globalization.CultureInfo.CurrentCulture, source);
return true;
}
}
Expand All @@ -92,5 +94,43 @@ public static bool TryConvert(object source, [NotNull] Type destinationType, out
target = null;
return false;
}

public static Type GetDestinationType(ITypeDescriptorContext context)
{
if (context is DestinationTypeDescriptorContext c) return c.DestinationType;

return null;
}

private class DestinationTypeDescriptorContext : ITypeDescriptorContext
{
public DestinationTypeDescriptorContext(Type destinationType)
{
DestinationType = destinationType;
}

public Type DestinationType { get; }

public IContainer Container => null;

public object Instance => null;

public PropertyDescriptor PropertyDescriptor => null;

public object GetService(Type serviceType)
{
return null;
}

public void OnComponentChanged()
{

}

public bool OnComponentChanging()
{
return true;
}
}
}
}
@@ -0,0 +1,39 @@
// Copyright (c) Xenko contributors (https://xenko.com) and Silicon Studio Corp. (https://www.siliconstudio.co.jp)
// Distributed under the MIT license. See the LICENSE.md file in the project root for more information.

using System;
using System.ComponentModel;
using System.Globalization;
using Xenko.Core.Serialization;

namespace Xenko.Core.TypeConverters
{
/// <summary>
/// Defines a type converter for <see cref="UrlReference"/>.
/// </summary>
public class UrlReferenceConverter : BaseConverter
{

public UrlReferenceConverter()
{

//var type = typeof(Xenko.Core.Serialization.UrlReference);
//Properties = new PropertyDescriptorCollection(new System.ComponentModel.PropertyDescriptor[]
//{
// new Reflection.PropertyDescriptor(type.GetProperty(nameof(Xenko.Core.Serialization.UrlReference.Url))),
//});
}

public override bool CanConvertFrom(ITypeDescriptorContext context, Type sourceType)
{
return UrlReferenceHelper.IsUrlReferenceType(TypeConverterHelper.GetDestinationType(context));
}

public override object ConvertFrom(ITypeDescriptorContext context, CultureInfo culture, object value)
{
var attachedReference = AttachedReferenceManager.GetAttachedReference(value);
var destinationType = TypeConverterHelper.GetDestinationType(context);
return UrlReferenceHelper.CreateReference(destinationType, attachedReference.Id, attachedReference.Url);
}
}
}
Expand Up @@ -85,10 +85,8 @@ public static T CreateProxyObject<T>(IReference reference) where T : class, new(
public static T CreateProxyObject<T>(AssetId id, string location) where T : class, new()
{
var result = new T();
var attachedReference = GetOrCreateAttachedReference(result);
attachedReference.Id = id;
attachedReference.Url = location;
attachedReference.IsProxy = true;
InitializeProxyObject(result, id, location);

return result;
}

Expand Down Expand Up @@ -121,11 +119,15 @@ public static object CreateProxyObject(Type type, AssetId id, string location)
}
}
var result = emptyCtor.Invoke(EmptyObjectArray);
var attachedReference = GetOrCreateAttachedReference(result);
InitializeProxyObject(result, id, location);
return result;
}
private static void InitializeProxyObject(object proxyObject, AssetId id, string location)
{
var attachedReference = GetOrCreateAttachedReference(proxyObject);
attachedReference.Id = id;
attachedReference.Url = location;
attachedReference.IsProxy = true;
return result;
}
}
}
@@ -0,0 +1,56 @@
using System;
using System.Collections.Generic;
using System.Text;
using Xenko.Core.Annotations;
using Xenko.Core.Assets;

namespace Xenko.Core.Serialization.Serializers
{
/// <summary>
/// Serializer base class for for <see cref="UrlReference"/>.
/// </summary>
public abstract class UrlReferenceDataSerializerBase<T> : DataSerializer<T>
where T: UrlReference
{
/// <inheritdoc/>
public override void Serialize(ref T urlReference, ArchiveMode mode, [NotNull] SerializationStream stream)
{
if (mode == ArchiveMode.Serialize)
{
var attachedReference = AttachedReferenceManager.GetAttachedReference(urlReference);
if(attachedReference == null)
{
throw new InvalidOperationException("UrlReference does not have an AttachedReference.");
}

stream.Write(attachedReference.Id);
stream.Write(attachedReference.Url);
}
else
{
var id = stream.Read<AssetId>();
var url = stream.ReadString();

urlReference = (T)UrlReferenceHelper.CreateReference(typeof(T), id, url);
}
}
}

/// <summary>
/// Serializer for <see cref="UrlReference"/>.
/// </summary>
public sealed class UrlReferenceDataSerializer : UrlReferenceDataSerializerBase<UrlReference>
{

}

/// <summary>
/// Serializer for <see cref="UrlReference{T}"/>.
/// </summary>
/// <typeparam name="T">The type of asset.</typeparam>
public sealed class UrlReferenceDataSerializer<T> : UrlReferenceDataSerializerBase<UrlReference<T>>
where T : class
{

}
}
@@ -0,0 +1,94 @@
using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Globalization;
using System.Text;
using Xenko.Core.Annotations;
using Xenko.Core.Assets;
using Xenko.Core.Serialization.Contents;
using Xenko.Core.Serialization.Serializers;

namespace Xenko.Core.Serialization
{
/// <summary>
/// Represents a Url to an asset.
/// </summary>
[DataContract("urlref", Inherited = true)]
[DataStyle(DataStyle.Compact)]
[ReferenceSerializer]
[DataSerializer(typeof(UrlReferenceDataSerializer))]
public class UrlReference
Copy link
Member

@xen2 xen2 Dec 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if it wouldn't be better as a struct?
Especially since we also have to check for IsEmpty so there is two ways to have invalid values currently (either the UrlReference instance is not set, or IsEmpty is true). That might be little bit annoying to tests those two things everytime.

This would also remove lot of null check as well.
Not sure if all the converters and editor code will like it, but might be worth a try don't you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AttachedReferenceManager requires objects because it uses a ConditionalWeakTable to attach references. Ideally the UrlReference types would not have a default constructor so you could not instantiate "empty" ones. But various parts of the asset stuff and UI require a default constructor for them to work. It would require a lot more changes to accomplish this without default constructors.

I have created an interface and a base class so the UrlReference and UrlReference<T> types can both be sealed. I have also tidied up a bunch of things:

  • Fix some formatting (remove blank likes etc.)
  • Fixed/added doc comments
  • Fixed some exceptions being thrown (Had parameters around the wrong way).
  • Removed/sorted using statements
  • Added License notice to the tops of files

Copy link
Member

@xen2 xen2 Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point about attached reference, makes sense. Thanks!

Wondering, could we do it the other way around and forbid to use null/empty string then?
i.e. if not set in editor, UrlReference instance itself is null. If instance is set, it has to have an actual valid Url value?
This implies UrlReference wouldn't have an empty constructor anymore too.

Note: if annoying/complex, no need to do that now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did have a couple of solutions that didn't expose the default constructor that worked mostly. For example, make the default constructor internal and change how instantiation code works. I had a couple of different versions of this that "worked" but I wasn't too happy with the bits I changed.

Though I might have just thought of an option....

But even still, doing it this way meant that using collection properties i.e. List<UrlReference> don't work because the UI code cannot instantiate an instance without a default constructor. So would either have to leave this feature out or change the UI code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, let's keep it like this for now, thanks.

{
private string url;

public UrlReference()
{

}

/// <summary>
/// Create a new <see cref="UrlReference"/> instance.
/// </summary>
/// <param name="url"></param>
public UrlReference(string url)
{
if (string.IsNullOrWhiteSpace(url))
{
throw new ArgumentException($"{nameof(url)} cannot be null or empty.", nameof(url));
}

Url = url;
}

[DataMember(10)]
public string Url
{
get => url;
internal set
{
if (string.IsNullOrEmpty(value))
{
throw new ArgumentNullException(nameof(value));
}
url = value;
}
}

[DataMemberIgnore]
public bool IsEmpty => string.IsNullOrEmpty(url);

/// <inheritdoc/>
public override string ToString()
{
return $"{Url}";
}


}

/// <summary>
/// Represents a Url to an asset of type <see cref="T"/>.
/// </summary>
/// <typeparam name="T">The type off asset.</typeparam>
[DataStyle(DataStyle.Compact)]
[DataSerializer(typeof(UrlReferenceDataSerializer<>), Mode = DataSerializerGenericMode.GenericArguments)]
public sealed class UrlReference<T> : UrlReference
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same goes for this (struct).
Might need a IUrlReference with Url and IsEmpty to act as common base.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see reply above.

where T : class
{
public UrlReference() : base()
{

}

/// <summary>
/// Create a new <see cref="UrlReference{T}"/> instance.
/// </summary>
/// <param name="url"></param>
public UrlReference(string url) : base(url)
{
}

}


}