feat: disable facial features based on distance to player#860
feat: disable facial features based on distance to player#860AjimenezDCL merged 4 commits intomasterfrom
Conversation
|
After the CI passes: |
D4rWiNSS
left a comment
There was a problem hiding this comment.
LGTM with a small request! Not sure if each component should check to show the facial here or have a global controller. Anyway, this looks good!
| base.OnEnable(); | ||
| StartCoroutine(SetFacialFeaturesVisibleRoutine()); | ||
| } |
There was a problem hiding this comment.
Should we use the CourutineStarter here? Also, we should track the coroutine and clean it in the Cleanup method
There was a problem hiding this comment.
CoroutineStarter is a "hack" to have coroutines when you dont have Monobehaviours, which is not the case.
Tracking the coroutine is not really needed because they are aborted when the monobehaviour gets disabled (in this case when going back to the pool), but it doesn't hurt to make it explicit. Gonna do it!
There was a problem hiding this comment.
Cool! As far as I know, we were using CoroutineStarter (Besides de hack of course) because in some situations the coroutines will stay in memory even if the monobehaviour is destroyed. @BrianAmadori can you confirm on this?
There was a problem hiding this comment.
I created the CoroutineStarter and that was the purpose, to have coroutines non dependant of monobehaviours haha.
Coroutines are parts of the monobehaviours (that's the reason if you have a disabled one you cannot start a coroutine, because the update is never called) and I haven't been able to find anything related to leaks in coroutines.
In fact using the CoroutineStarter can actually lead to some leaks if the coroutine it's never released and the handler gets destroyed.
BenchmarkMovingStill |
Fixes #826
How to test
It's easier to test this in Unity. Just lock the editor window to an avatar and either move the avatar away or move yourself away to see how the mouth, eyes and eyebros dissapear.
Test coverage
All of our AvatarShape tests are disabled (it takes too long to load an Avatar) and there's an ongoing effort to first refactor AvatarShape/AvatarRenderer and then implement a proper test suite. Therefore I decided to not waste time on making a test that is going to be disabled.
Implementation details
Instead of checking each time the player or the other avatars change position, I decided to have a routine running and performing the distance calculation.
I was worried of triggering a Distance calculation (expensive) each time (possibly multiple times).