-
Notifications
You must be signed in to change notification settings - Fork 409
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 two new update methods for AudioComponents to use #1120
Conversation
protected virtual void UpdateComponent() | ||
{ | ||
|
||
} |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
@@ -43,10 +43,10 @@ protected override void Dispose(bool disposing) | |||
base.Dispose(disposing); | |||
} | |||
|
|||
public override void Update() | |||
protected override void UpdateState() |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
@@ -119,7 +119,7 @@ public override void Update() | |||
currentAmplitudes.FrequencyAmplitudes = new float[256]; | |||
} |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks okay to me, I guess. We'll determine later if running the FFT twice is going to be an issue.
Awaiting @Tom94's review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it stands now I see 2 problems with this PR:
- Calling
UpdateState
twice does not fully address race conditions. It merely reduces the latency until cached values are updated and thereby makes data races less likely to occur. I would prefer a more fundamental solution to this problem. - Setting
isPlaying
tofalse
from the update thread may cause unpredictable behavior in audio-thread code which depends on the current state ofisPlaying
. Essentially one type of data race is replaced by another.
Also changes order of execution to update the state *after* all queued actions are completed.
2d84a1c
to
4d6b330
Compare
@@ -21,9 +21,17 @@ public class AudioComponent : IDisposable, IUpdateable | |||
} | |||
|
|||
/// <summary> | |||
/// Run each loop of the audio thread after queued actions to allow components to update anything they need to. | |||
/// If we require |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This allows state updates to be performed a second time post queued actions. Until now, if a queued action was to change a state which only got revalidated in a component's Update, it would be stale for a single frame.
This caused all end of race conditions, such as ppy/osu#1369