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

Minor volume column porta fix #4

Closed
wants to merge 2 commits into from

Conversation

DanielOaks
Copy link
Contributor

So I'm looking into the porta issues in #1, specifically the file aurora_dawn.xm.

I haven't yet cracked why it sounds so interesting (Milky sounds like it's not even applying porta to the note, I think it might be something to do with the tempo and ticks, seeing what I'm reading through a few docs. Maybe something with how we do SLIDE_TOWARDS or update_frequency), but I'm gonna try and keep plodding along with it and see what I find.

But I did find this issue, which is why that big spike was happening. It was taking the effect number as well as the actual effect value (instead of 7 there, it was grabbing a much larger number because of the M).

I haven't had a good look around at xm implementations, and I haven't done much audio work before, but I think porta's only meant to slide towards between the current and new note, right? I find it odd that even a larger than normal tone_portamento_param would make the note frequency spike – if anything, I'd expect it to just go between the two notes much faster than usual. But again, I'm not too familiar with XM, so I'll keep plodding along and see if I can work out the issue!

edit: Having a play with MT, it looks like it is just doing the porta very fast, I suspect due to the tempo variable I saw some documentation about a bit earlier. I'll do some testing and see if I can get libxm sounding just like MT/etc sounds.

@Artefact2
Copy link
Owner

I've added a failing test in 73ac33b. Turns out Mx seems to behave like 3x0 instead of 30x in MilkyTracker, but it doesn't sound that way in libxm.

@DanielOaks
Copy link
Contributor Author

Ah, I see now. I made a little test case and that's exactly what's going on, shouldn't take too much to fix hopefully. Thanks for pointing that out, @Artefact2!

@Artefact2
Copy link
Owner

Actually according to http://www.milkytracker.org/docs/MilkyTracker.html#fxMx Mx behaves like 3xx (but I can not hear the difference). Same for panning.

@DanielOaks
Copy link
Contributor Author

Ah, I see. The push I just did looks to do it (I also checked the value of ch->tone_portamento_param in both sets of the Mx and the 3xx lines in my test case xm file, and the final value of that variable match perfectly), though I may want to double-check with more values and make sure they're equal.

The only major issue is that first porta, where the M7 seems to jump the porta pitch up quite a bit. I've got my test xm here on Dropbox

@Artefact2
Copy link
Owner

69a5e8a fixes behavior of Mx and Px. Sounds good but the spike is still there.

@DanielOaks
Copy link
Contributor Author

Ah, that works. Yeah, that spike is very strange, if you change this:

        XM_SLIDE_TOWARDS(ch->period,
                         ch->tone_portamento_target_period,
                         (ctx->module.frequency_type == XM_LINEAR_FREQUENCIES ?
                          4.f : 1.f) * ch->tone_portamento_param
        );

in xm_tone_portamento to just

        ch->period = ch->tone_portamento_target_period;

temporarily for testing, given the way it sounds I think it could be an issue with how tone_portamento_target_period gets set, or something along those lines, possibly? I haven't been able to nail it down yet, though.

edit: It also only happens the first time the porta M7 / 370 effect happens in a track. After the spike happens once on a track, the rest of them don't have that same spike.

@Artefact2
Copy link
Owner

Thanks for noticing that! It's now fixed in 467d312.

The bug happened when you would use a tone portamento first to set the effect value but without a target note.

@Artefact2 Artefact2 closed this Jan 8, 2015
@DanielOaks
Copy link
Contributor Author

Awesome, no problem! Thanks a bunch for the help.

@DanielOaks DanielOaks deleted the minor-porta-fix branch January 8, 2015 14:38
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.

2 participants