user variables for scripted cycles - #110
Conversation
Benchmark for f153fdfClick to view benchmark
|
Benchmark for c8242c9Click to view benchmark
|
Benchmark for 77adad2Click to view benchmark
|
|
Looks great. I'll add a few nitpicking comments in the review and will do more testing over the weekend. Else this looks straight forward and like a really great addition! |
|
Hey, did you mean that you wanted to add those comments to this as-is, or are you waiting for me here to make the PR more complete? I'd appreciate any nitpicks here before continuing! |
|
There should be a new |
Benchmark for cf34af4Click to view benchmark
|
|
I've separated those and refactored the scripted cycle a bit, since the cycle either has a static table mapping or a callback (and a timeout hook always exist with a callback and never with a mapping), having these as an enum makes the code more self-explanatory. Added a new kind of context for the variables callback that gets the parameters and the current iteration of the cycle assigned, do you think anything else would make sense here? Updated the API docs and the guide with the var function example. |
Benchmark for 259018aClick to view benchmark
|
|
That looks great. Thanks. Give me some time to review and test this, as this is a larger change. Will do do ASAP beginning next week. |
Yes, all non mapping specific stuff that's also in the mapping callbacks: playback_state, time_base There are a few other small issues, like missing resets for the callback. This stuff unfortunately is really tricky in the details. I'll go though this in detail tomorrow and will fix that, okay? |
That would be great! |
…d cycles with variable callbacks
Benchmark for 21d5337Click to view benchmark
|
|
Fixed a few little things here and there and updated the callback context generation and docs: This is what the /// Sets the cycle context for the mapping callbacks.
pub fn set_cycle_map_context(
&mut self,
playback_state: ContextPlaybackState,
time_base: &BeatTimeBase,
channel: usize,
step: usize,
step_length: f64,
) -> LuaResult<()> {
self.set_context_playback_state(playback_state)?;
self.set_context_time_base(time_base)?;
self.set_context_cycle_step(channel, step, step_length)?;
Ok(())
}
/// Sets the cycle context for var callbacks.
pub fn set_cycle_var_context(
&mut self,
playback_state: ContextPlaybackState,
time_base: &BeatTimeBase,
parameters: &ParameterSet,
iteration: u32,
) -> LuaResult<()> {
self.set_context_playback_state(playback_state)?;
self.set_context_time_base(time_base)?;
self.set_context_parameters(¶meters)?;
self.set_context_cycle_iteration(iteration)?;
Ok(())
}Actually wouldn't hurt to also add the new Using an enum for the map callback or static map makes a lot of sense as they are mutually exclusive. I think we should do the same for the Unlikely to be a problem, but if both, I'll try to take care of this tomorrow. Else I think this is ready and works great as it is! |
|
Thanks! It's a small thing but this updated description of the
I've added one to the iteration (that starts from zero) so it would be an idiomatic index in lua. When the It's tangentially related but while experimenting with variables I found the way enum parameter's need to be defined to be fairly inergonomic in that you have to write one case two times (one for the default arg and one inside the list). The work around is to define the table outside but that's adding boilerplate. It would be nice if for example |
The old doc comment got removed by accident. Sorry. Restored it now...
Yes, definitely. I think it would be "safe" to allow strings or numbers here. When passing a string, it must be one of the enum values, when passing a number it's interpreted as index. So using |
Benchmark for fc659d9Click to view benchmark
|
|
I've now made variables and mappings both: in
Currently, static variable tables are applied before parameter variables because the generate function applies parameter values on every run, as these values may change. If we want var to override parameter values, then we'd need to apply them every time in generate as well. Currently, this avoids some overhead by not doing so. Tested everything as thoroughly as possible. It would be great if you could also give this a try. Seems ready to merge from my side. |
Did you forgot to push this? I think we definitely want to let tables override the parameters. This is surely the more useful and less confusing behaviour. If we already cache the table the way you described above, then we could simply skip assigning/converting any parameter to a variable whose identifier exists in the variables table. This would result in less overhead as we could skip parsing parameters when their results would be overriden by the table anyway. Later on we can add caching unchanged parameter results as well. |
Oops. Indeed. Will do so tomorrow. Don't have that here on this computer.
Agree, but this will be more work then. I'll take care of this tomorrow as well. |
…make clear when and how vars are applied within the scripted cycle
|
Pushed that now and also added the var, parameter override. Sorry for the fuzz. |
Benchmark for 5710dd1Click to view benchmark
|
Benchmark for 68dab3dClick to view benchmark
|
2 participants
An initial draft for the discussed
varsfunction on the cycle object.Works similar to
:mapin that you can supply either a static table or a function returning a table.I guess we'll need a different context for
vars? Since variables are only being assigned once pergenerateinstead of for each event, the fields related to single steps are meaningless.Here, the table variant inserts the variables to the cycle directly instead of passing around the table (contrary to the case with
map) whereas the callback variant follows a similar pattern to themapping_function.Not sure what else should happen with the callback and its errors, but it seems to work fine so far.