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

Performance settings #2351

Closed
wants to merge 14 commits into from
Closed

Performance settings #2351

wants to merge 14 commits into from

Conversation

0reo
Copy link
Contributor

@0reo 0reo commented Feb 10, 2022

Here's what I have so far for the avatar quality stuff. Ran into some issues with new lines when committing that I need to finish cleaning up still via a rebase, in particular on the app-manager file. Will update this with more info and questions shortly.

--

As I was learning the system, and using the existing functions to change the quality, my initial approach was to set a load property on the avatar, and to keep it from rendering until the load state was set to true. I attempted to have the quality settings be applied in the constructor of the Avatar class, but found that the sprite creator made it's own avatar instance, and was causing an infinite loop. so i moved setting the initial quality settings to just after creating the avatar, and the would enable the load property once the quality was set to start rendering. Looking at the setQuality function in avatars.js, I saw the current approach was to compress the existing model, attach the new model, and then hide the old one. But it wasn't keeping track of the sprite models, it just kept adding new ones, so I updated it to track the initial model, crunched model, and sprite, and only create new ones if there wern't any existing already.

I soon saw that the VRM templates had an activate callback which would set up an avatar for the local player. The app is manually calling setAvatarUrl, I felt it made sense to be able to utilise the activation callback and have the ability for the VRM template to assign itself as an avatar. And see as the quality settings were to be applied to all VRMs, not just the player's avatar, I decided to shift the initial quality logic into the template. I still need to add the cruncher into it though. I then modified the scene template to identify if it was loading a VRM or not, and if the model being loaded was marked as a player by adding an extra property to a model in the scene manifest. If the scene gets a VRM that's not the player, it make that VRM into an NPC. I then updated the setAvatarQuality function in game.js to account for the NPC's as well.

I also added watch to the app project to have it auto restart if I modified the templates. It's not a hot reload but it's less annoying than having to manually restart it.

Some questions:

  • making the VRM's into avatars discards their initial set position. I can change that, but wanted to make sure I'm going in the ideal direction before making more changes to to the avatar/NPC logic.
  • the VRM templates were running the toon shader on the models twice, once in default for unskinned, and one in the setSkinning function, and so it's currently doing that for spriting as well. I assume it should only need to be run once per VRM and reused?
  • applying the quality changes to all the avatars takes quite a bit of time, it can surely be sped up, but I didn't want to go too far off task adding a loading screen. Is the expectation that all the down-scaling will be done on the client side on load? Any plans to store/reuse models online/locally?

I'm not 100% on the "whys" of everything yet obviously. Getting more insight into how it currently works vs how you're intending it to work will help a lot. Looking forward to discussing in more detail.

resolve #2321

@0reo 0reo force-pushed the performance-settings branch 3 times, most recently from c0b63d9 to c3a26ab Compare February 11, 2022 00:34
@0reo
Copy link
Contributor Author

0reo commented Feb 12, 2022

moved access of quality settings for readability/ease of access. also enabled the unimplemented quality settings, and have a reasonable default being used if one has not yet been set. the crunched avatar seems visually lighter than I expected, but the materials look correct when looking through the object. would be good to get confirmation that it's correct.

@alisaad673
Copy link
Contributor

Is this in testable for? Should I test it?

@0reo
Copy link
Contributor Author

0reo commented Feb 12, 2022

@amnaarshed4224 this is to resolve #2321, it's still a work in progress but you should be able to test changing the quality settings. the following still needs to be address

  • turning a vrm into an npc resets it's position
  • diorama bug fixes
  • crunched avatar for diorama & post processing
  • possibly having the crunched avatar set in the vrm template like i did with the sprite mesh, would be good to hear opinions on that
  • possibly adjusting skinning/unskinning logic to make initial loading faster

also note this has a totum change. the .gitmodules file is currently updated to point to my forked repo.
0reo/totum@0e74f13

@avaer
Copy link
Contributor

avaer commented Feb 12, 2022

the crunched avatar seems visually lighter than I expected, but the materials look correct when looking through the object. would be good to get confirmation that it's correct.

That is correct behavior; almost by definition. The shader is much simpler than the MToon which is used at the highest quality setting. Additionally, the cruncher uses one material for the entire avatar which is usually not the case with the MToon shaded avas.

@avaer
Copy link
Contributor

avaer commented Feb 14, 2022

making the VRM's into avatars discards their initial set position. I can change that, but wanted to make sure I'm going in the ideal direction before making more changes to to the avatar/NPC logic.

I think it should be possible to make it work so that when you go into a VRM you "embody" it, while "disembodying" the previous one. So we could for example keep the head transform mapper correctly during the transition, which is what localPlayer.position is.

@avaer
Copy link
Contributor

avaer commented Feb 14, 2022

the VRM templates were running the toon shader on the models twice, once in default for unskinned, and one in the setSkinning function, and so it's currently doing that for spriting as well. I assume it should only need to be run once per VRM and reused?

Yes, that is correct, there are a lot of inefficiences in the avatars rendering.

Particularly:

  1. There is no real need to have skinned/unskinned avatars at all. This was a stopgap put in place because the bones computation was a killer and not needed for the unskinned (unworn) avatars case, and we wanted to get around it, so we created a mesh that doesn't do it. We would ideally re-use the avatar when we wear it; we just need to perform the proper resets and matrix update skips to keep the updates fast.
  2. We are not using the crunched version of the avatar for depth computations, when we really should. The avatar should not contributed 50 more geometry draw calls just to get its depth, when it could be one.

@avaer
Copy link
Contributor

avaer commented Feb 14, 2022

applying the quality changes to all the avatars takes quite a bit of time, it can surely be sped up, but I didn't want to go too far off task adding a loading screen. Is the expectation that all the down-scaling will be done on the client side on load? Any plans to store/reuse models online/locally?

This should ideally be done in a thread for starters, so it doesn't hitch. It is indeed a heavy optimization process to create a full animated spritesheet/texture atlas for an avatar.

Is the expectation that all the down-scaling will be done on the client side on load?

No, once we get it working in a separate thread, it is straightforward to set this up as part of the backend by importing that code. This could be done in totum, and cached, for example.

But this is indeed outside the scope of just getting it persisting.

@avaer
Copy link
Contributor

avaer commented Feb 14, 2022

Maybe removing the dial skinned/unskinned VRM model format should be part of this PR, since it might be getting in the way of clean initialization code.

@0reo
Copy link
Contributor Author

0reo commented Feb 14, 2022

Maybe removing the dial skinned/unskinned VRM model format should be part of this PR, since it might be getting in the way of clean initialization code.

sounds good to me

@avaer
Copy link
Contributor

avaer commented Feb 15, 2022

This PR touches on:

#2339
#2341
#2343

@0reo
Copy link
Contributor Author

0reo commented Feb 17, 2022

I've removed the unskinned meshes from the vrm template and cleaned up some bugs when switching between settings. There is an option to make other vrm's NPC's, but they are no longer NPC's by default. I'll push the branch here once I clean up the commit history tomorrow, but i'll link the temporary branch here until that's been done.

I've also started on the diorama updates, would like to confirm that the diorama avatar will ONLY be the crunched avatar, regardless of the user settings, or if it will match the user's settings.

https://github.com/webaverse/app/compare/master...0reo:temp-changes?expand=1

@avaer
Copy link
Contributor

avaer commented Feb 17, 2022

I've removed the unskinned meshes from the vrm template and cleaned up some bugs when switching between settings.

Awesome if true. This has been a problem for a while.

There is an option to make other vrm's NPC's

I don't understand this... NPCs and VRMs are different types of apps. Can they be converted somehow? I'm probably just confused.

@avaer
Copy link
Contributor

avaer commented Feb 17, 2022

I've also started on the diorama updates, would like to confirm that the diorama avatar will ONLY be the crunched avatar, regardless of the user settings, or if it will match the user's settings.

Some of the crunched avatars have morph target glitches when speaking, which can look bad in the diorama. But if we can get around that, that is ideal.

I think the ideal is crunched avatar in the diorama, and also for all depth rendering. The only place we need a different avatar render is 1) the main color pass in the scene, which should always respect the setting, or 2) spritesheet rendering, which is a completely different type of pass which alpha-discards, and would need a special case in the diorama to have the depth material work correctly instead of just drawing a square.

@avaer
Copy link
Contributor

avaer commented Feb 17, 2022

There were some recent changes in this area so this PR could use a merge from master.

@@ -869,6 +869,12 @@ export default () => {
getAvatarHeight(obj) {
return getHeight(obj);
},
getQualitySetting(){
return parseInt(localStorage.getItem('avatarStyle') || 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should include second arg to parseInt.

@@ -5,12 +5,16 @@
"main": "index.mjs",
"scripts": {
"start": "forever start index.mjs -p",
"dev": "node index.mjs",
"stop": "killall -SIGINT webaverse || true",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why make it auto-succeed? Seems like that would just lose information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on first run of watch there's no instance of webaverse running , and so the restart command would fail. This allows us to restart regardless of if there's an instance running already or not, while still showing the output from killall.

src/App.jsx Outdated

const gpuTier = await getGPUTier();
console.log("GPU INFO", gpuTier);
if (!metaversefileApi.getQualitySetting()){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would check for undefined or null, so this isn't confused with 0.

game.js Outdated
localPlayer.avatar.setQuality(quality);
npcs.forEach((npc) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we shouldn't iterate the NPCs, but the VRM apps in the world.appManager. NPCs are an abstraction over the VRM apps and would automatically be included.

That way even non-NPC VRMs will be optimized.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, this was based on my assumptions regarding NPC's which I commented earlier, and that the setQuality function was part of the Avatar class. Will be changing to call the vrm apps's directly.

@@ -166,6 +166,9 @@ class StatePlayer extends PlayerBase {
appsMap: null,
});
this.appManager.addEventListener('appadd', e => {
const trackedApp = this.appManager.getTrackedApp( e.data.instanceId);
const load = trackedApp.get('load');
Copy link
Contributor

@avaer avaer Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this works. Loading is conceptually a local property. But here we are making it a networked property shared over multiplayer? Won't that confuse multiplayer loading?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was done because i wanted the ability to add a totum app into the app manager, without it being immediately added to the scene. i had added a new "load" argument to appTrackedApp which defaults to true, setting it to false allows you to manually add it(load it) to the scene by setting it to true at a later time. Calling it "addToScene" may be more clear. But it may be unnecessary.



const quality = metaversefile.getQualitySetting();
avatar.setQuality(quality).then(()=>{
Copy link
Contributor

@avaer avaer Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems this should be in totum, and should block the load of the VRM (e.waitUntil) until we set the quality for it.

This doesn't seem related to player/avatar bindings, so it shouldn't be in this file (probably makeAvatar shouldn't be here either).

Also, setting extra multiplayer properties like load seems like a fragile hack, which won't be needed if we do this in the totum VRM implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, it's already removed from player-avatar-binding in the work i did yesterday.

@0reo
Copy link
Contributor Author

0reo commented Feb 17, 2022

I don't understand this... NPCs and VRMs are different types of apps. Can they be converted somehow? I'm probably just confused.

When I initially read Make the setting apply to all avatars in the scene (all [totum](https://github.com/webaverse/totum/) [VRM](https://github.com/webaverse/totum/blob/main/type_templates/vrm.js)), and I saw how quality settings were being set using avatars.js, I took that to mean that all app's created from the vrm templates would be attached to a type of Avatar. I saw the NPC class was a type of Avatar and figured that made the most sense to use. Since we've now spoken about having vrm's in the scene which aren't necessarily NCPs(for example not needing idle animation), I set it up so that in the scene templates(grid.scn for example) you can add 'is_npc: true' to any vrm's listed, and only if set will the scene template attach the vrm app to a new NPC.

@avaer
Copy link
Contributor

avaer commented Feb 17, 2022

I set it up so that in the scene templates(grid.scn for example) you can add 'is_npc: true' to any vrm's listed, and only if set will the scene template attach the vrm app to a new NPC.

Ok so in the engine there is currently not something like "an npc" -- though there are many utilities included for coding an NPC, the engine itself doesn't track NPCs or implement them. Instead an NPC app loaded through totum uses the APIs to administer itself, providing the actual coded behavior that the NPC is to perform -- how they move, speak, and respond. The app is the brain of the NPC which interfaces over metaversefile to perform functions and respond to inputs.

However, we can make npcs a first-class concept in totum, and that might be best. For example, the render settings are a first class-concept -- they are a loadable module implemented as a totum type, which lets it be used in any scene. We could do the same for NPCs, making a new e.g. .npc type which is JSON describing the NPC, and then implemented as a totum loader. That would allow for fully declarative NPCs.

However if we go that route, I don't think the scene should be setting up NPCs or aware of them. The scene should just have NPC typed objects in it and the scene loads them blindly like anything in Totum.

I.e. I don't think we should have is_npc at all.

Here is the current NPC implementation, though it will not work in master (it's not used in any scenes are requires a private repo to run): https://github.com/webaverse/npc However it could quite naturally become a totum type handler -- it's all just apps against the same metaversefile API.

@0reo
Copy link
Contributor Author

0reo commented Feb 17, 2022

Instead an NPC app loaded through totum uses the APIs to administer itself, providing the actual coded behavior that the NPC is to perform -- how they move, speak, and respond. The app is the brain of the NPC which interfaces over metaversefile to perform functions and respond to inputs.

got it.

However, we can make npcs a first-class concept in totum, and that might be best. For example, the render settings are a first class-concept -- they are a loadable module implemented as a totum type, which lets it be used in any scene. We could do the same for NPCs, making a new e.g. .npc type which is JSON describing the NPC, and then implemented as a totum loader. That would allow for fully declarative NPCs.

However if we go that route, I don't think the scene should be setting up NPCs or aware of them. The scene should just have NPC typed objects in it and the scene loads them blindly like anything in Totum.

I.e. I don't think we should have is_npc at all.

understood and aggreed

Here is the current NPC implementation, though it will not work in master (it's not used in any scenes are requires a private repo to run): https://github.com/webaverse/npc However it could quite naturally become a totum type handler -- it's all just apps against the same metaversefile API.

@0reo 0reo force-pushed the performance-settings branch from a3204f1 to d2dd42e Compare February 19, 2022 23:32
@0reo 0reo force-pushed the performance-settings branch from d2dd42e to fdecf2e Compare February 20, 2022 00:21
@0reo
Copy link
Contributor Author

0reo commented Feb 22, 2022

Diorama and depth rendering are addressed in the most recent push. Quality settings are set up to be applied to all vrms(rather than avatars/npcs), but ran into a roadblock trying to run the avatar cruncher in a worker, which was needed to apply the quality settings to other vrm's now that they aren't using the NPC class(although that still would have really slogged if there were too many items on screen). I think I figured it out while writing this though. I was unable to load the vrm to be cloned into the worker, nor could I send the serialised data and recreate the vrm inside the worker, and I couldn't figure out why. But I believe it's because of references to window in metaversefile, which seems to get loaded into most if not all the totum templates. Wasn't getting the error because the reference to window wasn't in the worker until after it loaded. Updating metaversefile(and possibly some other files) to reference self rather than window should get everything playing nicely together. Will test tomorrow, and that should wrap up the ticket.

@0reo
Copy link
Contributor Author

0reo commented Feb 23, 2022

quality settings now apply to all vrms. that said, there seems to be an issue with crunching miku specifically, regardless of the number of vrms in the scene.

@ahadshams
Copy link
Contributor

if it works fine for other VRMs (I can also provide you a few), will be good to know why its not working with Miku but we can park any specific issue related to Miku as its a stock asset and already is flagged as resource intensive

@0reo
Copy link
Contributor Author

0reo commented Feb 23, 2022

if it works fine for other VRMs (I can also provide you a few), will be good to know why its not working with Miku but we can park any specific issue related to Miku as its a stock asset and already is flagged as resource intensive

sounds good. i was able to test with scillia and scillia_drophunter_v15_vian models successfully so far. one guess is the number of bones the model has, but that's just a guess, haven't dug deeper into it yet

@0reo
Copy link
Contributor Author

0reo commented Mar 4, 2022

this has not been reviewed and is likely stale. I'm going to close this, update it with the current master, and make a new pr

@0reo 0reo closed this Mar 4, 2022
@0reo 0reo added this to the Avatar Quality Settings milestone Mar 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Performance settings persistence
4 participants