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

Beat and bassline bug #5673

Closed
AbaddonBent opened this issue Sep 8, 2020 · 11 comments
Closed

Beat and bassline bug #5673

AbaddonBent opened this issue Sep 8, 2020 · 11 comments
Labels

Comments

@AbaddonBent
Copy link

There's a bug where a instrument in beat and bassline has automation and the tracks above that instrument become hidden and un-editable.
image

I was making pitch adjustments when I noticed this

@AbaddonBent AbaddonBent added the bug label Sep 8, 2020
@thmueller64
Copy link
Contributor

Looking at your screenshot I assume you are using the 1.2.2 Appimage.
I unsuccessfully tried to reproduce the bug using the information in your screenshot with the following steps:

  1. Create a beat/bassline instance
  2. Create patterns inside the track with audio samples only (I created 16 as above)
  3. Automate the pitch of one of the last patterns

Can you reliably reproduce the bug? If so, could you please detail the steps to reproduce it? You could also share a project file.

@zonkmachine
Copy link
Member

I think this is mentioned somewhere else. A method to make empty Beat/Bassline tracks is to delete all beat bassline instances in the Song Editor. Existing tracks in the Beat/Bassline will then go blank. Sounds like you managed to do it in some other way.

@SecondFlight SecondFlight added the response required A response from OP is required or the issue is closed automatically within 14 days. label Sep 9, 2020
@AbaddonBent
Copy link
Author

I've been trying to duplicate it but nothing has been showing....
The file I had has changed and when I opened it, it has reset the beat and bassline, the screenshot is the only proof I've had, but I'll keep trying to see if it happens.
My theory is that it was a one time thing

@no-response no-response bot removed the response required A response from OP is required or the issue is closed automatically within 14 days. label Sep 20, 2020
@AbaddonBent
Copy link
Author

What I remembered what I did when this happened, I accidentally pressed the add sample track button. Then removed it and pressed the automation track and figured I'd mess with the pitch

@IanCaio
Copy link
Contributor

IanCaio commented Sep 20, 2020

I have experienced something similar by doing the following steps (I described it on my PR because at first I thought it could be related to it but then realized it was not):

  1. Open the default project (TripleOsc, SampleTrack, BBTrack and AutomationTrack).
  2. Create 2 TCOs on the TripleOsc track on Bars 1 and 2
  3. Ctrl+Drag the TripleOsc track to the BBEditor
  4. Add some notes to the Kicker track (not necessary for the bug to happen)
  5. Clone the BBTrack

Could it be you did something similar, like dragging an instrument that already had TCOs to the BB Editor before cloning? I'm still not sure what causes this bug, but could anyone check if they can reproduce it by following those steps?

@IanCaio
Copy link
Contributor

IanCaio commented Sep 20, 2020

Afaik, all BB Tracks share the same tracks, and what differs a BB pattern from another is the TCO being played/visualized. Something like this:

          BBTrack1        BBTrack2       BBTrack3
Kicker: === TCO 1 === | === TCO 2 === | === TCO 3 ===
Snare:  === TCO 1 === | === TCO 2 === | === TCO 2 ===

Trying to do some debugging I found out that the Kicker track on the default project had 2 TCOs instead of 1, which I'd expect since the default project only has 1 BBTrack at first. I think that could be related to the bug, but that's as far as I went into investigating it.

IanCaio added a commit to IanCaio/lmms that referenced this issue Oct 4, 2020
	Classes that inherit from "Track", also inherit the createTCO method. That method takes a MidiTime position as an argument, but except on the class SampleTrack that argument was ignored. That lead to unnecessary calls to TCO->movePosition after creating a TCO in many parts of the codebase (making the argument completely redundant) and even to a bug on the BBEditor, caused by a call to createTCO that expected the position to be set on the constructor (see issue LMMS#5673).

	That PR adds code to move the TCO to the appropriate position inside the constructor of the classes that didn't have it, fixes the code style on the SampleTrack createTCO method and removes the now unneeded calls to movePosition from source files on src/ and plugins/. On Track::loadSettings there was a call to saveJournallingState(false) followed immediately by restoreJournallingState() which was deleted because it's redundant (probably a left over from copying the code from pasteSelection?).
@IanCaio
Copy link
Contributor

IanCaio commented Oct 4, 2020

Could everyone that managed to experience this bug try to reproduce it on the build from PR #5699 ? I think I accidentally found the reason behind the bug and fixed it there. The method that creates TCOs expects a position as an argument but in many classes it didn't actually place the TCO on the position given. That went unnoticed everywhere else because every time a TCO was created there was an explicit call to movePosition right after it, but on the method getTCO there wasn't one, so the TCO was expected to be created in a position but wasn't. That's probably why they are all missing in the BBEditor, they are misplaced.

That PR should fix it but it needs testing.

@AbaddonBent
Copy link
Author

I got the issue again, this time I recorded it!
I don't know what happened but as soon as I added another track that uses the same instruments and up, the original sampled instrument and up is gone, meaning that the beat and bassline can't see it! on those tracks, I right clicked and edited them so my thought is that when you have an instrument and it uses custom edits such as odd timing, it'll bug out and appear like nothing was there.
here's the video
got em.zip

here's the project file
grim theme.zip

I had to zip them because github don't allow mp4/blend files

@AbaddonBent
Copy link
Author

I reloaded the file and it automatically solved it?

Spekular pushed a commit that referenced this issue Nov 7, 2020
* Fixes createTCO method on some classes

	Classes that inherit from "Track", also inherit the createTCO method. That method takes a MidiTime position as an argument, but except on the class SampleTrack that argument was ignored. That lead to unnecessary calls to TCO->movePosition after creating a TCO in many parts of the codebase (making the argument completely redundant) and even to a bug on the BBEditor, caused by a call to createTCO that expected the position to be set on the constructor (see issue #5673).

	That PR adds code to move the TCO to the appropriate position inside the constructor of the classes that didn't have it, fixes the code style on the SampleTrack createTCO method and removes the now unneeded calls to movePosition from source files on src/ and plugins/. On Track::loadSettings there was a call to saveJournallingState(false) followed immediately by restoreJournallingState() which was deleted because it's redundant (probably a left over from copying the code from pasteSelection?).

* Fix code style issues

	Fixes code style issues on some files (except for ones where the current statements already had a different code style. In those the used code style was kept for consistency).

* Fixes more code style issues

* Fixes code style issues on the parameter name

	Fixes code style issue on the parameter name of createTCO, where _pos was supposed to be just pos. The existing code had the old style and I ended up replicating it on the other methods.

* Code style fixes

	Fixes code style in the changed lines.

* Fixes bug with dragging to negative positions

	There was a bug (already present before this PR) where dragging
a selection before MidiTime 0 would result in some TCOs being placed on
negative positions. This PR fixes this bug by applying the following
changes:

	1) TrackContentObject::movePosition now moves the TCO to
positions equal or above 0 only.
	2) Because of the previous change, I removed the line that
calculated the max value between 0 and the new position on
TrackContentObjectView::mouseMoveEvent when dragging a single TCO (and
added a line updating the value to the real new position of the TCO so
the label displays the position correctly).
	3) Unrelated to this bug, but removed an unnecessary call to
TrackContentWidget::changePosition on the left resize of sample TCOs
because it will already be called when movePosition triggers the
positionChanged signal.
	4) Added some logic to the TrackContentWidget::pasteSelection
method to find the left most TCO being pasted and make sure that the
offset is corrected so it doesn't end up on a negative position (similar
to the logic for the MoveSelection action).
	5) Removed another line that calculated the max between 0 and
the new position on Track::removeBar since it's now safe to call
movePosition with negative values.

* Uses std::max instead of a conditional statement

	As suggested by Spekular, we use std::max instead of a
conditional statement to correct the value of offset if it positions a
TCO on a negative position.
@Spekular
Copy link
Member

Spekular commented Nov 7, 2020

@IanCaio should this be closed now that #5699 is merged?

@IanCaio
Copy link
Contributor

IanCaio commented Nov 8, 2020

@Spekular I believe so, the only steps I had to reliably reproduce this bug now work properly with the createTCO method fixes, so I think misplacement of the TCOs on the BBTrack was the root cause. Closing it then! 😃

@IanCaio IanCaio closed this as completed Nov 8, 2020
IanCaio added a commit to IanCaio/lmms that referenced this issue Mar 28, 2021
* Fixes createTCO method on some classes

	Classes that inherit from "Track", also inherit the createTCO method. That method takes a MidiTime position as an argument, but except on the class SampleTrack that argument was ignored. That lead to unnecessary calls to TCO->movePosition after creating a TCO in many parts of the codebase (making the argument completely redundant) and even to a bug on the BBEditor, caused by a call to createTCO that expected the position to be set on the constructor (see issue LMMS#5673).

	That PR adds code to move the TCO to the appropriate position inside the constructor of the classes that didn't have it, fixes the code style on the SampleTrack createTCO method and removes the now unneeded calls to movePosition from source files on src/ and plugins/. On Track::loadSettings there was a call to saveJournallingState(false) followed immediately by restoreJournallingState() which was deleted because it's redundant (probably a left over from copying the code from pasteSelection?).

* Fix code style issues

	Fixes code style issues on some files (except for ones where the current statements already had a different code style. In those the used code style was kept for consistency).

* Fixes more code style issues

* Fixes code style issues on the parameter name

	Fixes code style issue on the parameter name of createTCO, where _pos was supposed to be just pos. The existing code had the old style and I ended up replicating it on the other methods.

* Code style fixes

	Fixes code style in the changed lines.

* Fixes bug with dragging to negative positions

	There was a bug (already present before this PR) where dragging
a selection before MidiTime 0 would result in some TCOs being placed on
negative positions. This PR fixes this bug by applying the following
changes:

	1) TrackContentObject::movePosition now moves the TCO to
positions equal or above 0 only.
	2) Because of the previous change, I removed the line that
calculated the max value between 0 and the new position on
TrackContentObjectView::mouseMoveEvent when dragging a single TCO (and
added a line updating the value to the real new position of the TCO so
the label displays the position correctly).
	3) Unrelated to this bug, but removed an unnecessary call to
TrackContentWidget::changePosition on the left resize of sample TCOs
because it will already be called when movePosition triggers the
positionChanged signal.
	4) Added some logic to the TrackContentWidget::pasteSelection
method to find the left most TCO being pasted and make sure that the
offset is corrected so it doesn't end up on a negative position (similar
to the logic for the MoveSelection action).
	5) Removed another line that calculated the max between 0 and
the new position on Track::removeBar since it's now safe to call
movePosition with negative values.

* Uses std::max instead of a conditional statement

	As suggested by Spekular, we use std::max instead of a
conditional statement to correct the value of offset if it positions a
TCO on a negative position.
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this issue Jun 28, 2022
* Fixes createTCO method on some classes

	Classes that inherit from "Track", also inherit the createTCO method. That method takes a MidiTime position as an argument, but except on the class SampleTrack that argument was ignored. That lead to unnecessary calls to TCO->movePosition after creating a TCO in many parts of the codebase (making the argument completely redundant) and even to a bug on the BBEditor, caused by a call to createTCO that expected the position to be set on the constructor (see issue LMMS#5673).

	That PR adds code to move the TCO to the appropriate position inside the constructor of the classes that didn't have it, fixes the code style on the SampleTrack createTCO method and removes the now unneeded calls to movePosition from source files on src/ and plugins/. On Track::loadSettings there was a call to saveJournallingState(false) followed immediately by restoreJournallingState() which was deleted because it's redundant (probably a left over from copying the code from pasteSelection?).

* Fix code style issues

	Fixes code style issues on some files (except for ones where the current statements already had a different code style. In those the used code style was kept for consistency).

* Fixes more code style issues

* Fixes code style issues on the parameter name

	Fixes code style issue on the parameter name of createTCO, where _pos was supposed to be just pos. The existing code had the old style and I ended up replicating it on the other methods.

* Code style fixes

	Fixes code style in the changed lines.

* Fixes bug with dragging to negative positions

	There was a bug (already present before this PR) where dragging
a selection before MidiTime 0 would result in some TCOs being placed on
negative positions. This PR fixes this bug by applying the following
changes:

	1) TrackContentObject::movePosition now moves the TCO to
positions equal or above 0 only.
	2) Because of the previous change, I removed the line that
calculated the max value between 0 and the new position on
TrackContentObjectView::mouseMoveEvent when dragging a single TCO (and
added a line updating the value to the real new position of the TCO so
the label displays the position correctly).
	3) Unrelated to this bug, but removed an unnecessary call to
TrackContentWidget::changePosition on the left resize of sample TCOs
because it will already be called when movePosition triggers the
positionChanged signal.
	4) Added some logic to the TrackContentWidget::pasteSelection
method to find the left most TCO being pasted and make sure that the
offset is corrected so it doesn't end up on a negative position (similar
to the logic for the MoveSelection action).
	5) Removed another line that calculated the max between 0 and
the new position on Track::removeBar since it's now safe to call
movePosition with negative values.

* Uses std::max instead of a conditional statement

	As suggested by Spekular, we use std::max instead of a
conditional statement to correct the value of offset if it positions a
TCO on a negative position.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants