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 SetValueCurveAtTime #230

Merged
merged 1 commit into from Apr 4, 2019

Conversation

Projects
None yet
4 participants
@JHBalaji
Copy link

commented Apr 2, 2019

fixes #204

@Akhilesh1996

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

Hi,

We have finished implementing the SetValueCurveAtTime function (https://github.com/JHBalaji/media-1/blob/master/audio/param.rs#L418-L436) and written an example (https://github.com/JHBalaji/media-1/blob/master/examples/set_value_curve.rs) to check the functioning of the same.

@Manishearth
Copy link
Member

left a comment

Please run rustfmt on the test and param.rs.

let dest = context.dest_node();

//Initializing the values vector for SetValueCurve function
let mut values: Vec<f32> = Vec::new();

This comment has been minimized.

Copy link
@Manishearth

Manishearth Apr 3, 2019

Member

You can use let values = vec![0., 0., 0., 0., 1., 1., 1., ....]

@@ -324,6 +329,7 @@ impl AutomationEvent {
match *self {
AutomationEvent::SetValueAtTime(_, tick) => Some(tick),
AutomationEvent::RampToValueAtTime(_, _, tick) => Some(tick),
AutomationEvent::SetValueCurveAtTime(_, _, tick) => Some(tick),

This comment has been minimized.

Copy link
@Manishearth

Manishearth Apr 3, 2019

Member

This should be start + duration

/// https://webaudio.github.io/web-audio-api/#dfn-automation-event
pub(crate) enum AutomationEvent {
SetValue(f32),
SetValueAtTime(f32, Tick),
RampToValueAtTime(RampKind, f32, Tick),
SetTargetAtTime(f32, Tick, /* time constant, units of Tick */ f64),
SetValueCurveAtTime(Vec<f32>, Tick, /* duration */ Tick),

This comment has been minimized.

Copy link
@Manishearth

Manishearth Apr 3, 2019

Member

both these variants should have a comment saying that the first Tick is the start time

@@ -408,6 +415,25 @@ impl AutomationEvent {
*value = val + (event_start_value - val) * exp.exp() as f32;
true
}
AutomationEvent::SetValueCurveAtTime(ref values, start, duration) => {
let time_diff = ((duration.0 as f32) - (start.0 as f32)) as f32;

This comment has been minimized.

Copy link
@Manishearth

Manishearth Apr 3, 2019

Member

duration is already the time difference here

AutomationEvent::SetValueCurveAtTime(ref values, start, duration) => {
let time_diff = ((duration.0 as f32) - (start.0 as f32)) as f32;
let mut progress = ((((current_tick.0 as f32) - (start.0 as f32)) as f32) / time_diff) as f32;
if progress < 0.0 {

This comment has been minimized.

Copy link
@Manishearth

Manishearth Apr 3, 2019

Member

This should never be negative, perhaps we can add a debug_assert!() to check that instead of silently setting it to zero?

let next = k + 1. as f32;
let step = time_diff / (n - 1.);
if next < n {
let time_k = (k * step) as f32;

This comment has been minimized.

Copy link
@Manishearth

Manishearth Apr 3, 2019

Member

This is recalculating stuff we already know, an easier way to do this is to calculate k_float with k = k_float.floor(), and then interpolate between v[k] and v[k+1] with progress k_float - k`.

@Manishearth
Copy link
Member

left a comment

(The k_float stuff still needs to be fixed)

SetValueCurveAtTime(
Vec<f32>,
/* start time, units of Tick */ Tick,
/* duration, units of Tick */ Tick,

This comment has been minimized.

Copy link
@Manishearth

Manishearth Apr 3, 2019

Member

(no need to clarify the unit, the type is the unit)

This comment has been minimized.

Copy link
@Akhilesh1996

Akhilesh1996 Apr 3, 2019

Contributor

Hi Manish,

Thank you for the comments, I don't quite understand how to implement the k_float stuff. Do you want me to get rid of the step variable and just store (current_tick.0 as f32) - (start.0 as f32) in t_k (which will be progress * duration) and something else in t_k_next? If so, I don't understand how I can calculate the t_k_next value without using the step value.

Can you help me out with this?

Thanks.

This comment has been minimized.

Copy link
@Manishearth

Manishearth Apr 3, 2019

Member

You don't need time_k_next

You already have k_float - k, which is a number 0 to 1 representing where to interpolate.

So you can do something like

if k + 1 < n {
   let progress = k_float - k
   *value = v[k] * (1 - progress) + v[k + 1] * progress;
} else {
  *value = v[n - 1];
}

This comment has been minimized.

Copy link
@Akhilesh1996

Akhilesh1996 Apr 3, 2019

Contributor

Hi Manish,

Thank you. I have made the changes and ran rustfmt. Is it fine now?

@Manishearth
Copy link
Member

left a comment

Looks good, please squash your commits

@Akhilesh1996

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

Hi Manish,

Thank you.

We had first cloned the repo and added commits, after that forked the repo again and pushed changes to it. So there are 6 commits made to the original servo/media in-between the commits made by us. How would you advise us to rebase and squash our commits into a single one?

Thanks
Akhilesh

@Manishearth

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

@JHBalaji

This comment has been minimized.

Copy link
Author

commented Apr 4, 2019

Thanks. Would report back and hopefully in a while.

@JHBalaji

This comment has been minimized.

Copy link
Author

commented Apr 4, 2019

@Manishearth The merge between the repos and branches we did earlier seems to causing trouble with Squashing. However I have synced with upstream now. Could you please advise on how to squash the commits in this situation?

@Manishearth Manishearth force-pushed the JHBalaji:master branch from 040f237 to 2e93d0f Apr 4, 2019

@Manishearth Manishearth force-pushed the JHBalaji:master branch from 2e93d0f to c80549a Apr 4, 2019

@Manishearth

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

You had some merge conflicts that needed to be worked out. I pushed a fix.

In the future, please rebase over master instead of merging in master.

@Manishearth

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

📌 Commit c80549a has been approved by Manishearth

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

⌛️ Testing commit c80549a with merge edfca0c...

bors-servo added a commit that referenced this pull request Apr 4, 2019

Auto merge of #230 - JHBalaji:master, r=Manishearth
 Implemented SetValueCurveAtTime

fixes #204
@JHBalaji

This comment has been minimized.

Copy link
Author

commented Apr 4, 2019

Thank you. We would take that note.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

☀️ Test successful - checks-travis
Approved by: Manishearth
Pushing edfca0c to master...

@bors-servo bors-servo merged commit c80549a into servo:master Apr 4, 2019

2 of 3 checks passed

Travis CI - Pull Request Build Created
Details
Travis CI - Pull Request Build Passed
Details
homu Test successful
Details

bors-servo added a commit that referenced this pull request Apr 5, 2019

Auto merge of #235 - ferjm:rm.media.submodule, r=Manishearth
Remove empty 'media' submodule

#230 introduced an empty `media` submodule. I assume this was accidental and it is not needed, so I am deleting it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.