tidalcycles / strudel

Web-based environment for live coding algorithmic patterns, incorporating a faithful port of TidalCycles to JavaScript
https://strudel.cc/
GNU Affero General Public License v3.0
586 stars 105 forks source link

Little fix for control.mjs + Proposed modification to scale() #977

Closed eefano closed 4 months ago

eefano commented 4 months ago

EDIT for clarity: This patch addresses two separate instances:

eefano commented 4 months ago

I have modified the scale() function also, to handle this case:

n("0 a").scale("c:major")

that was causing: expected hap.value to be an object, but got "a"

felixroos commented 4 months ago

Test case:

"a5".gain(0.5).note()

i don't understand what the regression is? this didn't work before https://github.com/tidalcycles/strudel/pull/973 and now it works. On the live site it still doesn't work because it has not been deployed yet, but it works locally.

n("0 a").scale("c:major")

Should this be a valid use case? I agree that the error is certainly confusing, but do you think the a should be treated as a note here?

eefano commented 4 months ago

I have scripts from months ago that stopped working correctly (and they're playing at half the speed they used to). One of them is this (the melody is not played): https://strudel.cc/?sG92VYvyKUjS

Regarding the scale() patch: without it is not possible to have control functions between the pattern and scale(). At the moment the situation is: "0 c#".scale("c:major").note() works "0 c#".pan(0).scale("c:major").note() doesn't work n("0 c#").pan(0).scale("c:major").note() plays c# (not panned) but not 0 n("0 c#").pan(0).scale("c:major") plays 0 (panned) but not c#

eefano commented 4 months ago

To be fair, i'm experiencing the problem on the live site only, not in the actual main branch, so my fix on control is not really relevant to the problem (but it still prevents the altering of the original hap by the delete statement).

eefano commented 4 months ago

Found the problematic commit: https://github.com/eefano/strudel/commit/5c71bb95a0ec1384b45a0716d9fb2963036dea3d When legato was "de-legacyfied", it followed the behaviour of the other control functions.

eefano commented 4 months ago

Ok with the latest modification, the object's value property is transformed by scale(), but only when n: is not already present. At least something like that will be allowed: "0 c#".pan(0).scale("c:major").note()

The n() variants will behave as usual.

felixroos commented 4 months ago

can you update the issue's top post to summarize what these changes are doing?