Conversation
Benchmark for b41e98eClick to view benchmark
|
|
Looks great so far. Only have read through the diff - haven't tried it out yet, but will soon. Should I take care of injecting the vars or do you want to? We need to take care here and avoid wasting useless CPU cycles in case parameters don't change and in case there are no variables present in the cycle. Else this should be straight forward.
I'd vote for throwing errors here, as I can't think of a use case where remapping parameter string values would make sense or add a great new feature. Especially because you can't remap a parameter value to complex expressions but just note values or numbers. |
|
Note, this could be useful for not just straight parameter injection, but being able to do something programmatic with or without parameters. Like what you suggested with I imagine a similar thing as I suppose injecting the parameters should be the default behaviour without the need to write anything but a function could be specified to override this.
You can map to any |
Wouldn't this be possible with a regular identifier ( Either way, we can simply assert unresolved identifiers after mappings have been applied, at: pattrns/src/emitter/scripted_cycle.rs Line 131 in eb50150 But I'd design this for what we have now, rather than what we might have later. If we add other things later, we can adjust the assertions then as well. |
The difference is that variables are resolved any time the cycle generates so you can inject values that affect the structure of the output, whereas by remapping identifiers, you can only change the events after they have been generated. The things on the right side of all operators are affected by this:
If the variable is on the left side and both variable and remap uses the same value then the results are the essentially the same, |
|
Because of the overlap in functionality, I wonder if simply merging these two things would be nicer here. We could reserve So For the table, instead of enum VarKey {
Ident(Rc<str>),
Exp(Rc<str>),
}
type VarTable = HashMap<VarKey, Option<Constant>>;Where For example would result in Cycle {
vars: HashMap {
VarKey::Ident("kick") : None,
VarKey::Exp("repeat") : None,
VarKey::Ident("bass") : None,
VarKey::Ident("mult") : None,
VarKey::Exp("(step % 100) / 100") : None,
}
..
}So any string inside an For example you could use the above as return pattern {
unit = "1/1",
parameters = {
parameter.integer("repeat", 1, {1, 8})
},
event = cycle("kick*$repeat bass*mult kick bass*mult:v=${(step % 100) / 100}")
:vars(function(ctx)
return {
kick = "c4 #0",
bass = "c3 #1",
mult = ctx.parameter["repeat"] * 2,
}
end)
}(note, some helper functions for creating note constants in lua would be needed here and currently a variable in cycles cannot be compound like a note and targets like instrument) Additionally, the outer context could choose to try to assign So any invalid |
|
That would be pretty powerful, but I find it confusing when identifiers and expressions are mixed up in functionality. Then it's unclear what each one actually is and when to use it. In the above example, the same could be done with the regular map and expressions, but it's then more obvious that a name (identifier) resolves to some note (instrument) and that the expressions things are doing something fancy: return pattern {
unit = "1/1",
parameters = {
parameter.integer("repeat", 1, {1, 8})
},
event = cycle("kick*$repeat bass*${repeat * 2} kick bass*${repeat * 2}:v=${(step % 100) / 100}")
:map{
kick = "c4 #0",
bass = "c3 #1",
}
}Implementation wise, it also will be tough to apply mappings before generating the cycle. Also, it's unclear to me which kind of mappings would work and which ones not. Would this work? return pattern {
unit = "1/1",
event = cycle("a <b c>")
:with{
a = "[kick]*2",
b = "[kick g4 <a4 a5>]",
c = "[a4 g4]",
}
:map{
kick = "c4"
}
}Note Then one could split up complex cycles via identifiers, which is pretty cool and then definitely would be worth the extra complexity :) Apropos. How exactly would we resolve identifiers within In the example above ${(step % 100) / 100}Could scope the context variables here, ${(context.step % 100) / 100}then only Or we enforce using ${($some_parameter % 100) / 100}Which feels a bit ugly. |
|
While more flexible mappings of expressions and identifiers (or just parameters) would be great, let's finish the PR as it is, with simple evaluation of Should I take care of injecting the variables/parameters, or would you like to give this a try? |
Sure, but it would need you to repeat the same expression like in your example. This isn't a big deal in such a small example but if you want to reuse the same value in more places or define deeper relationships between different variables, pure repeated expressions become unwieldy and hard to experiment with.
Currently a variable can be any value from a pub enum Constant {
#[default]
Rest,
Hold,
Float(f64),
Integer(i32),
Pitch(Pitch),
Chord(Pitch, Rc<str>),
Target(Target),
Name(Rc<str>),
}This excludes Also, it would be very natural to use "a, b, c..." for this like your example, but these are reserved identifiers for notes. For simple substitution there could be a separate helper in Cycle that would return a string for you with the subcycles replaced but this wouldn't be dynamic, it would act like a macro. This could be easy to implement and doesn't have any of the caveats of variable subcycles, but it's also fairly rigid. cycle(replace("a a b <c d>", {
a = "bd bd*2",
b = "<[~ hh] [hh hh]>?0.2",
..
}))
True. I suppose the most flexible would be to have a sort of let block where you can specify the identifiers that you want which would be executed before resolving expressions and could decide what identifiers exists. This would make it so that you can use your own variables inside expression which I think is fairly important on the long run. So, the default could be for example bringing all context variables into the scope, then overriding with parameter names. Then you could further override this table yourself with something like cycle("...")
:let(function(context, vars)
-- vars is already a table with the defaults applied
-- override as you wish
vars["step"] = context.parameter["something_else"] * 4 * context.step
return vars
end)Personally I am all for reducing the stuff you need to write here, so useful defaults with the ability to bend it to your will feels right to me even if there are some subtle things that can happen like a parameter overriding a context variable. If you made a parameter "step" it is safe to assume you want to use that instead of "context.step" if you find yourself wanting to use both then you could simply rename the parameter, I think this is better price to pay in these cases than learning a new syntax or having to write |
Makes sense, guess it might be safe to decide that writing
That would be nice! |
Yes. Actually we can do that with simple Lua string manipulation hacks right now as well.
Make sense! I'd vote to call that
I vote for a clean separation between variables and names, to make them easier for users to understand, and easier for us to implement. What about:
I'll have a look at the parameter injection stuff tomorrow. |
|
As expected, injecting the vars was pretty straightforward. You may want to check that the parameter value and cycle value look OK ( |
Benchmark for beeb1b2Click to view benchmark
|
Benchmark for d9f0d38Click to view benchmark
|
|
Enum parameters just becoming a So here you could both create a kind of I made the Since parsing a string as a constant can fail (when calling |
Benchmark for 788efb9Click to view benchmark
|
I find that too vague and magic, and I would simply have kept those values as strings. It's not clear what for example, More complex parameter value conversions should IMHO be mapped manually later in those custom bash like expressions, e.g. |
"1/8" throws an error (currently discarded) since it cannot be parsed a single constant (we could add support for Other values will behave pretty much as if you wrote them into the cycle string. I think this is a really powerful and straightforward behaviour that lets you select from a list of values without any additional boilerplate or extra syntax. Even if it was possible via When needed, you can always use unique names to be able to remap them later (or just other variables where you convert the enum any way you want), but one of the key designs we agreed on is that parameter notation is for values to be used before generation happens to be able to affect the structure of the output. If we just cast to string here, then the usage becomes "parameters can affect the structure, except when they are enums, you'll have to revar/remap those, parse strings by hand, hit the right indices, implement conversion functions etc". I think we'd lose out on a very low hanging fruit for ergonomics.
What do you mean here? Such an enum would output notes in both types of enum conversion, wouldn't it? Unrelated but do the bindings for this dev branch need something special or they are just broken at the moment? Currently in Renoise using the |
|
As I mentioned before, I find that too clever - or not clever enough. For me, an enum parameter is a string and thus should be treated like one, or it should be a full cycle sub expression, but not something in-between. I don't think it's clear to users what can be magically auto-mapped and what not. For example: pattern {
parameter = {
parameter.enum("enum", "c4", {
"c4", -- works
"c5 c6", -- does not work and is not causing an error
"c5:v0.2", -- does not work and is not causing an error
"48", -- works
"bla", -- does not work and is causing an error as expected
"~", -- works
"!", -- does not work
} ),
},
event = cycle "g5 $enum"
}Also this has significantly slowed down the cycle parser. Not sure if that's worth it? Comparing f5a9cfa to latest locally here: I'd love to have sub cycle expressions here, but don't think it's worth the trouble to solve that problem partially now. What we have now already is a great addition as it is. Keeping things simple and solving problems step by step. As a compromise, number strings, as only exception, could be coerced to numbers though. Because that's what Lua does everywhere else too and is simple to define and explain. |
|
Note, the "not causing an error" cases above do cause an error, it's just discarded here currently when turning the Line 70 in db2b8fc Implementation-wise the only exception for single values is the
That would already be good as numbers are a big part of making this useful, although it does feel like a similar "solving the value injection problem halfway".
This is true for this entire PR, right? As we are introducing a new type of single value with |
|
It seems most of the performance drop was my silly mistake switching to eagerly evaluated error formatting. |
Benchmark for c092fe9Click to view benchmark
|
Benchmark for 441241bClick to view benchmark
|
Benchmark for 5a6dae0Click to view benchmark
|
Benchmark for 06a564fClick to view benchmark
|
|
I've made vars into subcycles with the caveat that a subcycle cannot itself contain variables. This restriction makes it easy to not have to deal with cyclic references between subcycles (for now), to make this explicit, the The exception around injecting individual repeats remain as this is a harder problem to solve. If this is enough to be considered for the automatic conversions of enum parameters then I'll write some more tests. Made it so that errors for parsing the enum aren't discarded, but not sure how to properly handle the errors in the lua context and the compiled cycle emitter. Could you take a look at these? Lines 388 to 393 in 875a6aa pattrns/src/emitter/scripted_cycle.rs Lines 298 to 320 in 875a6aa |
I wanted to make our life's easier, but this of course is super great to have and works great from what I've tested! This will especially be cool with the planed I think the panic in the non scripted cycle emitter is fine, as sub cycles are evaluated immediately when setting parameters. Alternatively I'll check how hard it is to return a result in
The note event struct layout changed in the dev branch. There's a new Line 133 in b807609 It should be compatible with the master version, but I haven't tested this. Alternatively you can build and run the playground locally:
Just noticed that a combination of newer rustcs with newer emscripten emccs no longer work here. Seems that in newer versions |
|
Fixed the playground builds in dev: 6e73330 So you may want to rebase, if needed... |
unlessgames
force-pushed
the
cycle-vars
branch
from
875a6aa to
eb8977c
Compare
Benchmark for d0e1a9bClick to view benchmark
|
…or dynamic lengths
Benchmark for 64566baClick to view benchmark
|
Benchmark for 424bc74Click to view benchmark
|
Benchmark for 111eb9aClick to view benchmark
|
|
I've made repeats also dynamic so they can be injected via variables as well. I also missed that dynamic weights and replicate somewhat broke polymeter expressions so these needed a refactor. Unfortunately all of this does come with some performance cost as the length of a section is no longer known at parse time so it has to be calculated while generating to correctly output polymeters. I suppose there is some opportunity to optimize here by caching but I'd do this in another go. If that's alright we could merge this now. |
- mention enum parameter name and value in sub cycle parameter error strings - forward parse errors as runtime error, but return a "rest" value on errors and continue parsing, so parse errors don't get masked by other errors - add some basic tests
|
I've tweaked the error handling a bit so it's a bit more clear where exactly the error happened. Else this looks and works great. Would be great to update the docs and tutorial as well, but we can add those later, when adding the 'var" function or as a separate PR as well. So let me know when you're done. I'll then squash this into dev. |
Benchmark for 8c9089bClick to view benchmark
|
Benchmark for 57972a9Click to view benchmark
|
|
Tried caching some stuff but it didn't matter much, on the other hand, I've tweaked some things unrelated to this PR that improved the performance a bit, this should be a more acceptable cost to pay here overall. With that, I am fine to merge this. I agree that the docs might be better updated when |
2 participants
A draft implementation of #95
ReplicateandWeightsimilar to other expressions in that they are being applied at runtime instead of while parsing. By relying on existing functionality, this also extended them so that now floats can be used on the right side (instead of the previous integer restriction) likea@1.5 b.Unfortunately, since we have to support the case without parameter like
a!anda ! ! !as a "repeat last" shorthand, it isn't possible to parse patterns on the right side of this without rewriting a huge part of parser to be a lot more stateful.Value, it can now can be eitherConstant(the struct previously calledValue) or aVariableorVariableTarget, these latter cases hold an identifier string that the cycle resolves based on anHashMap<Rc<str>, Constant>into aConstantwhen outputting events. This works for essentially any single value inside the cycle including targets using the$identifiernotation.a * $undefined_idNote, tests for cycles were moved to a separate file because rust-analyzer started to struggle a bit while editing.