equip sound + sword grunt + sword sound #3674
Conversation
tcm390
changed the title
character-sfx.js
Outdated
| })); | ||
| } | ||
| const _handleCombo = () => { | ||
| if (this.player.hasAction('use') && this.player.getAction('use').behavior === 'sword' && this.player.getAction('use').animationCombo) { |
There was a problem hiding this comment.
Why look it up 3 times per if?
character-sfx.js
Outdated
| if (index > maxDeltaIndex) { | ||
| this.alreadyPlayComboSound = true; | ||
| this.playGrunt('attack'); | ||
| const soundIndex = this.player.avatar.useAnimationIndex * 4 + Math.floor(Math.random() * 4); |
There was a problem hiding this comment.
Why 4? Don't include random constants in the middle of the code.
There was a problem hiding this comment.
| @@ -0,0 +1,45 @@ | |||
| import * as THREE from 'three'; | |||
There was a problem hiding this comment.
Why break out sound only into the component? There is a lot of use behavior we can move to the use component, but this simply makes the code worse for now since it fragments the behavior even more across files.
If we are doing to break out use into a component, we should do it for all use behavior, not just sounds.
| } | ||
| } | ||
| const _unwear = () => { | ||
| if (component.behavior === 'sword') { |
There was a problem hiding this comment.
This should not be an if, but a declarative object.
| })); | ||
| } | ||
| const _handleCombo = () => { | ||
| if (this.player.hasAction('use') && this.player.getAction('use').behavior === 'sword') { |
There was a problem hiding this comment.
Why is this query tripled/quadrupled? Each one is a lookup so this is really inefficient.
| this.player = player; | ||
|
|
||
| this.lastJumpState = false; | ||
| this.lastStepped = [false, false]; | ||
| this.lastWalkTime = 0; | ||
|
|
|
|
||
|
|
||
|
|
||
| this.lastSwordComboName = null; |
There was a problem hiding this comment.
There are now so many random state keys on this class that it needs to be broken up further.
|
|
||
| }; | ||
|
|
||
| _handleCombo(); |
There was a problem hiding this comment.
This is poor quality unstructured code. The rest of the file has the exact same problem of being random unstructured if statements with a huge state vector, but it's gotten to the point where it is difficult for anyone to review this or even understand what is going on here.
IMO this file needs a complete redesign under some sort of structured coding theory before anyone can review it.
| @@ -534,6 +534,122 @@ const {CharsetEncoder} = require('three/examples/js/libs/mmdparser.js'); | |||
| }); | |||
| // console.log('got animations', animations); | |||
|
|
|||
| const comboAnimationNames = [ | |||
There was a problem hiding this comment.
This code is completely unabstracted and unreadable.
There was a problem hiding this comment.
There are no comments or functions here, so it's not clear what this is supposed to do or why it would be correct.
There was a problem hiding this comment.
The code quality on this PR and associated files is too poor to properly review. There is little function abstraction, with massive code blocks, repeated lookups with little variable reuse, and enormous state vectors on classes that are randomly accessed.
Most of the files this touches need a structured rewrite before this can even be reviewed for correctness.
metaverse_components/wear.jsinstead ofcharacter-sfx.js(Should we just only play the equip sound for localPlayer?)
index.jsto play the sword whoosh, can we create a metaverse componentsusefor the app to handle the sound event?Sibling:
webaverse/sword#2
related:
#3659
Result:
Webaverse.-.Google.Chrome.2022-08-31.20-40-50.mp4