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 sound to timer when it alerts #2378

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

96LawDawg
Copy link
Collaborator

@96LawDawg 96LawDawg commented Nov 29, 2024

Fixes #1333 if this and #2377 are merged.

Adds a property to timers alertSound that links to a URL and is played if the timer alert is triggered.

The tutorial is updated to relfect the change.


PR-SERVER-BOT: You can play around with it here: https://test.virtualtabletop.io/PR-2378/pr-test (or any other room on that server)

Fixes #1333 if this and #2377 are merged.

Adds a property to timers `alertSound` that links to a URL and is played if the timer alert is triggered.

The tutorial is updated to relfect the change.
@96LawDawg 96LawDawg added the enhancement New feature or request label Nov 29, 2024
@bjalder26
Copy link
Collaborator

Comparing the old code to the new code, this looks like it just takes the check to see if the alert should be set, and stores that in a variable that is then used to 1) trigger audio, and 2) sets the alert (as usual).

The only feedback I have is that there looks like there is extra space before const isAlert ... which should be deleted, and spaces were added around <= and >=, which might be better for all I know.

Also, if #2377 is merged first (as is), then count: 1 should be added.

@ArnoldSmith86
Copy link
Owner

Why do you link to the web instead of including the sound file as an asset?


In theory, it probably makes more sense to trigger playing the audio in applyChangesToDOM when alert changes. Because the change of alert is already sent to all clients and your implementation makes a separate broadcast to all clients in order to play the audio.

I don't want to work on this though so if you don't want to refactor this, it can just stay the way it is. It's just a few bytes...

@96LawDawg
Copy link
Collaborator Author

Why do you link to the web instead of including the sound file as an asset?

Fixed.

In theory, it probably makes more sense to trigger playing the audio in applyChangesToDOM when alert changes. Because the change of alert is already sent to all clients and your implementation makes a separate broadcast to all clients in order to play the audio.

I don't want to work on this though so if you don't want to refactor this, it can just stay the way it is. It's just a few bytes...

It's not that I don't want to get it right, but I don't understand what you are suggesting. I'll leave it here in case @bjalder26 can figure it out. Otherwise, I'll probably merge in a couple of days.

@ArnoldSmith86
Copy link
Owner

In applyDeltaToDOM in timer.js, check for delta.alert === true and if it is, call await addAudio(audioSource, maxVolume, length) directly. No need for toServer in that case.

Give it a try but it's no big deal if we use the current version.

@96LawDawg
Copy link
Collaborator Author

In applyDeltaToDOM in timer.js, check for delta.alert === true and if it is, call await addAudio(audioSource, maxVolume, length) directly. No need for toServer in that case.

That worked and makes sense now that you explain it. Except I couldn't use await because it isn't async. Or that's what VSC told me.

@ArnoldSmith86
Copy link
Owner

Except I couldn't use await because it isn't async.

That's fine but then please append .fetch(...) so that audio exception get logged to the console but don't trigger error reporting. Ideally, I would like to know about them but audio issues shouldn't prevent players from playing..

@bjalder26
Copy link
Collaborator

Is this the type of thing you're looking for?

fetch(this.get('alertSound'))
  .then(response => {
    if (!response.ok) {
      throw new Error('Failed to fetch alertSound');
    }
    return response;
  })
  .then(response => addAudio(this.get('alertSound'), 1, 0, 1))
  .catch(error => {
    console.error('AlertSound error:', error); 
  });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Repeat Sound and Add to Timer
3 participants