llealloo / audiolink

Audio reactive prefabs for VRChat
Other
364 stars 40 forks source link

Reduce network size for mini player and theme controller #315

Closed Happyrobot33 closed 3 weeks ago

Happyrobot33 commented 3 months ago

Hello! Just a simple PR to fix the bloat cost of arrays, specifically for the theme colors. Change is based on my calculator here: https://docs.google.com/spreadsheets/d/1WXrHxJQW_evivVpajzLnrZnmu0otWxVsBsfUZiQXLFg/edit?usp=sharing

The simple difference this makes is going from 136 bytes for the theme controller down to either 100 or 92 bytes depending on how the client decides to sort the variable names. Let me know if you have any questions! I can also simplify this to not use an array at all if you'd like, but this was the most drop in solution as it keeps the array accessing logic completely the same

Happyrobot33 commented 3 months ago

there was another optimization I wanted to make but it would've changed the access level of a variable I'm pretty sure, so I haven't committed that in here

float3 commented 3 months ago

what variable? these things are negotiable

Happyrobot33 commented 3 months ago

what variable? these things are negotiable

The ones I was looking at are _syncOwnerPlaying and _syncLocked in the mini player. They should be packed into a byte instead of being individual booleans. I could do this, although the one variable being nonserialized makes it more complicated than I think its worth now thinking about it

float3 commented 3 months ago

couldn't we keep them the way they currently are and just serialize and deserialize them that way when we send them over the wire?

Happyrobot33 commented 3 months ago

couldn't we keep them the way they currently are and just serialize and deserialize them that way when we send them over the wire?

well yes that would be the fundamental change. I can look into it deeper, it honestly might still be fine as a drop in property setup

Happyrobot33 commented 3 months ago

I have changed over to dedicated methods and obsoleted the array. I also went ahead and made the change to the mini player, but would like a more thorough testing of it as I'm not fully aware of the codebase and am unsure if I made the proper changes to fully support it.

Happyrobot33 commented 2 months ago

bump on review

Happyrobot33 commented 2 months ago

wow this is some spaghetti code in the name of optimization, lgtm but we need to wait for pema to check and we should probably test it

If you have any suggestions on what youd like to see cleaned up id be willing to try tp figure it out. Ive done some minimal testing but i dont use the features touched here extensively so yes more testing would be nice

pema99 commented 2 months ago

I'll take another look this weekend. Thanks for reminding

pema99 commented 2 months ago

Sorry, got sidetracked again. Looks fine overall but I had some additional comments