specklesystems / speckle-unity

AEC Interoperability for Unity through Speckle
https://speckle.systems/tag/unity/
Apache License 2.0
55 stars 20 forks source link

Serialised SpeckleProperties #27

Closed JR-Morgan closed 3 years ago

JR-Morgan commented 3 years ago

In regards to editor mode receiving of streams. I would really like SpeckleProperties.Data to be serialised so that the SpeckleProperties of streams received in editor mode can be used during play mode.

https://github.com/specklesystems/speckle-unity/blob/8a1e781e9e849132e960fa846f477359465e4e1e/Assets/Speckle%20Connector/SpeckleProperties.cs#L10-L13

Unity doesn't support serialisation of Dictionary<K,V> types which makes SpeckleProperties effectively unusable for most applications of Editor mode receiving.

Steps to reproduce issue:

  1. Receive a stream through the StreamManager in editor mode.
  2. SpeckleProperties.Data will be null in play mode and after reloading scripts or scene.

In the past, I have used several different open-source implementations of a serialisable dictionary, but they have all had their own quirks that mean that it wouldn't be a simple drop-in replacement for the current Dictionary type. Perhaps @haitheredavid could suggest some possible solutions.

JR-Morgan commented 3 years ago

Possible to do something similar to the code example here: https://docs.unity3d.com/ScriptReference/ISerializationCallbackReceiver.html or perhaps a more reusable solution to create a SerialisedDictionary type. However there is an additional consideration concerning serialising List<object> .

haitheredavid commented 3 years ago

@JR-Morgan good looks on the link above! That could definitely be a good way of tackling that solution. I'm not sure what's causing the SpeckleProperties.Data to be cleared during runtime, but that is mos def something that needs to be fixed. I am planning on building out the Receiver object so it takes care of the data being instanced from a stream. I imagine that will lead to the issue with the data being cleared.

Regarding the serialization, could you give me an example of how you would use that? Are you thinking the Data in the Dictionary would be accessible via a [SerializeField] with maybe a function to hook into a desired object?

I think I would like to make the object in the dictionary a [System.Serialzed] object and use the Convert.Unity class to parse the data into the object... maybe something like this? (need to test this thou)

 [Serializable]
 public class BasicSpeckleData { }

 [Serializable]
 public class SpeckleProperties : MonoBehaviour
 {
    public List<BasicSpeckleData >data; //serializes
 }

If we wanted to have something more generic, I think we could use something like this...

 [Serializable]
 public class GenericSpeckleData<T> where T : ISerializable { }

 [Serializable]
 public class YourSupeDopeSpeckleData : GenericSpeckleData<DopeData> { }
haitheredavid commented 3 years ago

@JR-Morgan meow meow 🐱 tested!

Here are the classes that could Serializable

 [Serializable]
    public class BasicData {

        public string value = "DopeData";

    }

    [Serializable]
    public class BasicData<TData> {

        public TData typeValue = default;
    }

    [Serializable]
    public class StringBasicData : BasicData<string> { }

    [Serializable]
    public class NestedClassData : BasicData<NestedData> { }

    [Serializable]
    public class NestedData {

        public int data = 10;
        public string value = "EvenDoperData";
    }

Here is what the mono-b looks like

 public class DataTest : MonoBehaviour {

        [SerializeField] private BasicData basicData;
        [SerializeField] private  NestedClassData nestedClassData;
        [SerializeField] private  StringBasicData stringBasicData;
    }

Editor view

image

JR-Morgan commented 3 years ago

@haitheredavid Interesting, I'm not quite sure how this is going to work with the mixed types that SpeckleProperties.Data.Values stores.

From Revit I'm receiving values of types Strings, Double, Boolean, RevitLevel, List<Parameter>, Line, Mesh, RenderMaterial, Polycurve, etc.

Ideally I would like all of these to persist from edit mode to play mode runtime and therefore be available during play mode.

I don't see a way of using Unity's script serialisation to do this, at least not in the current form of SpeckleProperties.

Perhaps the root Base JSON can be stored and re-convert at the start of play mode runtime? This would re-create the GameObjects each time, exactly the same with intact SpeckleProperties

JR-Morgan commented 3 years ago

I think I might be slightly misunderstanding what you are proposing here. 😅 I'm not quite sure how the ConverterUnity would be used with your proposed classes and exactly what data BasicData would hold.

haitheredavid commented 3 years ago

I think we are sorta dancing around the same idea. The test above is trying to show a solution for how we could serialize different speckle properties so they are accessible during edit and play mode while also being visible in the editor. I'll try tackling a more legible sketch of this in the next couple of days.

teocomi commented 3 years ago

Nice convo! I think a solution could be adding setters and getters to the SpeckleProperties Data so that if in editor mode data is serialized to a private json string. In game mode, when retrieving it, if it's stored in the json string it gets deserialized and set.

haitheredavid commented 3 years ago

@teocomi I took a pass at implementing something like dis?. I set two different fields that pretty much store the same data with the only difference being that one is in a lil wrapper class that stores the key string from the Dict<string, obj>. I'm not entirely sure if each obj will be of Base type or if this is the best way to go about serializing this data... Would it make more sense to reference PropertyInfo instead like in GetInstanceMembers()?

@JR-Morgan you could call SerializedData getter to fetch all the object data from that collection.

JR-Morgan commented 3 years ago

Hi @haitheredavid I've taken a look at your changes to SpeckleProperties on #28. It looks like a few of the data types I'm receiving from Revit don't inherit from Base. Mostly primitives or lists.

I had been working on my own solution to this problem which you might be interested in. I'm using Operations Serialisation for Base types, and then JsonUtility for everything else.

I quite like the idea of keeping the Dictionary as the only public way to access the speckle data, so I'm reconstructing it from the Lists that I'm serialising.

I had to do some funky generic wrapping using reflection to get JsonUtility to work since it requires fields to be declared as their actual type (not object) in order to serialize and deserialize.

haitheredavid commented 3 years ago

@JR-Morgan oh oh I like your solution! I blindly tried using the same JsonUtility function but hit a similar road bump as you, I'm really happy that you got it working with the wrapper reflection 🛩ī¸. woot woot! 👍đŸģ

JR-Morgan commented 3 years ago

@haitheredavid - Excellent, I'm glad that you like it. Should I submit a PR for it? or would you prefer to merge the changes into your PR?

teocomi commented 3 years ago

I haven't played with this at all yet, so please take my comment with a pinch of salt, but it could be worth trying to see if the serialization methods in Core could be used instead as they handle Base objects natively: https://github.com/specklesystems/speckle-sharp/blob/master/Core/Core/Api/Operations/Operations.Serialize.cs#L18

JR-Morgan commented 3 years ago

Great minds must think alike @teocomi because both mine and @haitheredavid's solutions do use Operations for Base objects. But I'm also using JsonUtility just to catch any non-Base objects that Operations can't serialise (primitives and lists).

I think ideally, if it were possible just to use Operations serialisation then it would be better than having these two different serialisation systems working side-by-side, but this is a problem reasonably self contained in SpeckleProperties so I don't see it causing problems.

teocomi commented 3 years ago

Ah sweet! So, one way to achieve that (I think) would be the following:

JR-Morgan commented 3 years ago

Interesting idea @teocomi, If SpeckleProperties.Data is a Base then that would mean there is less serialization code in SpeckleProperties

I'm just trying to think about how we can allow modifications to theSpeckleProperties's data in a convenient way that would update the Base object. If you just expose the GetMembers() method, that works great for reading, but adding or modifying members would be a little more difficult to get those changes to update the Base.

haitheredavid commented 3 years ago

@teocomi getting limber with that Base đŸĒ€. I took a pass at trying with my recent push. That does make things a lot cleaner 🗡ī¸

@JR-Morgan I still think your solution is valuable since it brings all the data into a format that we could use in the editor. GetMembers() still returns a format that is cannot be easily serialized into the editor so I think we would need to utilize your reflection logic. Maybe we could set a class that inherits from your Wrapper<> and set the generic to be type Base. That way if we find that down the road we want to visualize the properties in the editor we can build out a simple class that looks like dis?

// Base object with properties we want in the editor
FutureAwesomeBase : Base 
{ }

FutureSpeckleAwesomeData : Wrapper<FutreAwesomeData>
{

void Unwrap(FutureAwesomeBase obj)
{
// unwrap properties for serializing into editor
}

FutureAwesomeBase Wrap( )
{
// code for rebuilding base for stream
{

As for updates for catching any updates from the editor, we could utilize the OnValidate event from Monobehaviour so we know if any data was changed after loading from stream.

EDIT: Actually after writing this out, I'm thinking we shouldn't do this. đŸ¤ĻđŸģ ... Using Wrapper<> would still make us rely on the JsonUtility class and to @teocomi point of relying on Operations instead. With switching SpeckleProperties.Data to Base we get all the necessary serializing we were originally looking for. Instead of a wrapper class for different Base types that would be unwrapped it would probably make more sense to utilize the SpeckleKit functionality that would map the desired base object to a specific MonoBehaviour.

JR-Morgan commented 3 years ago

@haitheredavid I've taken a look at your recent push. It seems that the Base object will only be updated if the Data is set (i.e. the setter is given a new Dictionary) rather than just modified. For example:


mySpeckleProperties.Data = someDictionary; //This works fine!

mySpeckleProperties.Data.Add(someKey, someValue);  //This does nothing since SpeckleProperties.Data returns a new Dictionary

mySpeckleProperties.Data[someKey] = someValue; //This also does nothing!

I have thought about it and perhaps instead of trying to check for changes to the SpeckleProperties.Data and then updating the Base object. We could just recreate the Base every time we need to serialize (using ISerializationCallbackReceiver). Since we are only using the Base for serialization, I think it makes sense for the Dictionary to update the Base, not the other way around. Some thing like this perhaps.

Getting inspector UI to allow users to view/edit values in inspector is a separate issue that I think will require some custom UI, perhaps similar to what is done here.

haitheredavid commented 3 years ago

@JR-Morgan ah nice tests, it's great to see a solution to handling sending individual properties... but I'll have to admit I'm not entirely sure I fully grasp the issue with setting the data that way... isn't the data coming in from SpeckleProperties being passed as a full Dictionary<string, object> ? Is this solution meant to bring some functionality for setting /updating the data of Base object and sending that back into the stream?

EDIT : Regarding the UI stuff, I think it makes sense to build MonoBehaviour for any of the Base.Objects that we want to have converted into unity. Those scripts can inherit from SpeckleProperties and we can make the SetData call virtual so each object can handle the data however it wants and populate the editor UI however that lil' fellas like it wants too.

JR-Morgan commented 3 years ago

Is this solution meant to bring some functionality for setting /updating the data of Base object and sending that back into the stream?

That's correct, although this is already supported, but does not worked correctly when switching from editor to play mode.

I think that ideally, any solution to this issue should allow SpeckleProperties.Data to be updated like what is already supported for play mode (i.e the ways that I described previously), otherwise we are adding one feature (Serialization) but removing another (Ability to modify Data).

isn't the data coming in from SpeckleProperties being passed as a full Dictionary<string, object> ?

That's true for geometry coming in from Speckle. What about objects created from custom scripts? or properties that are updated by custom scripts? I feel that these are very valid use cases for the Unity connector that are already supported, but don't work when switching from editor to play mode.

Sorry, I didn't really explain my use case very well. In my project using this connector, I am adding custom boolean key-values to the SpeckleProperties.Data of certain objects in the scene, and while I don't actually need this to work in editor mode, my early solution did help me troubleshoot some problems with my project.

teocomi commented 3 years ago

Thanks both! Great convo and code samples here 🤘

I think this solution ticks all the boxes, although I haven't personally tested it.

I'm happy to jump on a quick call to review together if you guys feel the need to align, otherwise, I'm happy for that to be turned into a PR.

haitheredavid commented 3 years ago

@JR-Morgan ah got ya now! 👍đŸģ thanks for laying out that example. I mos def agree that this gives us the flexibility needed. I haven't tested it yet but I will be getting my hands back into unity tomorrow!

A quick side thought - will this make it so any properties that are added during playmode will be saved after playmode is stopped? If that is the case, we might want to add a bool toggle so a user can decide if they want this functionality since that sorta breaks the typical workflow of a unity object.

JR-Morgan commented 3 years ago

@haitheredavid That's right, any modifications made during play mode will not persist to edit mode as the scene does not get modified.

I don't know off hand how we could support this if we wanted to. There are some third party packages that can also do this, so it is possible.

You are right in that this would not be normal behaviour so should not be the default if we do implement it.

From the inspector, you can copy the state of a single Component, come out of play mode, and then paste the changes that way, but this would be one component at a time, not really applicable for the hundreds or thousand SpeckleProperties a stream might have.

haitheredavid commented 3 years ago

ah good to know!. I would prefer to not have the data move back into edit from playmode since that isn't how unity is expected to work and it's one less thing for the connector to handle 🗡ī¸ .

haitheredavid commented 2 years ago

@JR-Morgan I cam across an issue with how ISerializationCallbackReceiver works. If you click on any object that has a speckle SpeckleProps component you will notice a huge performance. 📉 It seems like the interface gets called every frame when you have any object selected with that implementation which basically sends the properties from Base into Operations.Serialize each call. I did implement a fix SpeckleData and SpeckleProperties. I'll try to get this into a pr for you to review soon, maybe next week or so!