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

Horizontal Shift for Audio Clips #141

Merged

Conversation

topisani
Copy link
Contributor

@topisani topisani commented Jul 3, 2023

I have always wanted to be able to change the start point of audio clips, like the end can be changed using the red bar. The current "easy" way to do it is: reverse clip, change end, reverse back.

I first looked at implementing a similar green start bar (which would probably be the most intuitive), but i understand why rohan left it out now - it introduces some complexity with being able to scroll left of zero etc. I do have a partial implementation of some of this, and if i can find good, intuitive behavior for it, i might submit that as well.

But for now, i chose to instead implement horizontal shift for audio clips. This is the feature where ▲▼ is held while ◀▶ is turned. It works the same as for instrument clips, except it does not wrap around, only shift along the underlying audio, and it cannot go left of zero (will display a "CAN'T SHIFT PAST START" message)

@topisani topisani marked this pull request as draft July 3, 2023 18:09
@topisani topisani marked this pull request as draft July 3, 2023 18:09
@topisani topisani marked this pull request as draft July 3, 2023 18:09
@topisani topisani changed the title Horizontal Shift for Audio Clips [Draft] Horizontal Shift for Audio Clips Jul 3, 2023
@litui litui added the enhancement New feature or request label Jul 3, 2023
@sichtbeton
Copy link
Contributor

Wow, very cool! Would this work for loops or leave a gap at the start?

@topisani
Copy link
Contributor Author

topisani commented Jul 3, 2023

Wow, very cool! Would this work for loops or leave a gap at the start?

Not entirely sure actually, it depends on whether loops are recorded to a single file, or multiple. It will work the exact same way as moving the start point using the waveform editor or the reverse->length->reverse flow

@sichtbeton
Copy link
Contributor

Wow, very cool! Would this work for loops or leave a gap at the start?

Not entirely sure actually, it depends on whether loops are recorded to a single file, or multiple. It will work the exact same way as moving the start point using the waveform editor or the reverse->length->reverse flow

Nice!

@sichtbeton
Copy link
Contributor

alright couple of findings:

  • it works! finally we can shift an audio clip
  • if the audio ends but the clip itself is longer its silent (as expected)
  • would be cool if it wraps around (but you said its complicated)
  • if you shift all the way to the left (no audio anymore visible) it crashes
  • couldnt tell if it respects the margin feature yet

very cool indeed! thx for your work on this. a lot of people asked for this.

@topisani topisani force-pushed the feature/audio-clip-rotate branch 5 times, most recently from d53c604 to a12f7cf Compare July 3, 2023 20:39
@topisani
Copy link
Contributor Author

topisani commented Jul 3, 2023

@sichtbeton helped me test this on a 7seg unit. The latest changes fixed some edge cases. I believe this now works as intended.

  • Fixed shifting past the end of the sample
  • unassigning and reassigning the voice fixed some issues that happened when shifting during playback.
  • Fix shift direction so it is consistent with instrument clips
  • Fix shifting of automation so it is consistent with audio.

Currently, the automation will be wrapped around like in instrument. I think this behavior is acceptable, as the use case probably is not that important, and it is still consistent, usable behavior. Also, This feature will probably often be used to either trim dead space off the start, in which case the automation will be removed from the end afterwards, or to shift the whole clip to the next loop, in which case keeping the automation in sync might make sense. I am not completely sure how automation is currently handled when an audio clip is reversed and trimmed anyway, but i think this makes at least as much sense as that. Do tell me if you disagree, i can look at implementing another solution, but id prefer not to let this minor UI improvement propagate all the way through the paramManager etc.

@topisani topisani force-pushed the feature/audio-clip-rotate branch 2 times, most recently from 3378a27 to c88d4b8 Compare July 4, 2023 16:20
@topisani topisani marked this pull request as ready for review July 4, 2023 16:24
@topisani topisani changed the title [Draft] Horizontal Shift for Audio Clips Horizontal Shift for Audio Clips Jul 4, 2023
Copy link
Collaborator

@sapphire-arches sapphire-arches left a comment

Choose a reason for hiding this comment

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

I think style check is going to get mad (see below) but this mostly LGTM. Thanks!

CommunityFeatures.md Show resolved Hide resolved
int64_t newStartPos = int64_t(sampleHolder.startPos) - getSamplesFromTicks(amount);
uint64_t sampleLength = ((Sample*)sampleHolder.audioFile)->lengthInSamples;

if (newStartPos < 0 || newStartPos > sampleLength) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

startPos == sampleLength is OK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had the same thought, but the sample marker editor allows it, so i assume it is ok

if (newMarkerPos > waveformBasicNavigator.sample->lengthInSamples)
newMarkerPos = waveformBasicNavigator.sample->lengthInSamples;

src/deluge/model/clip/audio_clip.cpp Outdated Show resolved Hide resolved
@sichtbeton sichtbeton mentioned this pull request Jul 5, 2023
5 tasks
@topisani
Copy link
Contributor Author

topisani commented Jul 5, 2023

Addressed review comments, I have played around with this a bit more, and see no issues

@sapphire-arches
Copy link
Collaborator

this was probably broken by the 2 big merges recently actually, but other than maybe fixing some style troubles this LGTM

…alues

I don't actually know if the latter is needed, but the sample_marker_editor
does it when changing these values, so it seems reasonable to do as well.

I also removed the "CANT" message when shiftHorizontally fails, as i felt it
to be a bit redundant - usually it fails for trivial reasons, such as start
or end reached
@jamiefaye jamiefaye merged commit 2e1ba62 into SynthstromAudible:community Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants