Abstract Away From The Serialisation/Deserialisation Implementation.

Step 0:

[X] I've read the contribution guidelines!

Issue

The api for converting between speckle and native types currently hides the dependency on extension methods named ToNative and ToSpeckle that lie in an assembly with Speckle and Converter in its name. Reading the code or documentation is the only way to realise the requirement of this dependency. This doesn’t offer a lot of flexibility in choosing how converters are consumed by the Speckle api.

Impact

Any projects where the current converter is used.

Looking through the repo, this effects:

SpeckleDynamo
SpeckleUnity
SpeckleRhino

Solution

Just an idea on how to solve this:

Converters to be injected as a dependency into a core converter class. This class will then handle conversion between types using the converters. This offers a layer of abstraction between the implementations that govern the conversion between speckle and native types and the calling code, without hiding the dependency on converters (namely ToNative and ToSpeckle).
For immediate compatibility the current Converter class will be persisted, serialising and deserialising as it currently does, however it will be expressly injected by consumers of the core converter class.
Converters that will deal with their respective native types can be implemented and then consumed by the api, without the requirement of a separate explicitly named assembly and extension methods.

Yes, I agree. The architecture around the conversion part needs to be improved. I'm really happy this issue was raised! I was thinking recently on the same lines, and this harks back to chats with @jmirtsch from almost one year ago. What you suggest is sound. Here's me saying it back to you, so we see if we agree, albeit i'll flow it differently + I'm after a day long of meetings, so my head isn't fully clear, but i did my best below :)

The Converter class gets a list of (optionally) injectable specific ISpeckleConverters ✅

When an object/list of objects gets passed in for (de)serialisation, the core Converter will first check if any of the injected converters knows how to handle the object's type.

If some are found, it picks the first one and goes forward.

If none are found, it falls back on the existing abstract routines to try and make sense of it via all that stuff (I know a few shops are relying on this feature rather hard).

Another alternative would have the Converter's serialise and deserialise methods accept an optional ISpeckleConverter which would then force them to use the conversion routines from that specific class. I'll mull on this more!

If these converters are somehow dynamically injectable via the end user (ie, use the IFCConverter for this stream), this will need to persisted somehow on the client side...

Curiosity bites: what's the use case that made you raise this issue?

I think we’re on the same page! A stack of converters can be employed for converting different types. A developer could then inject the core converter with say, standard Speckle converters (the fall back converter you mentioned, maybe one for primitive types?), and also inject some custom converters fairly easily. This core converter can then be passed around and used wherever a conversion is needed in a client. For convenience a helper method could be used to serve up a converter pre-loaded with standard converters. Another bonus with this solution is it allows the converter to be easily expanded to cover all types, not just between native and speckle. (If the need arises?).

I think an issue with the second option is although a converter can be injected and preferred for conversion, standard routines for conversions are still somewhat hidden. Whereas with the first option, the standard converters being used can be exposed. They are then present in some capacity to some consumer of the api.

That’s interesting. A issue would be serving up valid converters for the stream on the end user side. Different clients will have dependencies on their respective software, so not all converters would be usable.

There was actually no real use case in this instance, I was just trying to understand the general usage of the SpeckleCore api for any potential future use cases.

We'll need to map out the changes first in more detail, as it might be a very extensive change. I'm even mulling, because of the rather large scope, to slap a 2.0.0 label on top of it, as I dread the # lines of code in need of change and the implications (partly the consequences of the 8.30-18 meeting i'm still in)

How would the ISpeckleConverter interface look like? I think maybe just like this:

interface ISpeckleConverter {
  // takes an object and speckleises it
  public SpeckleObject ToSpeckle(object obj); 

  // the reverse of the above
  public object ToNative(SpeckleObject obj);

  // helpers, optional, not sure if needed
  public bool CanConvertToSpeckle(Type t)
  public bool CanConvertToNative(Type t)
}

I would still keep the SpeckleCore.Converter class static. Ie, when a gh/revit/dynamo etc. client reinstates itself or is created, it will register whatever sensible converters it wants/has available in the core converter, and thereafter you'll still have the simplicity of just calling SpeckleCore.Converter.ToNative( myAmazingObject ).

How would the ISpeckleConverter interface look like? I think maybe just like this:

interface ISpeckleConverter {
  // takes an object and speckleises it
  public SpeckleObject ToSpeckle(object obj); 

  // the reverse of the above
  public object ToNative(SpeckleObject obj);
}

kinda like this #56 ? 😜

I would still keep the SpeckleCore.Converter class static. Ie, when a gh/revit/dynamo etc. client reinstates itself or is created, it will register whatever sensible converters it wants/has available in the core converter, and thereafter you'll still have the simplicity of just calling SpeckleCore.Converter.ToNative( myAmazingObject ).

👏

On a more serious note, have a look at the awesome & automagical way NancyFX does

assembly scanning for discovering & auto-registering compatible 3rd party extenstions https://youtu.be/7jg0u-YaRxQ?t=1367
bootstrapping for replacing core Nancy modules

@radumg, there's no facepalm reaction, but yeah - thanks for reminding us about #56, where we conclude we don't want to force dependencies on speckle core by others, hence no interfaces. Meanwhile though, speckle did grow a bit so we could go that way in 2.0.0 or if we'll have the bandwidth to do it non-breaking in 1.x.x and on an elegant opt-in way like @RyaPorter suggests.

Nancy is a super perfect good shout actually... and perhaps the way to go forward. I've never seen the similarity between the hacky reflect-o-matic speckle approach and nancy, but will for sure have a look!

@didimitrie I think the speckle convert could be exactly like that, with one minor change. Introduce generic types instead of concrete object and SpeckleObject types, so something like:

public interface ISpeckleConverter<TSpeckle, TNative> where TSpeckle : SpeckleObject
{
    TSpeckle ToSpeckle(TNative native);

    TNative ToNative(TSpeckle speckle);
}

So the standard, generic (fall back) converter would be:

public class GenericConverter : ISpeckleConverter<SpeckleObject, object>
{
    public SpeckleObject ToSpeckle(object native)
    {
         // Some conversion logic
    }

    public object ToNative(SpeckleObject speckle)
    {
         // Some conversion logic
    }
}

The generic types allow an implementer to explicitly state their argument and return types and serves as a concise way to distinguish between each implementer via their conversion types.

In regard to keeping the SpckleCore.Converter static, how would it get the registered converters, or get converters to use? This feels like it's introducing a lot of static state for the convenience of a simple method call? I think it's possible to have a convenient method call for converting types, similar to SpeckleCore.Converter.Serialise(something), whilst avoiding global data. But at the cost of slightly more configuration.

As an idea of architecture, when the SpeckleCore.Converter is instantiated it is injected factory class that serves up converters for the core converter. This would happen within the constructor of both the sender/receiver. The Converter instance would contain Serialisation/Deserialisation members that can be called as conveniently as the current static class, something like theConverter.Serialise(someObject). But this would only be a wrapper around the factory call, where types are passed in and a converter pops out. I admit this adds some extra lines of code, but it does decouple the state of the converters and Converter from the global app domain.

An simple implementation of the ConverterFactory class:

public class ConverterFactory
{
    public void RegisterConverter<Tspeckle, TNative>
        (ISpeckleConverter<TSpeckle, TNative> speckleConverter) where TSpeckle : SpeckleObject
    {
         // Register the converter.
    }

    // This might not be required?
    public void UnregisterConverter<Tspeckle, TNative>
        (ISpeckleConverter<TSpeckle, TNative> speckleConverter)
    {
         // Unregister the given converter.
    }

    public dynamic GetConverter(Type speckleType, Type nativeType)
    {
         // Get the Converter
    }
}

A simple implementation of the Converter class. This would serve as a wrapper around the factory and converter implementations, allowing a simple call ,converter.Serialise(anObject):

public class Converter
{
    public Converter(ConverterFactory factory)
    {
         Factory = factory;
    }

    ConverterFactory Factory { get; }

    public dynamic Serialise(dynamic native)
    {
        // Instruct the factory to return a converter that will serialise from the run-time native type to the speckle type.
        // Call ToSpeckle on this speckle converter.
        // return the speckle type.
    }

    public dynamic Deserialise(SpeckleObject speckle)
    {
        // Instruct the factory to return the converter that will deserialise from the speckle type to a native type.
        // Call ToNative on this speckle converter.
        // return the native type.
    }
}

At compile-time the implementer of ISpeckleConverter that will be returned by the factory is not known. Nor is the exact type of SpeckleObject known (it could be a derived type, or the base SpeckleObject). The use of dynamic circumvents this problem. And although not necessary with the SpeckleObject it does avoid boxing. A dynamic object can be returned in place of a ISpeckleConverter as the only objects that will be returned by the factory will implement the ISpeckleConverter interface (An indirect constraint defined by RegisterConverter). And because of the type constraint on TSpeckle, this returned converter will (if implemented properly) safely convert the given type to some SpeckleObject. This provides some guarantee of safe calls to ToSpeckle and ToNative.

Configuring the factory and converter doesn’t take a lot of lines of code (the majority is registering converters):

// Create a factory and inject it into a core converter. Multiple converters can be registered with the factory.
var factory = new ConverterFactor().RegisterConverter(new GenericConverter());
var converter = new Converter(factory);

Once configured, the below call can be made anywhere the core converter is in scope

var speckleObject = converter.Serialise(anObject);
var nativeObject = converter.Deserialise(aSpeckleObject);

This is easily adapted and would require only a new implementation of a converter that scans for 3rd party extensions to be registered to use something similar to the Nancy approach mentioned.

speckleworks / SpeckleCore