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

Implemented horn sound base functionality #73

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

Implemented horn sound base functionality #73

wants to merge 6 commits into from

Conversation

AdamLacko
Copy link

@AdamLacko AdamLacko commented Nov 15, 2019

-Added GetKey method (modified copy of GetKeyDown method already present there) in CustomInput to operate continuous key pressing

-Looping of the horn sound is still not perfect yet. As we discussed before, problem is apparently in the original horn sounds SA engine was most probably modifying at runtime to better blend.

-I do have concerns about the method MakeSubclip() that is used to create loop-able sample for AudioSource, as it hasn't been tested for leaks yet. There are probably none, but it's a good practice to test it anyways. So should be tested horn AudioSource, since it has to be created additionally (due to new radio implementation which already uses one AudioSource), to prevent more of these components being created on, say, scene loads or whenever Awake is additionally called (but I reckon such scenario is not very common in game like this? I'm thinking this further as I work with RPGs where such scenario is quite common). I'll do all this in future commints.

-There is a doppler effect on horn sound. It sounds good in environment, although the effect is also laudable for the player inside car as well. This can be prevented pretty easily though, I'll implement that in next commit.

-added method GetKey (modified copy of GetKeyDown method already present there) in CustomInput to operate continuous key pressing
@in0finite
Copy link
Owner

TODO:

  • Make looping of the sound better.
  • PlayerController.ReadEvents() is used to read events, not states. With GetKey() you are reading the state of the key. ReadStates() is used for reading states.
  • BaseScriptState should not be aware of vehicles. Yes, it should have OnHornButtonPressed(), but it should not handle the logic for this. The logic should be handled in VehicleSittingState.
  • Vehicle.IsHornOn has a wrong name. It's name suggests in what state is the horn, but you are using it as 'was horn button pressed this frame'.
  • Use different sound for different type of vehicle.
  • You are creating memory leaks. Every created AudioClip should be destroyed. Or, even better solution would be to create AudioClip only once during application lifetime and store it in a static variable.
  • Should work in multiplayer (I don't think it works right now)
  • As for CustomInput - again, you don't understand the difference between key state and key event. GetKey() is used for state, GetKeyDown() is used for event.

@@ -1,6 +1,7 @@
using UnityEngine;
using SanAndreasUnity.Utilities;
using SanAndreasUnity.Net;
using System;
Copy link
Owner

Choose a reason for hiding this comment

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

unnecessary using

@@ -191,7 +198,7 @@ void Start()

currentRadioStationIndex = Random.Range(0, RadioStation.stations.Length);

Debug.LogFormat("Created vehicle - id {0}, name {1}, time: {2}", this.Definition.Id,
Debug.LogFormat("Created vehicle - id {0}, name {1}, time: {2}", this.Definition.Id,
Copy link
Owner

Choose a reason for hiding this comment

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

don't do style changes

@Bluscream
Copy link

rip P/R

@TheBatter32
Copy link

Umm,,mmmmmm

Copy link

@AntKovachev AntKovachev left a comment

Choose a reason for hiding this comment

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

Approved

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.

5 participants