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

Fix duplicate fields and buttons in the inspector when using inheritance #16

Merged
merged 1 commit into from
Feb 5, 2024

Conversation

PJBarczyk
Copy link
Contributor

The following pull request provides a solution to the issue of duplicated fields in the inspector when using inheritance (#15 and partially #12). The cause of the problem is the concatenation of the members of the current type and its base types.

Solution

The proposed solution is to use Distinct to remove duplicates, using both MemberInfo.Name and MemberInfo.DeclaringType as the equality check:

class MemberInfoEqualityComparer : IEqualityComparer<MemberInfo>
{
    public static MemberInfoEqualityComparer Instance { get; } = new();

    public bool Equals(MemberInfo x, MemberInfo y)
    {
        return x.Name == y.Name && x.DeclaringType == y.DeclaringType;
    }

    public int GetHashCode(MemberInfo obj)
    {
        return HashCode.Combine(obj.Name, obj.DeclaringType);
    }
}

static IEnumerable<MemberInfo> GetMembersIncludingBaseNonPublic(Type type, BindingFlags bindingAttr = BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance | BindingFlags.Static)
{
    return EnumerateTypeHierarchy(type)
        .Reverse()
        .SelectMany(t => t.GetMembers(bindingAttr))
        .Distinct(MemberInfoEqualityComparer.Instance);
}

The same logic was applied to GetAllFieldsIncludingBaseNonPublic, GetAllPropertiesIncludingBaseNonPublic and GetAllMethodsIncludingBaseNonPublic.

I've decided to use Reverse operator too, to order the fields from the base types first, then the fields from the current type - same as the default Unity inspector does.

Tests

Recreated the example from #13, adding private methods of same name too:

// file: A.cs
public class A : MonoBehaviour
{
    public string aPublic;
    [SerializeField] protected string aProtected;
    [SerializeField] private string aPrivate;

    [Button] public void TestA() => Debug.Log($"Test from {nameof(A)}");
    [Button] private void TestPrivate() => Debug.Log($"Test from {nameof(A)}");
}

// file: B.cs
public class B : A
{
    public string bPublic;
    [SerializeField] protected string bProtected;
    [SerializeField] private string bPrivate;

    [Button] public void TestB() => Debug.Log($"Test from {nameof(B)}");
    [Button] private void TestPrivate() => Debug.Log($"Test from {nameof(B)}");
}

// file: C.cs
public class C : B
{
    public string cPublic;
    [SerializeField] protected string cProtected;
    [SerializeField] private string cPrivate;

    [Button] public void TestC() => Debug.Log($"Test from {nameof(C)}");
    [Button] private void TestPrivate() => Debug.Log($"Test from {nameof(C)}");
}

Inspector of a C instance:

C Inspector


#15 seems to be caused by two different issues, as its still not fixed. The following script:

public class ExampleBehaviour : MonoBehaviour
{
    [SerializeField] private new Rigidbody rigidbody;
    [field: SerializeField] public Transform camera { get; private set; }
    public float light;
}

...still isn't displayed correctly in the inspector (compare to the example in the issue):

ExampleBehaviour Inspector

I'll update the issue, if the pull request is accepted.

Other solutions considered

  • Replacing Concat in the original implementation with Union, leaving the GetAll...IncludingBaseNonPublic recursive. This would cause the Union to be evaluated for each level of the hierarchy, resulting in redundant equality checks.
  • Using Type.Name alone as the equality check. This would affect types that hide inherited members or redeclare private members of base types, returning only the member closest to the current type. Same goes for methods - only the closest method to the current type would get a button when using ButtonAttribute. In the example above, only A.TestPrivate would be visible in the inspector of a C instance.

@AnnulusGames
Copy link
Owner

Sorry for the late confirmation. It seems to work fine, so I'll merge the PR. Thank you for your contribution!

@AnnulusGames AnnulusGames merged commit fcdf46a into AnnulusGames:main Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants