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

struct field not being deserialized #54

Open
snarlynarwhal opened this issue Feb 17, 2023 · 9 comments
Open

struct field not being deserialized #54

snarlynarwhal opened this issue Feb 17, 2023 · 9 comments
Labels
bug Something isn't working

Comments

@snarlynarwhal
Copy link

Hello, I have a model with fields that consist of primitive types and a custom struct. The primitive fields get deserialized just fine, but the struct field does not and retains it's default values. Here are the TomlModelOptions I am using:

public static readonly TomlModelOptions TomlOptions = new TomlModelOptions {
    IgnoreMissingProperties = true,
    IncludeFields = true,
    ConvertPropertyName = name => name,
    ConvertFieldName = name => name
};

Any ideas on how I can get it to also deserialize the custom struct field?

@xoofx xoofx added the question Further information is requested label Feb 17, 2023
@xoofx
Copy link
Owner

xoofx commented Feb 17, 2023

Could you post a reproducible simple Program.cs just to make sure about your exact use case?

@xoofx
Copy link
Owner

xoofx commented Feb 17, 2023

I can see in the tests that there are no tests with struct, so it is likely that I didn't need struct for my use cases and so didn't implement anything to support this (it always requires a special path with reflection, as you need to reassign the value type back after setting its fields to where it should be stored)

@xoofx
Copy link
Owner

xoofx commented Feb 17, 2023

(So, as a workaround, if you can, use class for now. It could be that I won't support this scenario - because I don't have a personal interest to implement this - but would emit an error instead)

@snarlynarwhal
Copy link
Author

I appreciate the fast response. I just wanted to mention this since I assumed it was a bug. Thanks for the suggestion. Changing the struct to a class is unideal for my situation which requires the use of structs for performance. I also pass the value around a lot and don't want to pass a reference. The struct fields change, so I would have to invoke a clone/copy method everywhere. But I did implement an intermediary class just for the deserialization. A bit of a hack, but it works just fine 👍

Here is a simple reproducible script in case you decide you would like to implement it:

using Newtonsoft.Json;
using System;
using Tomlyn;

class Program
{
    public class Character
    {
        public string Name;
        public int Level;
        public Stats Stats;
    }

    public struct Stats
    {
        public int Strength;
        public int Dexterity;
        public int Wisdom;
    }

    const string toml = 
        @"
        Name = ""Tom""
        Level = 10
        [Stats]
        Strength = 7
        Dexterity = 8
        Wisdom = 9
        ";

    public static readonly TomlModelOptions TomlOptions = new TomlModelOptions
    {
        IgnoreMissingProperties = true,
        IncludeFields = true,
        ConvertPropertyName = name => name,
        ConvertFieldName = name => name
    };

    static void Main(string[] args)
    {
        Character character = Toml.ToModel<Character>(toml, null, TomlOptions);
        string json = JsonConvert.SerializeObject(character, Formatting.Indented);
        Console.WriteLine(json);
        /*
        {
            "Name": "Tom",
            "Level": 10,
            "Stats": {
                "Strength": 0,
                "Dexterity": 0,
                "Wisdom": 0
            }
        }
        */
        Console.ReadLine();
    }
}

Thanks again for the fast response and help!

@xoofx xoofx added bug Something isn't working and removed question Further information is requested labels Feb 18, 2023
@xoofx
Copy link
Owner

xoofx commented Feb 18, 2023

Changing the struct to a class is unideal for my situation which requires the use of structs for performance.

Yeah, Tomlyn was not really designed with performance in mind 🙂 (the parser is not wasting GC but otherwise, it's using reflection when mapping to .NET objects which kills quickly performance anyway...) but mainly to help loading easily config files, hence why I didn't bothered with structs (afair)

@snarlynarwhal
Copy link
Author

If you point me in the right direction, I can take a look and attempt a PR when I get some time, if you're open to adding support for structs.

@snarlynarwhal
Copy link
Author

@xoofx I just wanted to follow up and see if you would be accept a PR that added support for structs. Let me share my use case: I am a game developer who uses toml + tomlyn for game data. Right now I use toml to define mods. A lot of the built-in Unity data types as well as our own custom data types are structs for various reasons ranging from performance to the need to pass by value a lot. So, while the class hack works for now, it's unideal for our project needs long term. Let me know what you think! I am happy to give it a go, but wanted to run it by you before attempting a PR.

@xoofx
Copy link
Owner

xoofx commented Mar 3, 2023

If you point me in the right direction, I can take a look and attempt a PR when I get some time, if you're open to adding support for structs.

My apologize, forgot to come back to you.

Most of the code is in SyntaxToModelTransform.

Then there is the code associated with list, primitives and managed object accessors (for example: StandardObjectDynamicAccessor)

The challenge for working with struct is that when you start to update a property/field and the target type is a struct, you would need to visit the stack of objects being written and apply the set on the parent objects (and so on, if the parent is the struct, you need to repeat this).

The class SyntaxToModelTransform contains the stack of objects being written to via _objectStack. I would believe that this could be used as is to traverse parent objects.

@snarlynarwhal
Copy link
Author

Thanks so much for this, I will take a look when I have some time and see if I can figure it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants