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

Allow sample track TCOs to resize smaller than one bar #4933

Merged
merged 15 commits into from Apr 15, 2019

Conversation

Projects
None yet
5 participants
@Spekular
Copy link
Contributor

commented Apr 5, 2019

Currently, TCOs in sample tracks can't be resized to less than one bar, which is frustrating if short samples are loaded (they'll have useless empty space to the right). This PR allows sample tracks TCOs to be resized to less than one bar's size. Empty TCOs will still be created with a size of one bar.

At 100% zoom, it becomes difficult to see the TCO if it is at minimum size, but this already occurs with BB Track TCOs, so I wouldn't consider it a regression.

@Spekular

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

Looks like some weird unrelated stuff ended up here, will fix. EDIT: Have fixed.

Revert "Relabel"
This reverts commit bf61c25.
Show resolved Hide resolved src/tracks/SampleTrack.cpp Outdated
@BaraMGB

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

Do you really want to change this for stable?

@Spekular

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2019

I think you could argue the original behavior is a bug. I did intentionally target it for stable but if there are objections I could retarget to master.

@JohannesLorenz

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

The freshly loaded samples (doubleclick on the sample track TCOs) load with a correct length, but when you resize them to 1, you can't resize them back. In this situation, even when you load the same sample file again, the TCO size is still 1. Both can cause confusion and may be worth a fix.

As for the stable-1.2 question: I'm fine with merging it into stable-1.2 after the review will have approved this branch.

@Spekular

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2019

Hmm, I definitely tried scaling them up from zero and it seemed to work fine for me. How exactly do I reproduce the issue? Does the drag handle show up? Does it work when zoomed in further?

@JohannesLorenz

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

  1. Click on the sample track to create a yellow, empty TCO
  2. Double click on that TCO, open a file
  3. Move mouse on border of TCO, resize dialog appears
  4. Resize to 1
  5. Try to resize to <1 at any zoom, drag handle will show up, but you can't make it smaller than 1
@Spekular

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2019

@JohannesLorenz can't reproduce with those steps. Are you holding ctrl to fine adjust? Since it resizes in bar increments by default, 1 bar is the smallest it can go without fine adjusting.

The clip scales to the correct size when a sample is loaded for me too. However, if you attempt to reload the same sample, this doesn't happen. I think the call to setSampleFile might be skipped when loading the existing sample again? I'll look into that.

@Spekular

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2019

Reformatted for brevity, but here's what I think the cause is. Will fix.

if( af != "" && af != m_tco->m_sampleBuffer->audioFile() )
	{ m_tco->setSampleFile( af ); }
@Spekular

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2019

The sample track clip should now resize to the correct length no matter what sample is loaded (a new one or the same that it already contained).

@Spekular

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2019

There's currently a bug with cloning. A short (less than one bar) sample clip cannot be cloned to the right of itself unless another sample clip exists between it and the clone's destination.

In other words, a short sample at position A can be cloned to position C if B exists, but B cannot be cloned to position C.

A B [c]

@Spekular

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2019

A TrackContentWidget::dragEnterEvent is fired when a sample clip is Ctrl+dragged over or off of another sample clip. For a clip of 1 bar or more, we exit the clip's "airspace" directly into another bar than the original clip. For a short clip, we end up in the same bar.

tcoPos is apparently only accurate to one bar, so it looks like we're pasting to the same position when pasting a short clip to the right of itself. If the clip is dragged to another track and back to the original, or on top of anotoer clip in the same track and back, tcoPos is forced to update again, which can allow short clips to be cloned to their right.

@Spekular

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2019

Essentially short clips often evaluate true on tcoPos == grabbedTCOTact because tcoPos is only accurate to one bar and is only updated on some events. This returns false in canPasteSelection.

Spekular added some commits Apr 13, 2019

@Spekular

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2019

Previous issue is fixed. Credit to @JohannesLorenz

Allowing short sample clips exposes an edge case. Cloning a sample clip to a bar containing a short clip can completely cover the short clip. Whether or not a safeguard should be implemented for this is up for debate.

@JohannesLorenz

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

@Spekular I did a few tests. While everything worked fine it's a bit sad that you can't snap two short blocks together. Though I don't see that much use, as the sample TCO lengths/positions can't be set to an exact value anyways. I'd say this may be a nice feature, but it's absolutely not stopping this PR. If you want, you can try to add this to this PR, but I see it as totally optional.

@Spekular

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

@JohannesLorenz I'd rather leave it as is and then take a crack at #874 once this is merged.

@JohannesLorenz
Copy link
Contributor

left a comment

I'm through with the review. Feel free to fix what you like.

Show resolved Hide resolved src/tracks/SampleTrack.cpp Outdated
Show resolved Hide resolved src/tracks/SampleTrack.cpp Outdated
Show resolved Hide resolved src/tracks/SampleTrack.cpp Outdated
Show resolved Hide resolved src/tracks/SampleTrack.cpp
Show resolved Hide resolved src/tracks/SampleTrack.cpp
@Spekular

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

@JohannesLorenz fixed everything except the tick_t comment. I wouldn't know how to implement that one, the scope seems a bit big.

@JohannesLorenz

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

I'm OK with not fixing the tick_t. Can you please re-request review, so I can mark it as OK? I'm curious if that works 😄

Note, before merging, maybe it's good to squash those 10 commits.

Spekular added some commits Mar 12, 2019

Relabel
Allow sample track patterns to be less than one bar long

Reset size when reloading same file

Use existing sampleLength() method

Fix cloning issue

Style and comparison improvements

Don't include QDebug

Unrelated cosmetic fix
@Spekular

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

I don't have that option, perhaps because

Pull request authors can't request reviews unless they are either a repository owner or collaborator with write access to the repository.

I've done my best to squash as well, no idea if it worked.

@JohannesLorenz JohannesLorenz self-requested a review Apr 14, 2019

@JohannesLorenz

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

OK, It's accepted now, anyways. I think you did not squash it correctly: You squashed (a091d50) , but then merged (062041f) your unsquashed branch on top? I think you can solve it all by one simple git rebase -i 97d5529c18bee51d638607a192ab91b131d41086, marking all s, and no further merge.

@Spekular

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

When I try that, I immediately get a merge conflict :)

@Spekular

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

I ran into a brand new error on this squash attempt, aside from merge conflicts some commits couldn't be applied because ???, and I couldn't really tell how to proceed. Maybe whoever merges can do a squash and merge?

@PhysSong

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

@JohannesLorenz Using the Squash and merge feature will be perfectly fine in this case. You may want to edit the commit message to remove redundant messages though.

@JohannesLorenz JohannesLorenz merged commit 5a56969 into LMMS:stable-1.2 Apr 15, 2019

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.