Skip to content

Conversation

emuell
Copy link
Member

@emuell emuell commented Mar 5, 2025

  • support subdivisions or alternating parameters as targets
  • parse op_replicate , op_weight and op_degrade as number value only

Follow up of #36

- support subdivisions or alternating parameters as target
- parse op_replicate , op_weight and op_degrade as number value only
@emuell emuell requested a review from unlessgames March 5, 2025 15:13
@emuell
Copy link
Member Author

emuell commented Mar 5, 2025

@unlessgames actually wanted to work on #31 but then noticed that this isn't supported yet, and should be cleared first.

The `impl From<&Rc> for Target' is pretty ugly, but I didn't want to change the pest rules too much and sort it out there. This at least fixes some other, related workarounds, but maybe you have a better idea here...

Copy link

github-actions bot commented Mar 5, 2025

Benchmark for 862c8b5

Click to view benchmark
Test Base PR %
Cycle/Generate 58.5±0.92µs 68.7±2.18µs +17.44%
Cycle/Parse 316.4±6.01µs 329.9±9.17µs +4.27%
Rust Phrase/Clone 443.2±3.21ns 433.1±7.76ns -2.28%
Rust Phrase/Create 65.5±1.03µs 65.7±1.61µs +0.31%
Rust Phrase/Run 678.3±6.42µs 704.2±36.33µs +3.82%
Rust Phrase/Seek 160.0±302.85µs 159.8±301.31µs -0.13%
Scripted Phrase/Clone 632.8±5.05ns 647.6±22.83ns +2.34%
Scripted Phrase/Create 951.6±10.51µs 942.4±10.86µs -0.97%
Scripted Phrase/Run 1827.0±37.32µs 1805.3±18.66µs -1.19%
Scripted Phrase/Seek 232.6±474.20µs 230.9±467.70µs -0.73%

@unlessgames
Copy link
Collaborator

Not sure if this gives the right behaviour, an expression like

[a b]:[1 2 3]

results in

  │00│ 0 -> 0.333 | Pitch(Pitch { note: 9, octave: 4 }) Index(1)
  │01│ 0.5 -> 0.666 | Pitch(Pitch { note: 11, octave: 4 }) Index(2)

Where the structure of the pattern changes according to the targets' pattern, this is expected for cases where the right side multiplies the left side (ie it affects the structure) but in this it should only affect the targets, not the structure maybe?

See a strudel example for something similar:

https://strudel.cc/#JDogcygiW2phenogamF6el06WzEgMiAzXSIpLl9wdW5jaGNhcmQoKQ%3D%3D

I'd expect the output to be

  │00│ 0 -> 0.5 | Pitch(Pitch { note: 9, octave: 4 }) Index(1)
  │01│ 0.5 -> 1.0 | Pitch(Pitch { note: 11, octave: 4 }) Index(2)

@emuell
Copy link
Member Author

emuell commented Mar 5, 2025

When Tidal does this, people will expect it here too. Wasn't ware of this. I wouldn't have started this if I knew that.

Do you think it's possible to add such a behavior on top of this PR? Else decide you please if this PR should be skipped, extended or ...

@unlessgames
Copy link
Collaborator

I'll look into it. I think the application of targets has to be done separately since even though the structure is not applied the way it is with multiplication, it still needs to be applied to create stacks: if the right side has them, the output will get stacked as well.

https://strudel.cc/#JDogcygiW2phenogamF6el06WzEgMywgNCA1XSIpLl9wdW5jaGNhcmQoKQ%3D%3D

@unlessgames
Copy link
Collaborator

Here I made it behave as it should.

While this works, it's definitely not optimal and I feel we are accumulating more tech debt here, there should be some generialization possibility as well..

Copy link

github-actions bot commented Mar 7, 2025

Benchmark for 9b1729b

Click to view benchmark
Test Base PR %
Cycle/Generate 58.1±0.76µs 59.1±0.79µs +1.72%
Cycle/Parse 322.4±5.89µs 321.9±5.54µs -0.16%
Rust Phrase/Clone 438.1±11.13ns 434.1±6.64ns -0.91%
Rust Phrase/Create 65.7±1.09µs 65.9±1.07µs +0.30%
Rust Phrase/Run 689.0±6.09µs 675.3±7.70µs -1.99%
Rust Phrase/Seek 159.7±301.47µs 159.0±299.49µs -0.44%
Scripted Phrase/Clone 630.0±22.18ns 627.4±10.40ns -0.41%
Scripted Phrase/Create 944.2±18.31µs 937.7±10.94µs -0.69%
Scripted Phrase/Run 1806.9±8.99µs 1790.2±32.65µs -0.92%
Scripted Phrase/Seek 218.9±445.64µs 230.5±466.83µs +5.30%

@emuell
Copy link
Member Author

emuell commented Mar 8, 2025

Thanks. I'll give this a good test later.

It's true that things likely only gets more complicated from here on, but how do you think it would be possible to streamline things here? I guess the main problem here is the emulation of a functional language here.

Or do you think we should simply stop here and avoid adding all features from tidal here?

@unlessgames
Copy link
Collaborator

unlessgames commented Mar 8, 2025

Have to think about streamlining but for one, I think we need more generalized methods to combine different event streams together like I mentioned here.

Don't think it's much about the language itself. Rust is powerful enough even if more verbose for some things.

That said, there isn't too much left to do in terms of mini-notation anyway. Improving the afseq specific target features to include volume and so on might be the kind of priority here that could bring the largest benefits:

For example, patterning bjorklund parameters can be mimicked with a bit of duplication, or chained expressions can be fixed by forcing the evaluation order using brackets like [ [a b] / 2 ] * 3 but we can't attach volumes or other parameters to notes even though that might be quite useful.

Otherwise, the rest of tidal mainly happens outside the mini-notation where you can combine different patterns together with helper functions. Allowing some of these combination techniques in afseq would be the responsibility of the outer context I guess, not this mini-notation part, because ideally one could utilize such helpers across regular afseq rhythms and cycles.

@emuell
Copy link
Member Author

emuell commented Mar 9, 2025

That said, there isn't too much left to do in terms of mini-notation anyway. Improving the afseq specific target features to include volume and so on might be the kind of priority here that could bring the largest benefits

Agreed. That's why I started this PR in the first place. Thanks for taking care of it. Works as promised now, so let me merge this...

Otherwise, the rest of tidal mainly happens outside the mini-notation where you can combine different patterns together with helper functions.

Yes, let continue discussing this in a new issue or on the forums...

@emuell emuell merged commit f572132 into master Mar 9, 2025
2 checks passed
@emuell emuell deleted the feature/dynamic-cycle-targets branch March 9, 2025 14:18
Copy link

github-actions bot commented Mar 9, 2025

Benchmark for d153cbd

Click to view benchmark
Test Base PR %
Cycle/Generate 58.9±1.15µs 58.8±0.69µs -0.17%
Cycle/Parse 319.5±4.08µs 317.7±4.45µs -0.56%
Rust Phrase/Clone 437.1±23.28ns 435.5±6.08ns -0.37%
Rust Phrase/Create 65.8±2.73µs 64.7±1.08µs -1.67%
Rust Phrase/Run 675.3±6.48µs 671.7±5.16µs -0.53%
Rust Phrase/Seek 159.2±301.81µs 163.5±310.90µs +2.70%
Scripted Phrase/Clone 627.9±7.67ns 630.4±17.59ns +0.40%
Scripted Phrase/Create 943.0±9.52µs 942.0±31.80µs -0.11%
Scripted Phrase/Run 1781.6±14.22µs 1799.4±9.29µs +1.00%
Scripted Phrase/Seek 230.6±468.43µs 231.4±470.75µs +0.35%

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