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

ECS wrapper for the sound system #590

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

Conversation

IsaacLic
Copy link
Contributor

@IsaacLic IsaacLic commented Mar 5, 2021

Description

This is an ECS-based method for playing sounds. It makes use of the existing OggSoundManager, but adds support for entities. It also adds a MaterialType enum, currently only used for determining which sounds to play. In addition, it removes the duplicate, unused classes from game/sounds, which were identical to classes with the same names in assets/sounds.

There are three types of sound-playing added:

  • If an entity takes damage, if there is a specified damage sound for the type of damage and the material of the entity, a sound will be played.
  • When an entity experiences a collision, a collision sound is played based on the entity's type of material.
  • If a specific sound should be played, a SoundEvent can be sent.

This PR also adds in Asteroid's AsteroidCrack sound, which is the sound played when an asteroid gets destroyed.

Testing

  • Collide with the golden asteroid. It should make a sound when collided with.
  • Shoot the asteroid. It should make a sound when it gets destroyed (make sure your volume is on! it may need to be on max)

I wasn't able to test the collision sound locally (my setup has a few issues for me to work out), so that one especially needs testing.

@IsaacLic IsaacLic marked this pull request as ready for review March 9, 2021 02:44
Copy link
Member

@NicholasBatesNZ NicholasBatesNZ left a comment

Choose a reason for hiding this comment

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

Lots of good work here, thanks. And sorry for the extended delay :(
You may have to explain some things to me if I don't understand it properly :)


/**
* Gets the damage sound associated with the given {@link MaterialType} and {@link DmgType}. If no sound is defined,
* null is returned.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe return an Optional instead then for these two methods?
UPDATE: In light of later review comments, you may not even want this method at all

public DebugHint(SolObject owner, Vector2 position) {
private EntityRef entity;

public DebugHint(SolObject owner, EntityRef entity, Vector2 position) {
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this would be appropriate as an overload so you don't have to send through arbitrary null values

public float getDamage() {
return damage;
}

public DmgType getDamageType() {
return damageType;
Copy link
Member

Choose a reason for hiding this comment

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

What if damageType is null? Could make it an Optional

PlayableSound sound = specialSounds.getHitSound(materialType, event.getDamageType());
if (sound != null) {
Vector2 position = entity.getComponent(Position.class).get().position;
soundManager.play(game, sound, position, entity);
Copy link
Member

Choose a reason for hiding this comment

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

Why not reuse SpecialSounds.playColl()?

MaterialType materialType = entity.getComponent(Material.class).get().materialType;
PlayableSound collisionSound = specialSounds.getCollisionSound(materialType);
if (collisionSound != null) {
soundManager.play(game, collisionSound, position, entity, magnitude * Const.IMPULSE_TO_COLL_VOL);
Copy link
Member

Choose a reason for hiding this comment

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

Why not reuse SpecialSounds.playHit()?

return false;
}

Map<OggSound, Float> looped = loopedSoundMapOfEntities.get(source);
Copy link
Member

Choose a reason for hiding this comment

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

This is all duplicated code. Can you really not unify this?

* @param damageType the type of damage done
* @return the sound of the damage
*/
public PlayableSound getHitSound(MaterialType materialType, DmgType damageType) {
Copy link
Member

Choose a reason for hiding this comment

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

Why does this deserve to exist if hitSound(metal, dmgType) is already a thing?

@Cervator
Copy link
Member

Cervator commented Sep 3, 2021

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.

3 participants