stride3d / stride3d/stride

ObjectDescriptor serialization support does lots of work but leaves result unused.

Open
#1,843 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
C#
Stars
7.8k
Forks
1.2k
Avg merge
2d 17h
Merged PRs (30d)
49

Description

Release Type: Official Release + GitHub

Version: All

Platform(s): All

Describe the bug
The ObjectDescriptor class in Stride.Core.Reflection space has some major usage issues and this seems like a high use operation fundamental to serialization. It prepares 'MemberDescriptorBase' instances which seem in high use.

  • The method bool PrepareMember(...) is used to indicate that the member is prepared and should be serialized. It returns false if the member should not be serialized which is in conflict with the Func this method generates called 'ShouldSerialize' which goes unused. This method does a bunch of reflection and checking, then assigns a llambda with capture to the member -> e.g. (to a 'MemberDescriptorBase' type and 'member' property of type Func 'ShouldSerialize').
    • This llambda creates a closure that is capturing the member itself (because it is used within but not supplied as a parameter).
    • And the default value (which is available in the 'member' as the attribute is cached and only different except in one case...)
    • The default value, captured in the closure, is majority of the time 'null'...

*** This closure is created but is never used or invoked anywhere at all except in the testing script DescriptorTest.cs

Expected behavior
The member should be serialized based on the assigned method to determine if it should be serialized. Func 'ShouldSerialize' should be used, or this should be removed or greatly simplified. This one method is 200 lines long, performs a lot of reflection, and has limited result while assigning future GC collected memory to each instance, which goes unused.

  • Simplify method and/or use the result.
  • Change the Func signature to provide the instance of the member as an argument and move the llambda to a pure method. (capturing current state is unnecessary).
  • Move any default value adjustments (edge case having to do with matching numeric types) into the above method OR cache the modified default value directly into the 'MemberDescriptorBase' instance or the 'DefaultValueAttribute' (the default value attribute is already cached there).

Screenshots
Showing closure captures on mouse hover is removed when taking screenshot.

Additional context
I am investigating attribute usage and API documentation to learn the engine and I am raising an issue along the way. I do not have familiarity with the entire serialization system. However, following references seems all that is needed.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by tracing ObjectDescriptor.PrepareMember and its references, then inspect DescriptorTest.cs and all uses of MemberDescriptorBase.ShouldSerialize. Compare the preparation result with the serialization path and determine whether the unused delegate affects behavior or allocations. Done means the issue's requested serialization behavior and closure concerns are resolved with relevant tests.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
game-dev
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.