chore: optimize avatar names by AjimenezDCL · Pull Request #833 · decentraland/unity-renderer

AjimenezDCL · 2021-07-22T09:37:35Z

Fixes #825

To optimize the Avatar Names we are going to:

User the same canvas for each name
Sort the pieces so we can batch the draws of the containers, the text and the voice chat icon.

Approach:

Create a new HUD called AvatarNames.
Use the DataStore to keep a PlayerStatus collection.
Minimap refactoring to use the new PlayerStatus collection.
AvatarNameHUD implementation.

It must be tested with this Kernel:
decentraland/explorer#2503

Test Link: https://play.decentraland.zone/branch/chore/avatar-names-optimization/index.html?renderer=urn:decentraland:off-chain:renderer-artifacts:chore/optimize-avatar-names

github-actions · 2021-07-22T09:38:01Z

After the CI passes:

This branch can be previewed at https://play.decentraland.zone/index.html?renderer=urn:decentraland:off-chain:renderer-artifacts:chore/optimize-avatar-names
Code coverage report: https://renderer-artifacts.decentraland.org/branch-coverage/chore/optimize-avatar-names

AjimenezDCL · 2021-07-22T10:04:43Z

Measurements

Still WIP. Benchmark in Unity Editor

Standing Still

Before Optimization
PerformanceMeter - Data report step 2 - Processed values:
 * PERFORMANCE SCORE (0-100) -> 74,49
 * average FPS -> 29,59333
 * highest FPS -> 33
 * lowest FPS -> 25
 * 50 percentile (median) FPS -> 30
 * 95 percentile FPS -> 33
 * total hiccups (>0,05ms frames) -> 3 (0,33% of frames were hiccups)
 * total hiccups time (seconds) -> 0,2143326
 * total frames -> 900
 * total frames time (seconds) -> 30,00287

After Optimization
PerformanceMeter - Data report step 2 - Processed values:
 * PERFORMANCE SCORE (0-100) -> 75,92
 * average FPS -> 31,42437
 * highest FPS -> 36
 * lowest FPS -> 26
 * 50 percentile (median) FPS -> 32
 * 95 percentile FPS -> 35
 * total hiccups (>0,05ms frames) -> 5 (0,53% of frames were hiccups)
 * total hiccups time (seconds) -> 0,3589172
 * total frames -> 952
 * total frames time (seconds) -> 30,00278

Moving around

Before Optimization
PerformanceMeter - Data report step 2 - Processed values:
 * PERFORMANCE SCORE (0-100) -> 73,21
 * average FPS -> 28,47691
 * highest FPS -> 33
 * lowest FPS -> 22
 * 50 percentile (median) FPS -> 29
 * 95 percentile FPS -> 32
 * total hiccups (>0,05ms frames) -> 9 (1,04% of frames were hiccups)
 * total hiccups time (seconds) -> 0,5629196
 * total frames -> 866
 * total frames time (seconds) -> 30,0013

After Optimization
PerformanceMeter - Data report step 2 - Processed values:
 * PERFORMANCE SCORE (0-100) -> 74,98
 * average FPS -> 30,23965
 * highest FPS -> 35
 * lowest FPS -> 25
 * 50 percentile (median) FPS -> 30
 * 95 percentile FPS -> 35
 * total hiccups (>0,05ms frames) -> 4 (0,44% of frames were hiccups)
 * total hiccups time (seconds) -> 0,2732468
 * total frames -> 918
 * total frames time (seconds) -> 30,0129

AjimenezDCL · 2021-07-23T09:43:50Z

Measurements

Benchmark in Build

Before Optimization
PerformanceMeter - Data report step 2 - Processed values:
 * PERFORMANCE SCORE (0-100) -> 26.64
 * average FPS -> 18.37345
 * highest FPS -> 19
 * lowest FPS -> 11
 * 50 percentile (median) FPS -> 19
 * 95 percentile FPS -> 19
 * total hiccups (>0.05ms frames) -> 437 (77.35% of frames were hiccups)
 * total hiccups time (seconds) -> 23.67953
 * total frames -> 565
 * total frames time (seconds) -> 30.0313

After Optimization
PerformanceMeter - Data report step 2 - Processed values:
 * PERFORMANCE SCORE (0-100) -> 39.05
 * average FPS -> 19.19595
 * highest FPS -> 20
 * lowest FPS -> 11
 * 50 percentile (median) FPS -> 19
 * 95 percentile FPS -> 20
 * total hiccups (>0.05ms frames) -> 319 (53.89% of frames were hiccups)
 * total hiccups time (seconds) -> 16.94389
 * total frames -> 592
 * total frames time (seconds) -> 30.02672

Remove old AvatarName implementation

D4rWiNSS

LGTM! Some suggestions were added.

I think we should somehow ( maybe each X seconds ) change the names of the avatars when we are moving and we are above the max numbers of avatar names.

Imagine that you are in a concert and we are moving on to where our friends are. So we are moving from the entrance to the other part of the concert. You will want to see the avatar names of the nears players and not the first 200 avatars names.

Take this into account for the future!

D4rWiNSS · 2021-07-26T10:29:31Z

+
+        private void OnEnable()
+        {
+            canvasRect.GetComponent<Canvas>().sortingOrder = -2;


We should move this to somewhere common to the project like Settings, So in the future, if we need to change the sorting order in other parts of the project we have some visibility about the current canvas ordering. What do you think?

Not sure about that, this is an specific sorting set for the AvatarNames. I dont think any other part of the project should be aware of what sorting order does the AvatarNames canvas have.

As you wish!

D4rWiNSS · 2021-07-26T10:33:51Z

+
+        public void DestroyUIElements()
+        {
+            Object.Destroy(background.gameObject);


Shouldn't we use GameObject.Destroy instead of Object in unity objects?

Destroy is a member of Object. GameObject has it because inherits Object.

Yes, but Unity uses an implementation where he passes the time to be destroyed, not sure if it makes any change though

I mean, GameObject doesn't override Destroy. It's the same call

D4rWiNSS · 2021-07-26T10:35:21Z

+        protected override IEnumerator TearDown()
+        {
+            if (hudView != null)
+                Object.Destroy(hudView.gameObject);


Same here, GameObject instead of Object?

sandrade-dcl

Reviewed and tested, LGTM! I only left some suggestions/doubts. Good work.

sandrade-dcl · 2021-07-26T10:25:32Z

-        private MinimapMetadata.MinimapUserInfo avatarUserInfo = new MinimapMetadata.MinimapUserInfo();
        bool initializedPosition = false;

+        private PlayerStatus playerStatus = null;


In order to avoid confussions, maybe a better name for this playerStatus would be avatarStatus? As this data is managed by any avatar in the world and not only by our avatar... What do you think?

It contains information about the player, such as name and the voice chat status along the position. I think it would be more confussing to have it named avatarStatus.

sandrade-dcl · 2021-07-26T10:28:00Z

+            if (playerStatus != null)
+                playerStatus.worldPosition = entity.gameObject.transform.position;


Couldn't we do this position update in the OnEntityTransformChanged() instead of do it in each frame?

The entity transform position is not where the Avatar is rendered, we interpolate the target position to make it smoothly. We actually have to do it each frame or the AvatarName will be lagging behind. A nice "side-effect" is that the minimap is now smoother as well (since it can use this data).

AjimenezDCL · 2021-07-26T11:17:36Z

LGTM! Some suggestions were added.

I think we should somehow ( maybe each X seconds ) change the names of the avatars when we are moving and we are above the max numbers of avatar names.

Imagine that you are in a concert and we are moving on to where our friends are. So we are moving from the entrance to the other part of the concert. You will want to see the avatar names of the nears players and not the first 200 avatars names.

Take this into account for the future!

@Suduck Yeah I was thinking the same but as I started benchmarking I noticed we can drastically increase the number of Avatar Names in screen (thanks the the batching of the UI parts). It's a nice suggestion but it wouldn't have much use until we can comfortably have more than 200 users on screen.

AjimenezDCL added 2 commits July 21, 2021 15:55

Now minimap uses the PlayerStatus collection from the DataStore

41e9b87

Avatar names WIP 2

2952e90

AjimenezDCL self-assigned this Jul 22, 2021

Remove log

7c7fb0a

Added voice chat icon to AvatarNames

d4a5cf8

AjimenezDCL mentioned this pull request Jul 22, 2021

chore: add new AvatarNamesHUD decentraland/explorer#2503

Merged

AjimenezDCL added the wip label Jul 22, 2021

AjimenezDCL added 2 commits July 23, 2021 10:14

Merge branch 'master' into chore/optimize-avatar-names

c9bf2b0

AvatarNames working!

f3b87df

AjimenezDCL added 5 commits July 23, 2021 11:56

Bug in AvatarShape reporting positiong

45ef325

Fix tests

1b52de0

Test suite

fe12797

Trying to fix voice chat

679234e

Misspeled test assertion

30647bd

BrianAmadori reviewed Jul 23, 2021

View reviewed changes

AjimenezDCL and others added 7 commits July 23, 2021 17:08

Player status reset when cleaning up AvatarShape

fa6e115

AvatarShape now reacts to changes in player names

3215bd6

Disable AvatarNames components if out of bounds

4634132

Cache Camera.main call

4c9b317

Now avatar names are not drawn on top of the scenes UI

aa3874b

Enable visual tests

ada9319

Remove old AvatarName implementation

Merge branch 'master' into chore/optimize-avatar-names

83dd825

AjimenezDCL added ready for review and removed wip labels Jul 26, 2021

D4rWiNSS approved these changes Jul 26, 2021

View reviewed changes

sandrade-dcl approved these changes Jul 26, 2021

View reviewed changes

Some code review

6cf16b0

AjimenezDCL merged commit c3050fd into master Jul 26, 2021

AjimenezDCL deleted the chore/optimize-avatar-names branch July 26, 2021 14:33

		if (playerStatus != null)
		playerStatus.worldPosition = entity.gameObject.transform.position;

Conversation

AjimenezDCL commented Jul 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 22, 2021

Uh oh!

AjimenezDCL commented Jul 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Measurements

Standing Still

Moving around

Uh oh!

AjimenezDCL commented Jul 23, 2021

Measurements

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

D4rWiNSS left a comment

Choose a reason for hiding this comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AjimenezDCL Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

sandrade-dcl 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!

AjimenezDCL commented Jul 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AjimenezDCL commented Jul 22, 2021 •

edited

Loading

AjimenezDCL commented Jul 22, 2021 •

edited

Loading

AjimenezDCL Jul 26, 2021 •

edited

Loading