danomatika / ofxMidi

(maintained) Midi addon for openFrameworks
Other
262 stars 72 forks source link

Miliseconds? #67

Closed alex-calamar closed 5 years ago

alex-calamar commented 5 years ago

Good evening... Sorry, maybe I'm wrong or maybe this issue has been already discussed around here (I couldn't find it, if so)... In all the conversions (at least in the ofxMidiTimeCode.cpp file; haven't checked others because I was dealing with this one, sorry) from and to miliseconds you multiply and divide seconds by 100... Shouldn't it be divided or multiplied by 1000? I'm new in developing in here and I beg you pardon in advance if I'm wrong... But it seemed strange to me and I couldn't find a reason... Perhaps you are so kind of explaining me here. Thanks in advance for your kind time. Alex

danomatika commented 5 years ago

I don't see any divisions by 100, only 1000.0. Which lines are you referring to?

alex-calamar commented 5 years ago

Mmm... Sorry again, maybe I'm not understanding your code... Forgive a beginner... What about this?

/// convert to time in seconds double ofxMidiTimecodeFrame::toSeconds() const { double time = (double)hours 3600.0; // 60.0 60.0 time += (double)minutes * 60.0; time += (double)seconds; time += ofxMidiTimecode::framesToMs(frames, rate) / 100; return time; }

Thanks for your time. Alex

alex-calamar commented 5 years ago

Any news on this? Sorry...

danomatika commented 5 years ago

Have you tried how it runs with / 100 instead of / 100? I haven't had time to look into this again.

alex-calamar commented 5 years ago

Yes, with your code I had some strange behaviours... Then I reviewed and saw this "100 - 1000" issue... I changed your conversions from 100 to 1000 and then it worked fine for me...

Anyway it is strange that you have conversions from seconds to milliseconds just multiplying by 100... Like these functions here... Don't you think?

// -----------------------------------------------------------------------------
int ofxMidiTimecode::framesToMs(int frames, unsigned char rate) {
    return (int)((1.0/rateToFps(rate)) * (double)frames * 100.0);
}

// -----------------------------------------------------------------------------
int ofxMidiTimecode::msToFrames(int ms, unsigned char rate) {
    return (int)((double)ms / (1.0/rateToFps(rate)) / 100.0);
}

Well, thanks again for your time... Just want to help.

Regards. Alex

danomatika commented 5 years ago

Those look questionable, yeah. Also, no need to apologize when you think you found a bug!

danomatika commented 5 years ago

Try changing those conversions to 1000 and let me know if it works better overall.

alex-calamar commented 5 years ago

Good morning again, Yes, I did change all your conversions 100 and /100 with 1000 and /1000 and it worked better for me. I forked the project and there you can see my changes in the ofxMidiTimeCode.cpp and ofxMidiTimeCode.h files . I also added a new function toMillisecons, wich returns milliseconds as toSeconds returns just seconds. It would be wonderful if you could add also that to your code, I think it can be useful for everyone. I need it and I think it is interesting for the class to have it.

Well, just check it out and if you need any other help from me just tell me.

I just apologized because I'm new developing here in the community and I did not want to go wrong in my first approach. Just being polite... Once can't be polite in excess :dancing_men:

Thanks again for your time. Alex

danomatika commented 5 years ago

Try the latest commit.

I also added a new function toMillisecons, wich returns milliseconds as toSeconds returns just seconds.

I see that as a convenience function, so not really necessary. It's easy enough to do the conversion by multiplying to Seonds by 1000.

alex-calamar commented 5 years ago

Yes, you are right... I thought that maybe you could do some kind of rounding and return just "seconds", but I see that toSeconds returns a float, so it can be easily multiplied by 1000 to get miliseconds, you are right.

Thanks then. Nice to feel that I helped to a project in here. See you around. Alex

danomatika commented 5 years ago

Nice to feel that I helped to a project in here.

Sure. Every little bit helps. I actually ported the MTC and MIDI Clock code from another project and I noticed I had made the 100 / 1000 mistake there originally. So this issue has fixed a couple projects now.