UltraTracker fixes

[neumatho]

Just made some small fixes:

1. Fixed sample speed to calculate it correctly by using the original formula with floating points. "Break the beat" have a beat that runs too fast, because of miscalculation before.
2. Portamento to note fixed. According to the documentation, it should keep run until it reaches the note, even if other effects are running. "Consolidation" module has two porta in the beginning of the song and before the fix, it reached a false tone, because it didn't continue.
3. Clear invalid loop values. Some modules has values when loop is not set.

UltraTracker.zip

[sezero]

@AliceLR: can you please review?

[sezero]

I took the liberty of re-basing this patch to current git (@neumatho: please verify)

I wonder whether we can avoid libm:pow(). I also wonder why finetune is read as a signed short and not unsigned..

@AliceLR ?

[AliceLR]

I haven't thoroughly checked this patch (or the Farandole Composer patch) yet but I think pulling in libm and/or using floating point math in MikMod is not a very good idea. Part of the appeal of MikMod (to me anyway) is that it has sort of lingered in the same state for 20 years, so while it was maybe considered "slow" in 2001, it's probably the fastest of the main mod libraries today (not verified...). It hasn't accumulated floating point cruft and the same bizarre indirection e.g. libxmp has for some features.

Modern PCs can do floating point math just fine, but for example, one of MegaZeux's target platforms I've been looking at MikMod for is the Nintendo DS, an ARMv5 machine with no hardware floating point support that will almost certainly need to have a hardware mixer driver written for it.

I don't have any particular suggestions for getting rid of this particular call right now (unless MikMod has a base-2 exponent table sitting around somewhere already). In this case it's at least not as big of a performance concern as the one in the Farandole Composer patch, but I still don't like it. I could also see if it's possible to calculate a better interpolant than the original.

[neumatho]

I took the liberty of re-basing this patch to current git (@neumatho: please verify)

Its look fine by me.

[sezero]

Re: the wrong speed calculation without pow() usage, i.e. this part of the patch:

           pow(2,(double)s.finetune/OCTAVE/32768), but to avoid floating point
           here, we'll use a first order approximation here.
           1/567290 == Ln(2)/OCTAVE/32768 */
-       q->speed=s.speed+s.speed*(((SLONG)s.speed*(SLONG)s.finetune)/567290);
+       /* Thomas Neumann: Uncommented the original code and use the
+          right formula. Today's computers are fast enough to calculate
+          a little bit floating point.
+          The reason is because the module "Break the beat.ult" have a
+          finetune value of -169, so the original code calculates the
+          speed to -8363 and set it on an usigned variable, so we got a
+          really high value. By using the original formula, we get a
+          speed of 8360 */
+//     q->speed=s.speed+s.speed*(((SLONG)s.speed*(SLONG)s.finetune)/567290);
+       q->speed = s.speed * pow(2, (double)s.finetune / (OCTAVE * 32768));

Is it naive of me to think that abs()ing the calculated result would be a legitimate solution here?

[neumatho]

It will be positive, but not the exactly right value. According to the comment, the old formula result in -8363 and you will then abs()ing it to 8363. The right value should be 8360. If that is close enough and will not be out of tune, I do not know.

Maybe there could also be some other corner cases where it will be wrong.

[sezero]

You are right, there are several others than display significant differences.

[sezero]

You are right, there are several others than display significant differences.

Took all the files from http://ftp.modland.com/pub/modules/Ultratracker/ and ran a test on them printing the finetune and calculated speed values:

• original formula: original.txt
• orig.formula + abs: original+abs.txt
• with pow() formula: with-pow.txt

Apparently, abs() changes only three cases and only two of them actually correctly...

[neumatho]

I wonder if it is possible to retrieve the original source from somewhere (either a player or the editor) and see what they do in this case. It is probably written in assembly, so the use of floating point is minimal.

[sezero]

I don't know.

Playing with the non-zero finetune values, I don't know how I can get an acceptable result without using pow():

--- original.txt
+++ with-pow.txt
@@ -1,45 +1,45 @@
 u1/cyboccultation.ult
-678 -> 83630
+678 -> 8373

 u1/dance of the electric willows.ult
--1 -> 8363
+-1 -> 8362

 u1/dark force.ult
--536 -> 4294917118 (50178 with abs())
+-536 -> 8355

 u1/fight hunting!.ult
 50 -> 8363
--178 -> 4294958933 (8363 with abs())
+-178 -> 8360
--78 -> 0
+-78 -> 8361
--20 -> 8363
+-20 -> 8362
--200 -> 4294958933 (8363 with abs())
+-200 -> 8360

 u1/further.ult
-105 -> 16726
+105 -> 8364

 u1/i can't stop - ecstasy remix.ult
-600 -> 75267
+600 -> 8371

 u1/love starts with an l.ult
-522 -> 66904
+522 -> 8370
-100 -> 16726
+100 -> 8364

 u1/rack of lam.ult
 30 -> 8363
--40 -> 8363
+-40 -> 8362
-175 -> 25089
+175 -> 8365

 u1/sir pranz-a-lot.ult
--18 -> 8363
+-18 -> 8362

 u1/solar-dance.ult
 30 -> 8363
-80 -> 16726
+80 -> 8364
--133 -> 0
+-133 -> 8361
 50 -> 8363

 u1/spherical glide.ult
-25000 -> 3085947
+25000 -> 8739

 u1/the earth's last defender theme.ult
-500 -> 66904
+500 -> 8370

Unless someone provides another solution (@AliceLR ?), I will have to accept pow()?

[sezero]

@AliceLR: what do you suggest?

[neumatho]

Maybe you can use this code:

https://gist.github.com/Madsy/1088393

It contains pow and others using fixed points. I have not tried it, but maybe you can create a test project with it and test with the numbers from above and see if it can produce the right results?

[AliceLR]

I still think a numerical approximation could be used here depending on the domain of this function. I haven't looked close enough at it, sorry. I will get back to you soon re: this. edit: that said, including math.h isn't really the end of the world, and this isn't particularly high-traffic code.

[sezero]

I still think a numerical approximation could be used here depending on the domain of this function.

Indeed - If you have a solution, that'd be nice.

edit: that said, including math.h isn't really the end of the world, and this isn't particularly high-traffic code.

Yeah

[AliceLR]

Sorry for the delay (again). The linear interpolant used in the original code is fundamentally correct but, like many other things in MikMod, was implemented in a totally baffling and wrong way. Try this:

q->speed = s.speed * ((double)s.finetune / 567290.0 + 1.0);

Comparison of the exponent vs. the interpolant shows that, using the range of s.finetune as the domain, they are extremely close. I don't think a second order interpolant would improve very much. https://www.wolframalpha.com/input/?i=plot+2%5E(x/(12*32768)),+x/567290+1+on+%5B-32768,32767%5D

[sezero]

With Alice's version, Break The Beat sounds very close to libxmp's output. Thomas, what do you say?

[sezero]

Alice: apart from the pow approximation (which I intend to use yours, waiting to hear from Thomas), is the rest of the patch OK?

[neumatho]

With Alice's version, Break The Beat sounds very close to libxmp's output. Thomas, what do you say?

It seems to work.

[AliceLR]

Alice: apart from the pow approximation (which I intend to use yours, waiting to hear from Thomas), is the rest of the patch OK?

After having given it a look, I'm not sure. I think the fixed interpolant is fine but unrolling a continuous effect seems unusual and prone to breaking in edge cases. I'll need to familiarize myself with UltraTracker and that effect to have a better idea of how to deal with it.

edit: note libxmp has the same tone portamento unrolling hack.

[AliceLR]

Proof of concept for why the portamento unrolling that EVERY player seems to do (libxmp, MikMod with this patch, OpenMPT) isn't sufficient. PORTA.ULT.zip

The initial C-2 should start tone portamento at row 15 of order 0 and reach F#1 by the end of order 1. Instead, it decrements slightly each time the tone portamento command is encountered.

[sezero]

OK, I pushed ed3e5b3394a07669055a694f0ce3e60ebb905c48 for the sample speed issue. With this, I have only two modules giving different results than pow():

cyboccultation.ult finetune: 678 -> 8373 (pow) -> 8372

spherical glide.ult finetune: 25000 -> 8739 (pow) -> 8731

[sezero]

Waiting for the solution for portamento issue and the rest (i.e. items 2 and 3 on Thomas' original list).

[AliceLR]

I have a working tone portamento fix for libxmp that handles various minor quirks. Presumably this could be ported to MikMod. It might be more work since, as far as I know, it doesn't have persistent tone portamento implemented outside of the FAR branch (and FAR tone portamento IIRC behaves different compared to libxmp's persistent tone portamento implementations).

[sezero]

I have a working tone portamento fix for libxmp that handles various minor quirks. Presumably this could be ported to MikMod. It might be more work since, as far as I know, it doesn't have persistent tone portamento implemented outside of the FAR branch (and FAR tone portamento IIRC behaves different compared to libxmp's persistent tone portamento implementations).

I'd like to see that in mikmod if it's doable. Does #37 help in any way?

[AliceLR]

After some lengthy work on my module tools I was able to find two Ultra Tracker modules in my local ModLand copy that unambiguously require actual persistent tone portamento:

the earth's last defender theme.ult. Affected portamento occurs between orders 9 and 10. consolidation.ult. Affected portamento occurs between orders 23 and 24.

[AliceLR]

On further inspection of the original finetune algorithm, it seems to be wrong: in Ultra Tracker, finetune steps of 512 seem to roughly correspond to a semitone, so the "correct" formula might be 2**(finetune / (12 * 512)). (I don't know how correct this is, someone with more knowledge re: the GUS might have more insight.)

edit: it might be adding the finetune value to the Hz after octave calculations, because 512 at an octave lower sounds more like a whole step. WTF...?

[sezero]

On further inspection of the original finetune algorithm, it seems to be wrong: in Ultra Tracker, finetune steps of 512 seem to roughly correspond to a semitone, so the "correct" formula might be 2**(finetune / (12 * 512)). (I don't know how correct this is, someone with more knowledge re: the GUS might have more insight.)

edit: it might be adding the finetune value to the Hz after octave calculations, because 512 at an octave lower sounds more like a whole step. WTF...?

Can @sagamusix be of some help?

[sagamusix]

I don't have any specific insight here (it's very well possible that what OpenMPT currently does is wrong), but it is well-known that the GUS suffers in terms of frequency precision of lower notes in many trackers. This may cause issues in particular with slide precision if slides are directly computed in terms of GUS frequency units, which is a natural thing to do in a GUS-only tracker. The effective resolution of finetune and slides may in fact differ based on the number of initialized hardware channels - I don't know if UltraTracker always initializes the card with 32 channels, but if it does, that means that the slide precision is the lowest possible. I don't know the formula by heart, but it can be lifted easily from e.g. DOSBox' GUS emulation. I think one finetune step was 1/1024th of the GUS mixer frequency (which depends on the number of channels).

[sezero]

I rebased this to current master: Reviews? Comments?

[sezero]

@AliceLR: Can you review this? (Several parts had already been merged, it is now a small single patch.)

[AliceLR]

This is still unrolling tone portamento, so it is probably still incorrect. I'd have to refamiliarize myself with MikMod to figure out if the FAR features I mentioned above are the best way forward. IMO libxmp, MegaZeux, and a friend's game I'm trying to score are higher priority for now, sorry.

I will say that this is an uncommon format and probably this issue isn't worth blocking on if you're looking at tagging a release.

[sezero]

OK then. I plan to tag a new release this month. This will stay open, so whenever a proper fix is available, it will go in.

[AliceLR]

Actually, you know what? Stick a couple of visible "FIXME"s on this + file an issue and just merge it, it's better than nothing.

[sezero]

Actually, you know what? Stick a couple of visible "FIXME"s on this + file an issue and just merge it, it's better than nothing.

Can I leave that to you?

[AliceLR]

Actually, you know what? Stick a couple of visible "FIXME"s on this + file an issue and just merge it, it's better than nothing.

Can I leave that to you?

OK, will do in a little bit.

[sezero]

Thanks