Skip to content

Conversation

unlessgames
Copy link
Collaborator

refactoring Target

The PropertyKey stuff bugged me because it could express a lot of impossible values like an Index with something on the right, or a Name with Integer value and the fact that # name was treated as Index, so I simplified this as

Target {
  Index(i32),
  Named(Rc<str>, Option<f64>)
}

Cause this is actually what we have. Additionally, # name is parsed as Index instead of making it out of the parser as Named. If needed we can extend what can be on the right side of Named later, but using an f64 is enough for now imo.

assigning target patterns

Implemented assignment for targeting using =, to allow for easier typing targets like

[a b c d]:v=[.1 .2 .3 .4]

As per rules of expression chaining (ie no precedence rules, just nesting left to right), if one wants an expression for v here, the pattern need to be wrapped in [].

[a b c d]:v=[[.1 .2 .3 .4]/2]

Indices work without this ofc but you can still make it more verbose with #, ie

[a b c d]:[1 2 3 4]
same as
[a b c d]:#=[1 2 3 4]

With this, longer names can also be used (although these still need to be handled somehow outside of the cycle module):

[a b c d]:custom_param=<.2 .5 .8>

cropping

for target pattern overlapping (both the new assignment and the regular one) events should be cropped inclusively instead of dropping them when they start in the previous cycle, this required a somewhat ugly solution of carrying an overlap flag through the output functions, hopefully this doesn't matter much for performance. Below tried to illustrate the problem:

example

input      "[a b c d]:[[v.1 v.2 v.3 v.4]/3]"
cycle bounds   |1              |2              |3
step pattern   |a  |b  |c  |d  |a  |b  |c  |d  |a ...
target pattern |.1         |.2         |.3        |.4 
         default crop ------>  |
                               ^v.2^ 
                              a and b here need v.2 but it would be cropped out of the cycle

holds

Target patterns were not applying holds, this is fixed now, meaning [a b c d]:[1 _ _ 2] now correctly generates [a:1 b:1 c:1 d:2].

@unlessgames unlessgames requested a review from emuell May 16, 2025 16:25
Copy link

Benchmark for dedc219

Click to view benchmark
Test Base PR %
Cycle/Generate 48.4±0.39µs 48.0±0.73µs -0.83%
Cycle/Parse 366.7±5.37µs 363.8±4.45µs -0.79%
Rust Phrase/Clone 435.9±8.29ns 430.9±5.80ns -1.15%
Rust Phrase/Create 72.8±1.16µs 72.5±1.33µs -0.41%
Rust Phrase/Run 648.6±3.98µs 631.8±4.50µs -2.59%
Rust Phrase/Seek 145.2±271.53µs 144.6±268.84µs -0.41%
Scripted Phrase/Clone 611.2±10.16ns 615.3±6.65ns +0.67%
Scripted Phrase/Create 969.3±13.03µs 982.7±20.43µs +1.38%
Scripted Phrase/Run 1632.6±11.25µs 1632.8±7.80µs +0.01%
Scripted Phrase/Seek 202.7±405.07µs 211.4±426.98µs +4.29%

@emuell
Copy link
Member

emuell commented May 16, 2025

Great to see the target stuff simplified. The shorter target assignment process is very nice, too.

Should update the documentation here as well, otherwise no one will know that this exists?


How should invalid values with such assignments be handled:

As expected:

--this is an error, because `qwe` is not a valid property
return cycle("[c8]*4:[qwe 0.1 0.5]")
--just like this one
return cycle("[c8]*4:[x0.1]")

-- with the new assignment, this also causes an error as expected
return cycle("[c8]*4:v=[qwe]")

Unexpected?

-- this does interpret the second value as volume too.
return cycle("[c8]*4:v=[0.1 p0.2]")

I would expect this to either trigger an error, or that it overrides the default property key. So "v=" just sets the default property key.

@unlessgames
Copy link
Collaborator Author

Unexpected?

-- this does interpret the second value as volume too.
return cycle("[c8]*4:v=[0.1 p0.2]")

I would expect this to either trigger an error, or that it overrides the default property key. So "v=" just sets the default property key.

The reasoning here was that the right side does a cast to a float with whatever it gets (similarly to c*[2 p.5]), but I guess it might make sense to keep a target if you supplied such a thing.

I did that and added an explanation to the docs.

Copy link

Benchmark for dc33d5f

Click to view benchmark
Test Base PR %
Cycle/Generate 48.6±0.71µs 47.2±1.53µs -2.88%
Cycle/Parse 359.8±7.05µs 355.3±11.11µs -1.25%
Rust Phrase/Clone 430.5±8.88ns 425.6±11.24ns -1.14%
Rust Phrase/Create 71.1±1.69µs 70.4±2.04µs -0.98%
Rust Phrase/Run 619.2±11.84µs 630.4±13.38µs +1.81%
Rust Phrase/Seek 137.8±258.25µs 144.3±271.42µs +4.72%
Scripted Phrase/Clone 601.5±14.71ns 610.4±32.24ns +1.48%
Scripted Phrase/Create 943.5±26.45µs 967.3±58.28µs +2.52%
Scripted Phrase/Run 1620.3±15.86µs 1615.4±28.64µs -0.30%
Scripted Phrase/Seek 203.3±409.38µs 196.1±392.43µs -3.54%

Copy link
Member

@emuell emuell left a comment

Choose a reason for hiding this comment

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

Perfect. Works great!

@emuell emuell merged commit b706c26 into master May 19, 2025
2 checks passed
@emuell emuell deleted the cycle/target-assigns branch May 19, 2025 11:57
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.

2 participants