Ptplayer bugfixes

[rveilleux]

A pull request for a variety of fixes to ptplayer: I tried several MODs and now they all sound fantastic. They are probably still some little bugs on some effects, and I have a least two questions for you (KaiN) for further discussion / investigation for later.

[rveilleux]

Hi KaIN,

thank you for the feedback!

I did put some explanations in my individual commits: I wonder if the pullrequest brings them? Else perhaps you can gaze a my fork repo?

Also I've made a potential great speed improvement on findPeriod that we can discuss in another thread if you are interested.

Rem.

On Oct 27, 2022, at 12:43 PM, KaiN @.***> wrote:

 @tehKaiN requested changes on this pull request.

Overall I'd approve and merge it as-is, but the last commented-out code needs some more comments on why it is this way.

Tomorrow I'll try to check 2 things mentioned in review against original assembly to see if it's my mistake or if there's some kind of intent for current state of code.

In src/ace/managers/ptplayer.c:

@@ -1215,7 +1216,7 @@ static void mt_playvoice( pChannelData->n_reallength = uwSampleLength;

  // Determine period table from fine-tune parameter

• UBYTE ubFineTune = pSampleDef->ubFineTune; // d3
• UBYTE ubFineTune = pSampleDef->ubFineTune & 0xF; This worries me a bit. Is it for some kind of malformed MODs or perhaps there are values above 15 which need some kind of different handling for topmost bits? A comment in the source code would be helpful here.

In src/ace/managers/ptplayer.c:

@@ -1231,7 +1232,7 @@ static void mt_playvoice( // Set repeat pChannelData->n_looped = 1; pChannelData->n_replen = pSampleDef->uwRepeatLength;

• uwSampleLength = pSampleDef->uwRepeatLength + ubFineTune * 2;
• uwSampleLength = pSampleDef->uwRepeatLength; note to self: check against original .s file if it was there or if it's some kind of bug due to asm->c translation

In src/ace/managers/ptplayer.c:

@@ -1243,7 +1244,7 @@ static void mt_playvoice( }

// inlined set_regs function here:

• if(!pVoice->uwNote) {
• if(!(pVoice->uwNote & 0xFFF)) { note to self: check against .s file

In src/ace/managers/ptplayer.c:

@@ -1393,7 +1394,9 @@ static void intSetRep(volatile tCustom *pCustom) { systemSetCiaInt(CIA_B, CIAICRB_TIMER_B, 0, 0); }

-// One-shot TimerB interrupt to set repeat samples after another 576 ticks. +#define TIMERB_TICKS (576) this should be on top of file in the DEFINES section

In src/ace/managers/ptplayer.c:

@@ -1791,7 +1794,7 @@ static void mt_toneporta_nc( tChannelStatus pChannelData, volatile tChannelRegs pChannelReg ) { if(pChannelData->n_wantedperiod) {

• UWORD uwNew;
• WORD uwNew; since you've changed UWORD to WORD, the prefix should be changed from uw to w

In src/ace/managers/ptplayer.c:

@@ -2514,7 +2517,7 @@ static void set_sampleoffset( else { pChannelData->n_sampleoffset = ubArg; }

• UWORD uwLength = ubArg << 7;
• UWORD uwLength = ubArg << 6; I'd reword << 6 to something like * (256 / sizeof(UWORD)) to indicate the intent - ubArg is length in 256s of bytes, but we need the length in words, I assume. Perhaps also add a comment if that kind of notation is not clear enough.

In src/ace/managers/ptplayer.c:

• --ubPeriodPos;
• }
• if(pChannelData->n_minusft && ubPeriodPos) {
• // Negative fine tune? Then take the previous period.
• --ubPeriodPos;
• }
• // if(ubPeriodPos) {
• // // One before for less/equal
• // --ubPeriodPos;
• // }
• // if(pChannelData->n_minusft && ubPeriodPos) {
• // // Negative fine tune? Then take the previous period.
• // --ubPeriodPos;
• // } what's going on here?

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

[tehKaiN]

now I see that commit history describe the changes, but this kind of documenting is problematic - it isn't that easy to inspect without checking out the branch and using some kind of IDE gutter which shows the commit message near each of its related lines. Even then it only shows only message for latest change.

I strongly prefer to have this type of documentation as code comments. I'd be more than grateful if you could transfer most of the helpful messages to comments.

Re optimizations, I think it's best to keep PRs as concise as possible. Let's merge in this and discuss the optimization separately.

[tehKaiN]

I'm gonna take a look on that original .s file and will merge it today. Sorry for delay!