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

Added inspector multi-editing #50

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Conversation

D4KU
Copy link

@D4KU D4KU commented Jan 2, 2022

I extended multi-selection support over to the inspector. InspectorField's Value property is now an IEnumerable and got renamed to BoundValues, meaning that one drawer can now bind to multiple values.

Since some breaking changes were unavoidable, I used the opportunity to introduce some static type safety, which avoids many casts in sub-classes and made the public API clearer. Some concepts of multi-editing can get hard to communicate when everything is an object. The way it works is that InspectorField got a type parameter, which specifies the type with which bound values are stored. It still has a non-generic base class, which can't access the BoundValues. For this, I introduced two interfaces: IBound and ISupportsType. See their declaration for more information.

@yasirkula
Copy link
Owner

Thank you for this PR! After my current to-do list is finished (which will take some time, unfortunately), I'll take a good look at your commits, I'm already looking forward to it 😄

@yasirkula
Copy link
Owner

I didn't review all files yet but from what I saw, there'll be considerable amount of changes needed before I merge this PR. The major points are:

  • LINQ usage: I don't use LINQ in any of my plugins. I've heard people using this plugin on mobile and VR projects and I really don't want LINQ's GC allocation and performance issues
  • Assuming LINQ usages are replaced, IEnumerable references can also be changed with arrays. RuntimeInspector's Inspect function is seldomly called and since iterating over arrays will be faster than IEnumerable (which will happen multiple times per second for dozens of InspectorFields), we can force the user to explicitly pass an array to the Inspect functions
  • I've updated the contributing guide for pull requests. To summarize:
    • I'm still planning to support Unity 5.6 and onwards. For this reason, some useful C# features like local functions or inline variable declarations (e.g. obj is X x or out X x) can't be used
    • Usages of types that aren't available in 5.6 (e.g. RectInt) must be wrapped inside #if UNITY_X_OR_NEWER #endif directives
    • I've seen a few lines where Tabs were replaced with spaces. You can use the buttons at the bottom right corner of VS to fix these cases (use Tabs and CRLF)
  • Can the new system handle the testField below correctly? Are changes to that field in the RuntimeInspector correctly applied to the TestClass?
public class TestClass : MonoBehaviour
{
	[System.Serializable]
	public struct TestStruct
	{
		public int testField;
	}
	
	[System.Serializable]
	public struct TestStruct2
	{
		public TestStruct testStruct;
	}
	
	public TestStruct2 testStruct2;
}

After these changes, I may still ask for some medium-scale changes (though I don't think they would be as significant as these), so I'd understand it if you're unwilling to do these changes. Thank you for your effort regardless!

@D4KU
Copy link
Author

D4KU commented Jan 29, 2022

Don't worry, I was expecting this to take some iterations, I'm still willing to follow through with this PR. I already started working on your remarks and will update you soon. :)

@D4KU
Copy link
Author

D4KU commented Jan 31, 2022

Alright, the new commits:

  • Remove Linq
  • Solve the broken inspection of your TestClass
  • Use tabs everywhere
  • Make the plugin run in Unity 2017 again

However, there is problem in regards to Unity 5.6 I wanted to discuss.

I chose IEnumerable (and now IReadOnlyList) to ensure that code wanting to change the BoundValues property (formerly Value) of an InspectorField can only set the whole list instead of adding, removing, or changing entries. The getter of BoundValues is public, so exposing ways to manipulate its entries without the owning InspectorField receiving any callback would be a design flaw in my opinion.

The problem: in Unity 5.6, with .NET 3.5 as only option, ReadOnlyCollection<T> is the only index-able, read-only list class or interface available. There is no non-generic equivalent. Generic Variance, which my system relies on, only works on interfaces. The only way I see to make it work with 5.6 then is to rip out my generics again and use ReadOnlyCollection<object> everywhere, thus reintroducing all the ugly casting and loosing the ability the communicate that the list is only meant the store apples; not fruits, cars, or anything else.

Having said that, I would prefer to drop support for .NET 3.5. I like that the plugin is that much backwards compatible, but here I think the benefits for the code base of ditching 5.6 outweigh the benefits of keeping it. What do you think?

@yasirkula
Copy link
Owner

I'm sorry for replying late. Thank you for your new commits 😺 I was actually ready to convert the prefabs to 5.6 myself, sorry that I didn't mention it in my previous post.

I didn't check out all changes yet but could you share where you're using the non-generic equivalent of ReadOnlyCollection<T>?

@D4KU
Copy link
Author

D4KU commented Feb 6, 2022

Oh sorry you must have misunderstood something, to my knowledge there is no non-generic ReadOnlyCollection. I'm using IReadOnlyList<T> now where I used IEnumerable<T> before. For example, as type for InspectorField<T>.BoundValues.

@yasirkula
Copy link
Owner

I'm trying to understand which part of the code is currently not Unity 5.6 compatible.

@D4KU
Copy link
Author

D4KU commented Feb 8, 2022

While typing my response I actually figured out a way to make it work in 5.6 again. :)

@yasirkula
Copy link
Owner

I'm aware of my late response times. Stuff has been happening and I wasn't in the mood to review a large PR for quite some time. There'll still be a delay before I review this PR but know that this is one of the few things in my small to-do list.

@D4KU
Copy link
Author

D4KU commented Feb 15, 2022

No stress, take your time. I'm not dependent on the changes being upstream.

@D4KU
Copy link
Author

D4KU commented Mar 20, 2022

I worked with my multi-edit system for several weeks now and found a few improvements to my code that I just included in this PR, in case you're wondering where the latest commits come from.

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.

2 participants