Atom drawer rewrite #341
Atom drawer rewrite #341
Conversation
| @@ -0,0 +1,6 @@ | |||
| { | |||
There was a problem hiding this comment.
this file should not be part of the PR
| var indent = EditorGUI.indentLevel; | ||
| EditorGUI.indentLevel = 0; | ||
|
|
||
| property.objectReferenceValue = EditorGUI.ObjectField(position, property.objectReferenceValue, objectFieldSubType.SubType, false); |
There was a problem hiding this comment.
I'm not totally sold on ObjectFieldSubType. Is this really necessary?
What does it mean for the users? are there cases where they'd need to use this attribute? If so, wouldn't it be better for the developer experience if we'd use reflections to determine this type? (or if it is always AtomBaseVariable there is no need anyway)
There was a problem hiding this comment.
I think we might be better to use this as an internal part for the framework. For now I didn't know the best way to make this attribute internal. The AtomDrawer uses the Typecache API to determine th right Type for in the inspector. However, for AtomList and AtomCollection the defaultt type that is found for the elements in the list are AtomBaseVariableList type. This is the incorrect type because it should be AtomBaseVariable. The attribute overwrites the inspector list to show the correct type to the user. I first thought of a general option to do it in the AtomDrawer class, but i couldn't think of a clean solution so far.
these two PRs where already attribute to the V5 milestone.
I - personally - would've whished for those things being their own PR this would've made reasoning about them easier. Also the title of this PR is a bit confusing to me, what exactly is the feature this PR want's to introduce? a rewrite? Why is a rewrite a feature? Also: https://github.com/unity-atoms/unity-atoms/blob/master/CONTRIBUTING.md
Now where in this awkward situation to discuss within a single PR about multiple different things, and what's the best thing to do for each case... 1. [HideCreate] Attributeis the least controversial for me. I think it's fine and fast for internal usages. But I hope that common use cases for developers using Atoms don't require them to make overly use of this attribute. 2. [ObjectFieldSubType] AttributeAs I've commented on the file:
I have not yet looked super close into the issue, but I personally would've liked an Issue for this first, explaining the where the problem comes from and discussion possible solutions. 3. New Variables and Events are automatically fused as a subassetfor this one a PR already exists: #316 which should just be changed to target the v5 branch. 4. New Variables and Events that are created in the inspector get a default pre configured name.for this one a PR already exists: #317 which should just be changed to target the v5 branch. 5. This PR should be a fix for #67how? from skimming over the code, I couldn't see it. you may elaborate this. .... I could go on but don't have the time for this yet |
|
The [HideCreate] attribute is there to show the same inspectors as V4. So theres no create button in for example AtomList items. To get the same structure as V4 this was the only wayi could think of, because of the way generics are used. For issue #67 the fix is in AtomDrawer lines 221 - 233. It generates a default name users can accept for a newly created Atom Event/Variable. My apologies for the clutter in the PR. |
This PR is the V5 upgrade of my earlier uploaded PR's. I did a rewrite of the AtomDrawer Class and added a few things.
New Variables and Events that are created in the inspector get a default pre configured name.
New Variables and Events are automatically fused as a subasset. The UI for this is implemented too.
Implemented Typecache API to convert ObjectFields back into their correct type as they work in V4.
Added a [HideCreate] propertyAttribute to disable the create button for certain fields in the inspector (like Items in the AtomList/AtomCollection and the Submachine ObjectField in the FSM state).
Added a ObjectFieldSubType property attribute to convert the inspector objectfields into the right type. In e.g. AtomList and AtomCollection the items in the list and collection were of type AtomBaseVariableList. They should be AtomBaseVariable. This property attribute makes sure of that.
All feedback is appreciated. This version now should already work quite good.
This PR should be a fix for #290 , #67 , #149 and @AdamRamberg 's concerns about the Unity Editor search functionality for subclasses https://forum.unity.com/threads/generics-serialization.746300/page-2#post-5858386
Questions:
Fusing multiple Boolvariables on the FSM works. But the code now has some quirks doing this. I'd love some feedbakc to improve this even more or this PR in general :).
Do we want the Changed and ChangedWithHistory variables as properties or as it is now by using public? In V5 I saw it was changed to public but I don't if this was on pupose or not.