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

Remove explicit time calculations from Song and TimeLineWidget #3701

Merged

Conversation

michaelgregorius
Copy link
Contributor

Removes some explicit time calculations from Song and TimeLineWidget as part of #3284. See this comment.

@PhysSong
Copy link
Member

Good. Could you also fix two more places where * 60 * 1000 / 48 appears? They are in Song.cpp and you can use MidiTime::ticksToMilliseconds().

@PhysSong
Copy link
Member

Coding conventions looks inconsistent a little bit but isn't wrong.

Add a method to convert a MidiTime instance to milliseconds. Also add a
static method to MidiTime that computes the time in milliseconds for a
given number of ticks and a tempo.

Remove the method that sets the milliseconds explicitly from Song.
Replace it by a method that takes a MidiTime instance and one method
that takes an amount of ticks.

Remove several explicit time calculations from the implementation of
Song and TimeLineWidget. Instead use the new methods.
@michaelgregorius
Copy link
Contributor Author

@PhysSong Good catch! I had also checked the file for other occurrences (searching for "48" but somehow it did not occur to me that I can replace two more calls by using on of the methods that I had just added. 😄

I have removed two more calls and squashed the two commits into one.

@PhysSong
Copy link
Member

@michaelgregorius You don't need to squash commits manually. They can be squashed when PR is merged.

@zonkmachine
Copy link
Member

@PhysSong The main reason for squashing the commits yourself, the way I see it, is to have control of the final commit message. If someone else does it for you, which is a risk at places like this, they may just press merge and all the commit messages will be automatically stacked one after the other.

@PhysSong
Copy link
Member

I'm going to fix #2112. When I finish my work, I'll make a pull request against stable-1.2 and my PR needs this fix. Is it okay to rebase to stable-1.2 and change the base branch?

@tresf
Copy link
Member

tresf commented Jul 17, 2017

I'll make a pull request against stable-1.2 and my PR needs this fix. Is it okay to rebase to stable-1.2 and change the base branch?

If this bug is not new to 1.2, it shouldn't be targeted at 1.2. We are trying to freeze the codebase.

@michaelgregorius
Copy link
Contributor Author

Are you sure that the fix for #2112 should be targeted for 1.2? It's not in the milestones for 1.2 and at a certain point we should really get finished with 1.2. By the way, even if this fix goes into master first we can always cherry-pick it for 1.2.

@tresf What do you think?

@michaelgregorius
Copy link
Contributor Author

@tresf Seems like we posted at the same time. 😄

@PhysSong
Copy link
Member

Though it isn't a new bug and changes the codebase, it doesn't make something unstable or change the codebase a lot. Moreover, it is a bug fix.

@tresf
Copy link
Member

tresf commented Jul 17, 2017

I still don't see a compelling reason to merge this into stable. This code is called all over the place, I'd rather not change it at the RC4 state so close to release.

@PhysSong
Copy link
Member

Okay. Go to master.

inline int getMilliseconds() const
{
return m_elapsedMilliSeconds;
}
inline void setMilliSeconds( float ellapsedMilliSeconds )
Copy link
Member

Choose a reason for hiding this comment

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

I prefer leaving setMilliSeconds() to removing, but it's not bad. We can add this again when needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PhysSong Yes, we can always add it back if needed. However, for now it look like the milliseconds are rather an internal representation of the Song class and that other client "think" in different times, e.g. MidiTime.

@michaelgregorius michaelgregorius merged commit e05c827 into LMMS:master Jul 18, 2017
@michaelgregorius michaelgregorius deleted the 3284-RemoveExplicitCalculations branch July 18, 2017 19:26
@karmux
Copy link
Contributor

karmux commented Jul 19, 2017

Time display is not changing during playing anymore with this change. Time only changes when you click somewhere on timeline but not during playing.

@zonkmachine
Copy link
Member

I see this too. I've compiled both with and without debug flags.

@PhysSong
Copy link
Member

Time display is not changing during playing anymore with this change.

I haven't tested yet, but I will fix it if I can reproduce.

@zonkmachine
Copy link
Member

I haven't tested yet, but I will fix it if I can reproduce.

I can't answer for @michaelgregorius but why not leave him some time to respond? I'm pretty sure he knows what's up with this one already.

@PhysSong
Copy link
Member

@zonkmachine I think @michaelgregorius can fix it. However, I haven't seen this after my work for #2112.
If @michaelgregorius can fix it soon, I'll let him do.

m_elapsedMilliSeconds +=
( ( framesToPlay / framesPerTick ) * 60 * 1000 / 48 )
/ getTempo();
m_elapsedMilliSeconds += MidiTime::ticksToMilliseconds( framesToPlay / framesPerTick, getTempo());
Copy link
Member

@PhysSong PhysSong Jul 20, 2017

Choose a reason for hiding this comment

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

I think this line caused bug. framesToPlay / framesPerTick isn't an integer and smaller than 1.
Possible fix is: change
double MidiTime::ticksToMilliseconds(tick_t ticks, bpm_t beatsPerMinute) to
double MidiTime::ticksToMilliseconds(double ticks, bpm_t beatsPerMinute).

Copy link
Member

Choose a reason for hiding this comment

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

Or adding MidiTime::framesToMillisecond(frame_t ticks, const float framesPerTick, bpm_t beatsPerMiute)?

PhysSong added a commit to PhysSong/lmms that referenced this pull request Jul 20, 2017
@PhysSong PhysSong mentioned this pull request Jul 20, 2017
@michaelgregorius
Copy link
Contributor Author

Problem is fixed with commit bcdb5ec. I have decided to add a second version of MidiTime::ticksToMilliseconds which takes a double as the argument for the ticks.

Thanks for the hint, @PhysSong!

@zonkmachine
Copy link
Member

Tested, fixed!

The somewhat spooky thing is that I actually started a review and the action around the timeline was the only thing I got around to test and still I didn't spot it... :/

@michaelgregorius
Copy link
Contributor Author

@zonkmachine Thanks for testing! Seems to have been quite some elusive bug then. I think I only tested whether the play head moves or not.

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

Successfully merging this pull request may close these issues.

None yet

5 participants