Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix legato and duration #111

Closed
yaxu opened this issue May 8, 2022 · 17 comments · Fixed by #965
Closed

Fix legato and duration #111

yaxu opened this issue May 8, 2022 · 17 comments · Fixed by #965
Labels
compatibility Mainline Tidal Compatibility refactoring

Comments

@yaxu
Copy link
Member

yaxu commented May 8, 2022

The current legato implementation manipulates haps in a way that can lose data.

A fix would be to copy tidal's behaviour - store legato in a normal value key and then deal with it in the scheduler.

@felixroos
Copy link
Collaborator

Isn‘t this problem of loosing data also related to all other methods that use withHapTime? Iirc the problem was there is no way of knowing if an event part ever had an onset or not. But maybe there was another problem I don’t remember anymore. Generally, I like the possibility of controlling the hap length directly, because we then don’t need to react to that param for every output separately

@yaxu
Copy link
Member Author

yaxu commented May 10, 2022

It's not a problem with others that I know of, because where withHapTime is expanded, withQueryTime is reduced by the same factor (and vice-versa). The particular problem with this approach to legato is that we don't know by how much the query time should be expanded by.

We can still solve this once for everything though, as a simple pre-processing step that changes the duration of events before calling the callback. The only problem is that superdirt will do this a second time if it sees the legato parameter. So we need to remove it or reset it to 1 when the durations are changed.

@felixroos
Copy link
Collaborator

What are the consequences of not reducing withQueryTime by the same factor withHapTime is expanded? Can you think of an example problem this might cause? The thing with the pre processing step sounds like a good idea

@yaxu
Copy link
Member Author

yaxu commented Jun 28, 2022

The consequence with e.g. fast/slow would be doubling up or missing events.

The consequence in this case is events appearing or not depending on where you query:

A part from 0 > 2:

slowcat(1,silence).legato(2).queryArc(0,1)

Disappeared:

slowcat(1,silence).legato(2).queryArc(1,2)

This will result in glitches and inconsistent results, especially when it comes to combining it with other patterns.

We can't solve this by manipulating query time because we don't know how much to expand the query by.

@yaxu
Copy link
Member Author

yaxu commented Aug 11, 2022

The duration method has the same problem at the moment. This is kind of possible to fix for duration, as for query timespan (a,b), you could query (a-duration,b), to be sure of finding events that are expanded into the query timespan.. That would mess with the values returned from continuous events though.

@yaxu yaxu changed the title Fix legato Fix legato and duration Aug 15, 2022
@yaxu
Copy link
Member Author

yaxu commented Aug 15, 2022

I've fixed duration in 961a80d, but this breaks the 'jemblung' tune, as it has a pattern of the form "12".duration(0.05).add(12), that doesn't work when duration is promoted from metadata to data.

Perhaps it would be best to change all examples to use explicit parameter names for patterns, to make things clearer for beginners.

yaxu added a commit that referenced this issue Aug 15, 2022
@yaxu
Copy link
Member Author

yaxu commented Aug 16, 2022

Hm the tune sounds fine now I've merged from main. The test still fails, needs looking closely at the parts and wholes of the mismatching events to work out why..

Am I right that 'expected' is what we generate and 'received' is from the snapshot? If so, I think these should be swapped for clarity.

@felixroos
Copy link
Collaborator

are already running the tests with vitest ? it does expect(haps).toMatchSnapshot(); , where it should be flipped around. "expected" is whats in the tunes.test.mjs.snap file and "received" is what the code generates

@felixroos
Copy link
Collaborator

felixroos commented Aug 16, 2022

For reference: https://strudel.tidalcycles.org?iw5ossp4Sti1 the only effect duration has here is that the bass is short, it hasn't even an effect on the kick (although it might affect the event structure)

@yaxu
Copy link
Member Author

yaxu commented Aug 16, 2022

I see thanks, well maybe it's not important to understand the exact difference in haps, as long as it sounds the same!

@felixroos
Copy link
Collaborator

just hit an actual bug related using legato. For some reason, it does not create a scheduling error when the .s('triangle') is uncommented..

@yaxu
Copy link
Member Author

yaxu commented Jan 1, 2023

Legato causes missed events here:

s("bd*8").legato(0.5).rev()

I think we do need to turn legato into a normal param and deal with it in the scheduler

@yaxu yaxu added the compatibility Mainline Tidal Compatibility label Jan 16, 2023
@yaxu
Copy link
Member Author

yaxu commented Jan 16, 2023

Aside from the representation difference legato works a bit differently in superdirt for samples

legato 0.5 in haskell is .legato(0.5).clip(1) in strudel

Is clip redundant, i.e. could we just turn sample clipping on if legato is specified? I like the name 'clip' but it could just be an alias for legato then.

@felixroos
Copy link
Collaborator

yep clip(1) is what legato(1) does in tidal i think.. but clip takes just a flag atm, but it would make sense to make it behave as legato does. I think the migration of legato will be much easier when #288 is fixed first, because it allows using legato with plain values (also color and velocity)

@felixroos
Copy link
Collaborator

the nudge param falls into similar territory. It could make sense to handle these properties somewhere between pattern and output. right now they are in the pattern (illegal). They seem to be pretty global / output dependent so it feels a bit wrong to handle them at multiple places separately. so either they are handled automatically somewhere in the scheduler, or there is some function lets call it handlePostControls , that can be called from each output to handle those params.
The latter option seems cleaner to me, because the scheduler currently does not care about the hap.value , which would be nice if it stayed that way

@felixroos
Copy link
Collaborator

yep clip(1) is what legato(1) does in tidal i think.. but clip takes just a flag atm, but it would make sense to make it behave as legato does.

implemented in #598

@felixroos
Copy link
Collaborator

hello from the future, #965 should get rid of this mess once and for all :)

legato is now just a synonym of clip + duration is also just a control that sets the duration in cycles. The hap.duration getter returns the duration in cycles based on value.duration and value.clip.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility Mainline Tidal Compatibility refactoring
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants