Conversation
Streus
added
enhancement
| } | ||
| #endregion | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
newline at end of file
| EditorGUILayout.EndFadeGroup(); | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
newline at end of file
| } | ||
|
|
||
| GUILayout.Space(25); | ||
| if (GUILayout.Button(fadeGroupVal.value ? "Hide contents" : "Show contents")) |
There was a problem hiding this comment.
can we have a rule for always using braces? /nitpick
| public string TargetDirectory { get; private set; } | ||
|
|
||
| private Dictionary<string, ushort> nameToFAID; | ||
| private FeatureAsset[] features; |
There was a problem hiding this comment.
(for my own understanding) In short:
Adding a FeatureAsset to the Library essentially adds it to the end of an array, with an additional search structure which indexes by name (that is specified on add). Accessing a FA by index is trivial, accessing by name has a lookup in the name->index Dictionary.
There was a problem hiding this comment.
Additional note now that I'm looking at this more: nameToFAID.Values() will only contain sequential integers. I think you can just replace this with an array of strings of the same size as features, since their order will be equivalent (purely for storage). The Dictionary is necessary because of the need for a reverse lookup from names->assets.
| if (name == null || feature == null) | ||
| return false; | ||
|
|
||
| features[nameToFAID.Count] = feature; |
There was a problem hiding this comment.
features is an array, and it's only ever initialized to size 0. I don't see any lines where this array expands in size, so I think any time you call Add, you'll get an array index out of bounds exception.
There was a problem hiding this comment.
On further inspection, the only way this array gets created otherwise is by [de]serialization, so this is okay. This does limit the FeatureLibrary to being editor-only, and it can't be easily modified via code.
|
|
||
| public bool Contains(string name) => nameToFAID.ContainsKey(name); | ||
|
|
||
| public bool Contains(ushort faid) => 0 <= faid && faid < features.Length; |
There was a problem hiding this comment.
This zero is very important
=> 0 <=
| { | ||
| string str = ""; | ||
|
|
||
| foreach(Entry e in this) |
There was a problem hiding this comment.
i'm guessing this is implicitly calling IEnumerable.GetEnumerator()- neat syntax
| [SerializeField] | ||
| private List<string> serializedNames; | ||
| [SerializeField] | ||
| private List<FeatureAsset> serializedFeatures; |
There was a problem hiding this comment.
if i remember right, only the Dictionary has trouble serializing. the FeatureAsset[] features should be okay, so based on that, serializedFeatures might be unnecessary. If so, it can be replaced with just the list of strings in the same order as the FeatureAsset array.
rushingseas8
left a comment
rushingseas8
left a comment
There was a problem hiding this comment.
Generally looks good to me. Working with it in the Unity editor felt pretty okay.
2 participants
Starting this PR early 'cause core functionality is done, just need some last polish, so I figured I'd do that with PR comments, if there are any.
Create a FeatureLibrary via
Assets/Create/Labrys/Feature Libraryin the menu bar, or in the project view context menu. A prompt will appear asking for a target directory. This is the directory the library will scrape feature assets from.Auto refresh stuff might not happen. There aren't a lot of options for asset creation hooks.