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

Fix arpeggio + vibrato in linear frequencies #36

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rasky
Copy link
Contributor

@rasky rasky commented Nov 24, 2021

In #27, we refactored xm_frequency to split arpeggio (note offset) from
vibrato (period offset). Unfortunately, we did test it properly only
for amiga frequencies.

It turns out the linear frequency implementation is wrong. The note
offset cannot be added to the period, but must be added to the note
itself. In case of linear frequencies, that's trivial to do once we
get the frequency, as we can simply scale it.

This fixes binary_world.xm and so finally...

Closes #1

In Artefact2#27, we refactored xm_frequency to split arpeggio (note offset) from
vibrato (period offset). Unfortunately, we did test it properly only
for amiga frequencies.

It turns out the linear frequency implementation is wrong. The note
offset cannot be added to the period, but must be added to the note
itself. In case of linear frequencies, that's trivial to do once we
get the frequency, as we can simply scale it.

This fixes binary_world.xm and so finally...

Closes Artefact2#1
@rasky rasky marked this pull request as draft December 1, 2021 08:37
@rasky
Copy link
Contributor Author

rasky commented Dec 1, 2021

Switched to draft as I found a regression in a module with this which I cannot understand.

@Artefact2
Copy link
Owner

Would you mind sharing the module that triggers the regression? (Or a stripped example if the module cannot be shared for licensing reasons.)

@rasky
Copy link
Contributor Author

rasky commented Dec 28, 2021

I think it's this one:
https://modarchive.org/index.php?request=view_by_moduleid&query=36595

After the intro, in the chorus there are some high pitch notes that sound distorted with my commit

@sbechet
Copy link

sbechet commented Apr 10, 2024

Maybe this bug extract can help.

You can find here and here the implementation. It would also be necessary to clip the notes with the highest frequency but I don't want that in my version. It's too horrible.

@rasky
Copy link
Contributor Author

rasky commented Apr 10, 2024

Maybe this bug extract can help.

You can find here and here the implementation. It would also be necessary to clip the notes with the highest frequency but I don't want that in my version. It's too horrible.

I don't see how implementing the historical FT2 arpeggio table overflow here helps, given that binary_world.xm has tempo 5, so there is no table overflow. Just in case, using this patch is enough to test:

diff --git a/src/play.c b/src/play.c
index 6f1b931..f35dc15 100644
--- a/src/play.c
+++ b/src/play.c
@@ -195,6 +195,7 @@ static void xm_tremolo(xm_context_t* ctx, xm_channel_context_t* ch, uint8_t para
 }

 static void xm_arpeggio(xm_context_t* ctx, xm_channel_context_t* ch, uint8_t param, uint16_t tick) {
+       if (tick > 14) printf("arpeggio tick: %d\n", tick);
        switch(tick % 3) {
        case 0:
                ch->arp_in_progress = false;

and the printf never triggers during binary_world.xm playback.

@sbechet
Copy link

sbechet commented Apr 28, 2024

Hello rasky,

You right: it's only a part of the bug.

I look again and again and ... again because i want to fix this (maybe last) issue for my own code.

I found that.

It seems we must implement E3y effect (slide by semitones) for portamento and arpeggio to close this bug.

Original function is here.

Maybe this time we are close to the solution.

What do you think about that?

arp.xm.zip

@rasky
Copy link
Contributor Author

rasky commented Apr 28, 2024

Do you see that effect used in binary_world.xm?

The problem I hear is in pattern order 7 (index 4), channels 8 and 9, which sounds out of tune with libxm. The instrument being used is 9, which has autovibrato. In the pattern, effect 0 (arpeggio) is used. So it is probably a bug in the combination of the two.

@sbechet
Copy link

sbechet commented Apr 30, 2024

I realize that I have not detailed enough the elements that allow me to reach this conclusion. I started with binary_world.xm pattern number 7, channel 8. Like you, I noticed the instrument is 9. So I extracted this instrument with ft2-clone as well as the track to isolate it then I worked by gradually removing the effects on the instrument. You will also notice that it is the same sample in instrument 1 of my arp.xm example. I continued to compare the result between ft2-clone and players like libxm which do not work.

I too thought about the autovibrato, but it seems that it is a side effect since it changes the period "marginally" and that has an impact on the future.

By removing all the effects from the instrument, I found myself... without effects....I then started to play with the correction of the base frequency (C-4). II then did a basic track to highlight the problem that we see in arp.xm by listening to line 8 which should have increased by a semitone (B-5 -> C-6) which was not the case when listening via ft2-clone.

I then looked at the ft2-clone code. In particular this famous adjustPeriodFromNote() function which is systematically called during the tick!=0 of the arpeggio function. This function is also used for portamento only if the semitone increments flag is activated: the famous E3y effect (slide by semitones).

I'm sorry I didn't take the time to clarify my explanations in my last message.

I have the impression that we have a serious lead on these strange frequency changes.

Are my explanations clearer?

@sbechet
Copy link

sbechet commented Apr 30, 2024

I have done a test here.

@sbechet
Copy link

sbechet commented May 1, 2024

Hello,

As i expected binary_world.xm is now working correctly on my rust player. You can adapt code for libxm if you want using the code here.

I had to completely break the logic of managing current periods. In particular to work more comfortably with the non-linear periods of the Paula of the Amiga. To do this, I used a curve approximation function that I found using the curve_fit function of scipy.optimize. I don't hear any difference in sound on the few modules I have tried.

Looking back, I feel like there were some errors in notes 4 to 9 in the historical tables...

I'll add the last missing pieces (E3y and portamento) a little later. Finally, it's good to have May 1st in the rain...

Have a nice day.

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.

Playback inaccuracies
3 participants