search

sezero / mikmod

Mikmod Sound System (mirror of git repo at https://sf.net/projects/mikmod/)
http://mikmod.sourceforge.net/
69 stars 21 forks source link

Fixed a little bug in S3M effect A (set speed). 0x80 is also a negati… #35

Closed neumatho closed 3 years ago

neumatho commented 3 years ago

Fixed a little bug in S3M effect A (set speed). 0x80 is also a negative number, that's why the equal sign need to be there too. Found a module that sets the speed to 0x80, which made over a 2 minutes silence.

First I though that the bug need to be fixed in the DSMI/AMF loader, but found out that the issue is in fact in the player. Found module is attached. The speed is set at the end of the module.

Metromania Partie 1.zip

sezero commented 3 years ago

@AliceLR: Is this good?

AliceLR commented 3 years ago

At a glance this patch seems correct to me. Since it affects formats other than DSMI AMF (S3M, IT, GDM, IMF; OctaMED references this effect but bounds it outside of the affected value), I'll do a little bit of testing soon.

AliceLR commented 3 years ago

I haven't checked the other formats yet but the behavior of 0x80 being treated as speed 128 appears to be accurate to Dual Module Player 4.00, the MOD player that DSMI and the AMF format were originally created for. The notes in the final pattern also seem consistent with it being intended as speed 128 (subjective). OpenMPT also treats this as speed 128.

This doesn't mean this patch is wrong, but if the ST3 and DMP behaviors are different this effect might need to be split.

dmp_000

neumatho commented 3 years ago

Does DMP also have silence for over 2 minutes at the end? I just tried with OpenMPT to see how it handles it, and it also sets the tempo to 128, so it seems to be the correct way.

Now we only need to check how the Streamtracker 3 editor handles it, to see if we need to split the command or not. Can you check that?

sezero commented 3 years ago

Send pa patch (a PR) that overrides this one.

AliceLR commented 3 years ago

Going to go ahead and close this, since #42 has been merged.