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

add sword grunt #3660

Closed
wants to merge 8 commits into from
Closed

add sword grunt #3660

wants to merge 8 commits into from

Conversation

tcm390
Copy link
Contributor

@tcm390 tcm390 commented Aug 21, 2022

related: #3659

Sibling:
webaverse/silsword#12

sword does not have index.js to play its whoosh sound. Should I play the sound in totum?

Result:

Webaverse.-.Google.Chrome.2022-08-22.16-21-46.mp4

@tcm390 tcm390 marked this pull request as draft August 21, 2022 23:12
@tcm390 tcm390 marked this pull request as ready for review August 22, 2022 20:32
character-sfx.js Outdated
currentCombo = this.player.getAction('use').animation ?
this.player.getAction('use').animation // sword
:
this.player.getAction('use').animationCombo[this.player.avatar.useAnimationIndex] // silsword
Copy link
Contributor

@avaer avaer Aug 23, 2022

Choose a reason for hiding this comment

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

Why is this mentioning app names? Apps have nothing to do with the engine.

character-sfx.js Outdated

// combo
const dispatchComboSoundEvent = (combo, indexOfCombo) =>{
this.dispatchEvent(new MessageEvent('comboSoundEmit', {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a poor name for the event, not consistent with other events.

Copy link
Contributor Author

@tcm390 tcm390 Aug 23, 2022

Choose a reason for hiding this comment

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

Got it. Renamed event
webaverse/silsword@5d403c4
38d71b6

character-sfx.js Outdated
this.currentComboIndex = 0;
}
case 0: {
if(timestamp - this.playComboTime >= swordAnimationOffset[this.currentComboIndex]){
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that this code is repeated, this is not a good way to write this method. There should not be any numerical state machine.

Copy link
Contributor Author

@tcm390 tcm390 Aug 23, 2022

Choose a reason for hiding this comment

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

@avaer
Copy link
Contributor

avaer commented Aug 23, 2022

I think the sword animation still needs to be redone.

Both sword types/combo types should have the sword whoosh.

The event name could be meleewhoosh or something.

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.

2 participants