Instrument-track measure ticks sometimes off by 1px

[enp2s0]

In the song-editor, the ticks on each measure for the automation track are sometimes shifted forward by 1px.

Reproduce:
Open new project.
Make an instrument-track and an automation-track.
Put some random stuff into the tracks, about 8-16 measures is good.
Look at the ticks on the tracks, some will not line up!

[zonkmachine]

It looks like it's the Instrument track that is a bit off. The Automation and BBTracks align with the time line.

[enp2s0]

Interestingly on my system it appears as if the ticks "fall behind" on the Instrument tracks. The first few ticks line up but get worse as the track goes on.

Haven't looked at the code but I assume the ticks are drawn like this: Draw tick, move forward by X pixels, draw tick, ...

If this is the case then it appears that the value "X" being added is off by a very small amount.

[enp2s0]

BTW, I'm using LMMS 1.2.0-rc5, Qt 5.9.2, GCC 4.8.4 AppImage build.

If someone could point me to the correct file where this is done I'd be happy to look into it, but I can't seem to find the code

[zonkmachine]

It happened in commit 8c2ebf2

8c2ebf287d9de57466ffa8b1f4bda184c171dde8 is the first bad commit
commit 8c2ebf287d9de57466ffa8b1f4bda184c171dde8
Author: Dave French <dave.french3@googlemail.com>
Date:   Fri Dec 26 12:33:00 2014 +0000

    Proposed fix for 1255 Segment/clip not showing whole last note in Song Editor

Changing https://github.com/LMMS/lmms/blob/stable-1.2/src/tracks/Pattern.cpp#L887 to
( width() - TCO_BORDER_WIDTH ) seem to help a bit. The ticks still look a bit off but it's the same for all tracks. @noahb01 If you want to poke this one a bit that's fine with me. I just get the feeling this could be a frustrating job of pushing pixels around. You've been warned.

[PhysSong]

@zonkmachine ( width() - 1 ) looks better. See this:

lmms/src/core/Track.cpp

Lines 458 to 461 in c3b4767

	setFixedWidth(
	static_cast<int>( m_tco->length() * pixelsPerTact() /
	MidiTime::ticksPerTact() ) + 1 /*+
	TCO_BORDER_WIDTH * 2-1*/ );

And I think x_base + static_cast<int>( ppt * t ) - 1 should be x_base + static_cast<int>( ppt * t ) - 2 or static_cast<int>( ppt * t ) here for consistency with BB track and/or automation track. After applying two fix, everything should be fixed.

FYI, reverting 8c2ebf2 doesn't seem to cause any regressions(#1255 or other things) on master.

[enp2s0]

@zonkmachine Only the ( width() - TCO_BORDER_WIDTH ) (https://github.com/LMMS/lmms/blob/stable-1.2/src/tracks/Pattern.cpp#L887) change was required to fix the issue for me. @PhysSong what is the effect of the core/Track.cpp change? Is it only for consistency or does it have an effect on the output?

Also, why would we use 1 instead of TCO_BORDER_WIDTH? I assume that the border is not always 1px, especially on high res/HiDPi displays?

Edit(s) typos, rewording for clarification
Edit2 Nevermind, downloading the latest master branch causes the tracks to get out of alignment again.

[enp2s0]

Also it should be noted that the code for this in master and stable-1.2 is quite different...

[musikBear]

imo a 1.3+ 'issue'
noone should use time on this now -just saying
Currently there are two bug-categories: Bug & Critical. imo a category Minor could be used for things like this.

[tresf]

@musikBear is not a project maintainer. @PhysSong, myself and @zonkmachine are.

@musikBear Please leave this decision to the maintainers.

[musikBear]

@tresf How about not going after the man, but the ball?
I make a totally legit remark and suggestion, that will be beneficial, so what about making an argument instead of a Donald Trump style snuffof !

[tresf]

so what about making an argument

@musikBear I'm not a technical reviewer for this bug so I am leaving the decision up to those that understand the codebase well enough to milestone it.

I make a totally legit remark and suggestion, that will be beneficial,

It's not beneficial. I've received multiple complaints from developers that read a comment of yours and believe it reflects on the project. I'm letting @noahb01 know that your thoughts do not necessarily reflect the project as a whole. The team spends a lot of time supporting the users as well as the new developers of the project on several channels and mediums (FB, email, Discord, GitHub) and that can cause a delay when someone like @noahb01 asks a simple question: "What branch to target?". Having worked with both @PhysSong and @zonkmachine on hundreds of issues, I'm fairly confident they will come back with a well thought reply that answers the question directly.

What I'm not confident about is how your comments will be received, so I'm offering insight for @noahb01 as well as future bug submitters.

So when you say "go after the ball", I have to reply with, I do. I'm working on some of my own tasks with the project currently. I'd be happy to offer supporting evidence of this offline if you're interested. You know how to contact me. :)

[zonkmachine]

Also it should be noted that the code for this in master and stable-1.2 is quite different...

As @PhysSong dived deeper into this he should probably be the one to answer. My first impression though is that it would be OK to fix stable-1.2 with a one-liner and leave the rest for master. With 'the rest' here being the pattern shooting over to the right.

I don't currently build master so I can't chip in there.

.
<ot>

I make a totally legit remark and suggestion, that will be beneficial,

This discussion should probably go somewhere else but I don't find it beneficial to have people not involved in the coding trying to organize the workflow of it. Please leave issue labels and milestones to the coders.

[enp2s0]

I'm not closing this as the issue is still present in master, but it has been fixed in stable-1.2 in PR #4171.

[zonkmachine]

@zonkmachine ( width() - 1 ) looks better. See this:

I didn't get that, sorry. I tried #4171 now and it works. I think we can merge it now for rc6 and you can refactor the code later if needed.

[Umcaruje]

Closed in #4171