guycalledfrank / bakery-issues

Bug tracker for Bakery
4 stars 0 forks source link

Some performance improvements for the editor #125

Closed pedropla closed 2 years ago

pedropla commented 2 years ago

On a scene with thousands of point lights with the bakery script the editor can go to around 10fps performance when viewing the scene with gizmos.

image

Perhaps these can be in an OnDrawGizmosSelected instead?

In play mode on the same scene the BakeryPointLight.Update is also called thousands of times and even though nothing is run because of the if (EditorApplication.isPlayingOrWillChangePlaymode) return; it still has a noticeable impact on performance.

This is what is in the update function: void Update() { if (EditorApplication.isPlayingOrWillChangePlaymode) return; if (!ftUniqueIDRegistry.Mapping.ContainsKey(UID)) ftUniqueIDRegistry.Register(UID, gameObject.GetInstanceID()); if (gameObject.GetInstanceID() != ftUniqueIDRegistry.GetInstanceId(UID)) { UID = Guid.NewGuid().GetHashCode(); ftUniqueIDRegistry.Register(UID, gameObject.GetInstanceID()); } }

Wouldn't it be better to calculate the UID when performing a render rather than once per frame? On small scenes it isn't a problem, but as the scene grows in size with more lights the performance suffers significantly.

guycalledfrank commented 2 years ago

When having thousands of anything, Unity tends to be slow. I remember I once tried to add a special marking component on every mesh renderer in the scene, and even though it had no Start and Update, simply the existence of thousands of MonoBehaviours in the scene made it slow at runtime. That's just Unity. You can remove all Bakery components before building though.

I can't move the tiny sphere gizmo drawing code to OnDrawGizmosSelected, because the gizmos are used TO select lights.

But note that if you "minimize" the component by clicking on its name, it will prevent OnDrawGizmos from being called on all objects with this type of component. I do it when I want to see the image without gizmos:

image

And looking at the profiler, it seems to be gone in this case.


Update() situation: yeah, looks bad. Costs 0.5 ms with 2k lights in my test, that's a lot for something that does nothing.

Why was I even using the Update for it? My memory is hazy here. Apparently sometimes Start() wasn't called for some reason? The whole UID system was meant to provide reliable IDs that don't change from session to session, unlike GetInstanceID; so it mapped local session instanceID to persistent UID. But since UID is already stored on the component, why is it even necessary to have the instanceID connection? Actually there is a reason: because copy-pasting the component on another object will result in the same ID, and that shouldn't happen.

So UID must be both consistent across different bakes and also unique for a particular instance.

One thing I've just noticed is that OnDestroy is not called when the object is "destroyed" via Ctrl+Z. Possibly Update was just a quick workaround for all kinds of "something not called" like this.

Hmmm "won't fix": https://issuetracker.unity3d.com/issues/ondestroy-is-not-called-while-undo-is-performed-second-time Okay.

So I see the problem: if we simply move Update's code to the bake time, there won't be any way to tell which of the copy-pasted lights is the original holder of its UID, and which lights were created later and should generate new UIDs.

Anyway, this whole mess seems like an overcomplication; instead I can just store an array of gameobjects with array index being UID. This way copy-pasting invalidates the copy automatically (not added to the array), it's persistent, etc.

Done: https://github.com/guycalledfrank/bakery-csharp/commit/99870e0b1a752a4ca44c5391f0175aa30804ff33

Please try patching via the patcher/github

pedropla commented 2 years ago

Thanks!