Various fixes #176

lgarczyn · 2025-08-28T16:36:10Z

You may want to pick and chose, this is just all of the edits we made to fit our usecase, and some general fixes

Use TypeCache for faster retrieval of generators
Added sort command to order all or a selection by name
Fixed reordering only moving one items one row at a time (turns out the filtered view was being reordered before the event)
Fixed Add returning false when the item had an invalid backref to collection
Added whitespace to popup to prevent exploding beyond the screen
Stopped renaming "Asset" to "Asset1" if SOC tries to move it to the same folder
Added support for multiple collections sharing one Items folder
Fixed MoveItem causing a stack overflow

# Conflicts: # Scripts/Editor/CustomEditors/CollectionCustomEditor.cs # Scripts/Editor/Generators/CollectionGenerators.cs # Scripts/Runtime/Core/ScriptableObjectCollection.cs

Fixed Add returning false when collection was indeed corrected

RoyTheunissen

Found some stuff that is probably worth fixing. Mostly style inconsistencies, but some functional things as well. Please have a look

RoyTheunissen · 2025-09-01T09:19:14Z

Scripts/Editor/Core/CodeGenerationUtility.cs


-            // Delete any existing files that have the old deprecated extension.
-            string deprecatedFileName = finalFileName + ExtensionOld;
-            if (File.Exists(deprecatedFileName))


Why did you remove this exactly? It is useful when migrating from an older version of SOC that it doesn't leave the old .cs file but automatically removes it, otherwise you get compile errors and you have to manually find where it generated the code and delete the old .cs file

Our collections are in the same folder as the generated code, so this code deletes them. Not sure how we ended up with a different structure than recommended, but we have a 100+ collections now.

This wasn't meant to be sent with the pull request, but since I start the pr with master, it got updated

RoyTheunissen · 2025-09-01T09:21:43Z

Scripts/Editor/CustomEditors/CollectionCustomEditor.cs

+
+    internal struct LimitedComparer : IComparer<ScriptableObject>
+    {
+        private readonly HashSet<ScriptableObjectCollectionItem> m_ItemsToBeSorted;


The m_ prefix is not consistent with the rest of the package. According to the style of this package this field should be called itemsToBeSorted. That goes for m_FirstItemIndex as well

RoyTheunissen · 2025-09-01T09:25:39Z

Scripts/Runtime/Core/CollectionsRegistry.cs

            {
-                ScriptableObjectCollection collection = Collections[i];
-                if (!collection.AutomaticallyLoaded)
+                if (!collection)


It's fine to omit braces for one line statements after an if condition. It's not done 100% of the time so I wouldn't say it's a rule, just know that it's okay to do so. In cases like this where it's just a continue, I'd say it's simpler / snappier.

RoyTheunissen · 2025-09-01T09:27:50Z

Scripts/Runtime/Core/ScriptableObjectCollection.cs

-            if (items.Contains(item))
+
+            bool contains = items.Contains(item);
+            bool set = socItem.Collection == this;


I don't find set a very clear name. How did you intend it? As "should set collection"?

it's more meant as "collectionAlreadySet" and "collectionAlreadyContains"

RoyTheunissen · 2025-09-01T09:29:53Z

Scripts/Runtime/Core/ScriptableObjectCollection.cs

            ObjectUtility.SetDirty(this);
        }

+        private string Desc(int index)


The method name Desc is not consistent with other method names / not that clear. Please write it out fully and with a verb, like GetDescription, which I think is what you intended here.

RoyTheunissen · 2025-09-01T09:31:09Z

Scripts/Runtime/Core/ScriptableObjectCollection.cs


+        private string Desc(int index)
+        {
+            var item = items[index];


Please use explicit types unless the type is very long and it's also very obvious what it is.

Scripts/Runtime/Core/ScriptableObjectCollection.cs

RoyTheunissen · 2025-09-01T09:42:02Z

Scripts/Runtime/Extensions/TypeExtensions.cs

+{
+    public static partial class TypeExtensions
+    {
+        private static bool IsList(this Type type)


This method is either not named correctly or does not function as intended, it seems to check if something is an ICollection<> but that just indicates that it's a collection, not that it's a list. For example, if you called IsList or an a Dictionary it would also return true.

You should either rename this to IsCollection or make it check for IList<> instead.

RoyTheunissen · 2025-09-01T09:43:07Z

Scripts/Runtime/Extensions/TypeExtensions.cs

+            {
+                if (baseType.IsGenericType &&
+                    baseType.GetGenericTypeDefinition() == typeof(CollectionItemIndirectReference<>))
+                    return baseType;


Multi-line conditions should have braces

Thundernerd · 2025-09-01T09:55:38Z

Scripts/Runtime/Core/CollectionsRegistry.cs

        public bool HasUniqueGUID(ISOCItem targetItem)
        {
-            for (int i = 0; i < collections.Count; i++)
+            foreach (ScriptableObjectCollection collection in collections)


Is there a particular reason you've changed these for loops to foreach loops?

They're faster and I find them more readable

Thundernerd · 2025-09-01T09:56:59Z

Scripts/Editor/CustomEditors/CollectionCustomEditor.cs

+            if (toSortX)
+            {
+                // compare y with the first item in the list to sort
+                return x.Index - m_FirstItemIndex;
+            }
+            if (toSortY)
+            {
+                // compare x with the first item in the list to sort
+                return m_FirstItemIndex - y.Index;
+            }


Are the comments in these if statements perhaps flipped?

Thundernerd · 2025-09-01T09:57:57Z

Scripts/Runtime/Utils/SOCItemUtility.cs

            Action onCompleteCallback = null)
        {
-            if (item is ISOCItem)
+            if (item is ISOCItem iItem)


I think a better name would be socItem instead of iItem

Thundernerd · 2025-09-01T09:59:25Z

Scripts/Runtime/Core/ScriptableObjectCollection.cs

                        changed = true;
                        ObjectUtility.SetDirty(itemsFromOtherCollection.Collection);
                    }
+                    }


Is the indentation correct here? I'm seeing two } in a row on the same indentation

Co-authored-by: Roy Theunissen <roy.theunissen@live.nl>

lgarczyn · 2025-09-01T12:17:44Z

To be honest, I didn't really intend for this PR to be merged. It just contains a bunch of fixes to issues that you may or may not want to include. I won't have time to fit it to the repo standard until at least next week.

The most important fix by far is the ReorderableList fix

Thundernerd · 2025-09-01T12:32:15Z

@lgarczyn

To be honest, I didn't really intend for this PR to be merged. It just contains a bunch of fixes to issues that you may or may not want to include. I won't have time to fit it to the repo standard until at least next week.

The most important fix by far is the ReorderableList fix

That's totally fair and you did sorta already mention this in the description!

lgarczyn · 2025-09-01T15:21:58Z

@lgarczyn

To be honest, I didn't really intend for this PR to be merged. It just contains a bunch of fixes to issues that you may or may not want to include. I won't have time to fit it to the repo standard until at least next week.

The most important fix by far is the ReorderableList fix

That's totally fair and you did sorta already mention this in the description!

Thank you, sorry for the messy code :)

lgarczyn added 23 commits August 27, 2025 20:58

Use TypeCache instead of GetAllAssignableClasses

da7789b

Don't crash if collection in CollectionRegistry is null

1d9f0b5

Fixed error when refreshing a graph where the last item was deleted

72089b8

Added sort command for collection editor

82b43cf

fixed collection move not updating collection parent

6d3d97e

Don't move file if target folder is same

c6257f2

Fixed whitespace

8608bf5

Allow multiple collection namespace

c372719

Merge remote-tracking branch 'origin/master'

f419fa8

# Conflicts: # Scripts/Editor/CustomEditors/CollectionCustomEditor.cs # Scripts/Editor/Generators/CollectionGenerators.cs # Scripts/Runtime/Core/ScriptableObjectCollection.cs

Fixed absurdly long error message going off screen

fe9c8ef

Fixed moving asset from "Path/Asset.asset" to "Path/Asset1.asset"

9b94b69

Try to make asset moving faster by using AssetDatabase.StartAssetEditing

c1aac39

Fixed stack overflow

c5a4df5

Fixed path compare

8da50d7

Don't fight with neighbor collections

4484f5c

Fixed exception

2869c2d

Improved log

e45787f

Fixed Add returning false when collection was indeed corrected

Fixed compare

31f569d

Removed log

003b01e

FUCKING FIXED LIST ORDERING WOW

4c83758

Removed autoclean of ExtensionOld

b106540

Full removed ExtensionOld

40df7c7

Added checks for accessors in CollectionsRegistry.cs

e1138eb

RoyTheunissen requested changes Sep 1, 2025

View reviewed changes

Thundernerd reviewed Sep 1, 2025

View reviewed changes

lgarczyn and others added 2 commits September 1, 2025 14:14

Update Scripts/Runtime/Core/ScriptableObjectCollection.cs

b43af7a

Co-authored-by: Roy Theunissen <roy.theunissen@live.nl>

Update Scripts/Runtime/Core/ScriptableObjectCollection.cs

07f002c

Co-authored-by: Roy Theunissen <roy.theunissen@live.nl>

Various fixes #176

Are you sure you want to change the base?

Various fixes #176

Uh oh!

Conversation

lgarczyn commented Aug 28, 2025

Uh oh!

RoyTheunissen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgarczyn commented Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Thundernerd commented Sep 1, 2025

Uh oh!

lgarczyn commented Sep 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lgarczyn commented Sep 1, 2025 •

edited

Loading