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

[Design Choice Discussion] Instancers and Garbage Collection #370

Open
soraphis opened this issue Oct 19, 2022 · 6 comments
Open

[Design Choice Discussion] Instancers and Garbage Collection #370

soraphis opened this issue Oct 19, 2022 · 6 comments
Labels
community question Question from the maintainers to the community question Further information is requested

Comments

@soraphis
Copy link
Collaborator

https://discord.com/channels/640589221136171020/643165315546742785/1000332183057203210

Scriptable Objects (even the in-scene in-memory runtime copies) are not free'd if there is no reference pointing to it like normal objects (no garbage collection).

Freeing them requires to call Resources.UnloadUnusedAssets which is a kinda performance heavy call.

Also it is not really possible to Destroy the atom-instances in the instancers OnDisable since there could be remaining references (if destroy works at all for them).

When fiddling around with my own SOArchitecture approach my first implementation of Instancers wouldn't create SOs, but simple classes. at runtime there is no real need for an SO. Not sure if this would be suitable for Atoms without an rework, though.

@soraphis soraphis added community question Question from the maintainers to the community question Further information is requested labels Oct 19, 2022
@toasterhead-master
Copy link
Contributor

toasterhead-master commented Apr 27, 2023

I have implemented a solution to that. I'm still on the process of testing it with @Bumpty (from discord), there are some bugs still to be found and fixed. But so far, InfinityWaves works just fine with them, I only had to change 1 single script for the project to compile with my modified UnityAtoms package (And everything works the exact same). That's because I made sure to make it as backwards compatible as possible. Instead of creating a class like you suggested, I just made the instancer to behave exactly like an Atom would. Also, instead of instantiating a ScriptableObject for events (yeah, instancers are not the only source of SO instantiation) I made them copy their inner values instead in case a copy is needed. So that makes it that there's 0 SO instantiations on runtime!

@toasterhead-master
Copy link
Contributor

toasterhead-master commented Apr 27, 2023

And yeah, that means I copy pasted a lot of code. But my hands were tied and introducing a new class to be created for each instancer won't make the code much cleaner since SO and a separate class cannot inherit the same class. And Interface default method implementations were introduced only recently with newer Unity versions, and there are still lots of c# features missing in Unity. So yeah...

@soraphis
Copy link
Collaborator Author

Instead of creating a class like you suggested, I just made the instancer to behave exactly like an Atom would.

This has a different side effect, though.

GO1 is spawned from a prefab, and the instancer clones SO1 -> SO1_1. GO1 has a component that put's a reference of SO1_1 into a list of some manager class. GO1 is destroyed.

Currently, the reference to SO1_1 would just exist further and could be used within that list. If instead of SO1_1 the instancer is put into the list, the reference would be destroyed.

I'm not saying that we must support that scenario, just saying this is one case where backwards compatibility would be broken, whereas creating a POCO and letting the GC handle the cleanup would handle this scenario.

@toasterhead-master
Copy link
Contributor

toasterhead-master commented Apr 27, 2023

Well, in such case I guess having an additional class wouldn't hurt. Fortunately, I've implemented a unit testing infrastructure for UnityAtoms, so it will be easy to test it (I used the almighty powers of Reflection to make unit testing easy with unity atoms).

@toasterhead-master
Copy link
Contributor

toasterhead-master commented Apr 28, 2023

Actually, the best way to do that would be to only have 1 GenericAtomVariable class (Since the AtomVariable name is already taken, I added that prefix) for both Instancers and SO. This eliminates the repeated code, although the methods are still going to be implemented for the SO just calling for the methods implemented inside the GenericAtomVariable class. But now you see the problem with this approach that needs to be solved. How are we gonna view and edit the settings through the inspector? Because Unity doesn't support generics by default (I don't know how to support it, and even then backward compatibility would be ruined as a result). So SerializedField won't simply work. That's why I did the instancers as I did them, but forgot why I decided to take this approach. Is this solvable in any way? Or either way a sacrifice must be made?
And you might say: "Well, you probably don't need to check the runtime Atom the Instancer made, you know it has to be a copy of the _base attribute you set, and that's all you need to know". But it is, I would argue, a debuggability issue.

@soraphis
Copy link
Collaborator Author

I personally would be totally fine with having the instancers just behave like the atom instance. Like you did. Especially if it's the easier approach.

For Ions I went full interfaces. I have PureIons (which are just classes), IonComponents (which are MonoBehaviours) and IonSOs (ScriptableObject) which all implement the same interfaces.

a IonReference is basically just a fancy InterfaceHelper that allows putting anything that implements that interface.

Unity 2022+ definitely supports generics, to a pretty far extend just not for SerializedReferences and Assets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community question Question from the maintainers to the community question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants