Conversation
|
disabled references to getGfxSettingJSON and accessing local storage so this can get in without waiting on webaverse/app#2512 |
avaer
left a comment
avaer
left a comment
There was a problem hiding this comment.
Lots of code problems.
In addition, can we remove all name references in this PR? Names should not be used for logic.
type_templates/vrm.js
Outdated
| app.updateQuality = async () => { | ||
| // const quality = getGfxSettingJSON('character').details; | ||
| // return await _setQuality(quality, app) | ||
| const quality = JSON.parse(localStorage.getItem('GfxSettings')); |
There was a problem hiding this comment.
It's unacceptable for Totum to read local storage, since totum can run with environments that have no storage.
This must go through API.
There was a problem hiding this comment.
This is why I had get/set gfxSettings in the metaversefile, which you asked me to remove into a settings.js file.
webaverse/app#2512 (comment)
type_templates/vrm.js
Outdated
| await app.updateQuality(); | ||
|
|
||
| //prep any outstanding meshes | ||
| //may not need this yet |
There was a problem hiding this comment.
it will be immediately needed in the PR's for crunched and sprite meshes. I can delete it now, but it'll immediately be back in the next pr
| app.skinnedVrm = skinnedVrmBase; | ||
| await _toonShaderify(skinnedVrmBase); | ||
| app.skinnedVrms['base'] = skinnedVrmBase; | ||
| app.skinnedVrm = skinnedVrmBase; //temporary support for webaverse code base until it's updated |
There was a problem hiding this comment.
If I delete this, the webaverse app will break because it's currently referencing skinnedVrm when making avatars. We'll need to update the webaverse app to reference skinnedVrms, which will also cause it to break if this has not been merged yet.
| base: {} | ||
| }; | ||
| app.isToon = material => material[0] && material[0].isMToonMaterial; | ||
| app.isBasic = material => material.type == "MeshBasicMaterial" && material.name; //we're only changing named materials |
There was a problem hiding this comment.
Try not to base anything on THREE.js name, that is mostly for user convenience.
|
|
||
| const app = useApp(); | ||
| app.appType = 'vrm'; | ||
| app.active = 'base'; |
There was a problem hiding this comment.
Active sounds like a boolean?
There was a problem hiding this comment.
Active is the mesh that corresponds to the users performance settings. this only has support for base and toon, but there will also be sprite and crunched. I can rename if you'd prefer something else
There was a problem hiding this comment.
Yes. The answer to "What is the app's .active?" sounds like a boolean to me.
|
|
||
| const _prepVrm = (vrm) => { | ||
| //vrm.visible = false; //will need later | ||
| vrm.visible = false; |
There was a problem hiding this comment.
Don't do this. If the VRM is invisible it is probably for good reason.
There was a problem hiding this comment.
there will always be at least 2 vrms loaded, the base(high quality) vrm, and the crunched vrm. this defaults all the meshs to not visible to start, and the required meshes will be toggles accordingly.
| _addAnisotropy(vrm, 16); | ||
| } | ||
|
|
||
| app.getActive = (_app = false) => { |
There was a problem hiding this comment.
This is not a good function.
First of all it it's a getter that takes a parameter, which is bad.
Second, that parameter is not even consistent with the parameter of setActive.
Third, getActive sounds like a boolean. Is it active? But that is not what is returned here.
Fourth, the method is polymorphic and will return many different types.
Can we delete this?
There was a problem hiding this comment.
getActive is going to return the mesh that's corresponded with the current quality setting. this only has support for base(high quality) and toon(ultra quality), but there will also be sprite and crunched. I can rename for clarity, and remove the _app parameter so that only only 1 type is returned, and have the developer simply access .scene on their own.
| const _cloneVrm = async () => { | ||
| const vrm = await parseVrm(arrayBuffer, srcUrl); | ||
| vrm.cloneVrm = _cloneVrm; | ||
| vrm.toonShaderify = _toonShaderify; |
There was a problem hiding this comment.
Don't expose this, it's internal.
| app.isBasic = material => material.type == "MeshBasicMaterial" && material.name; //we're only changing named materials | ||
| app.setMaterial = (name, type, material) => app.materials[type][name] = material; | ||
|
|
||
| app.skinnedVrms = {}; |
There was a problem hiding this comment.
Don't expose this, it's internal.
type_templates/vrm.js
Outdated
| } | ||
|
|
||
| app.active = target; | ||
| !app.getActive().parent && _prepVrm(app.getActive()); |
There was a problem hiding this comment.
This is a very strange check. What's the point?
There was a problem hiding this comment.
If the active mesh is not added to the scene, then prep it(which adds it to the scene)
|
Given the problems in this PR I recommend a redo. I would not start with this code. |
2 participants
prepare toon materials and store basic and toon materials for quality changing. depnds on #72 and webaverse/app#2512. Will remove draft status once they have been accepted
closes #74