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 IT sample global volume and sample vibrato. #218

Merged
merged 1 commit into from Nov 16, 2020

Conversation

AliceLR
Copy link
Contributor

@AliceLR AliceLR commented Nov 11, 2020

Fixes a few bugs in IT sample global volume and sample vibrato handling:

  • IT sample global volume was being ignored in sample mode due to QUIRK_INSVOL not being enabled. QUIRK_INSVOL is now unconditionally enabled for IT modules.
  • IT sample global volume was also being ignored in instrument mode for IT 1.x modules due to QUIRK_INSVOL not being enabled for IT 1.x modules.
  • Fixed an incorrect bitshift on IT sample vibrato depth that resulted in IT sample vibrato being very weak.
  • Sample vibrato rate is now rounded instead of truncated in read_event.c. This is a bandaid fix and the LFO period size should probably eventually be expanded.

Fixes #102 and #103.

edit: 5 failing tests from this patch, will look into them.

@AliceLR AliceLR linked an issue Nov 11, 2020 that may be closed by this pull request
@AliceLR
Copy link
Contributor Author

AliceLR commented Nov 11, 2020

I fixed the one module data test that this broke (differences were due to the change in sample vibrato depth). Haven't looked too close at the four failing OpenMPT mixer tests yet.

* IT sample global volume was being ignored in sample mode due to
  QUIRK_INSVOL not being enabled. QUIRK_INSVOL is now unconditionally
  enabled for IT modules.
* IT sample global volume was being ignored in instrument mode for
  IT 1.x modules due to QUIRK_INSVOL not being enabled for IT 1.x
  modules. QUIRK_INSVOL is now unconditionally enabled for IT modules.
* Fixed an incorrect bitshift on IT sample vibrato depth that resulted
  in IT sample vibrato being very weak.
* Sample vibrato rate is now rounded instead of truncated. The LFO
  period size should probably eventually be expanded instead.
@AliceLR
Copy link
Contributor Author

AliceLR commented Nov 11, 2020

Remaining tests:

  • test_openmpt_it_autovibrato_reset.c: the property it is testing (the sample vibrato sweep) is unaffected by these changes, so I've just updated the data file.
  • test_openmpt_it_filter_reset.c: ditto (testing filter macros, changes are due to the sample global volume fix).
  • test_openmpt_it_panreset.c: ditto (testing X80, changes are due to sample global volume fix).
  • test_openmpt_it_samplenumberchange.c: ditto (testing sample numbers without notes, changes are due to sample global volume fix).

@sezero
Copy link
Collaborator

sezero commented Nov 11, 2020

Let's have @sagamusix and @cmatsuoka look at this.

Copy link
Collaborator

@cmatsuoka cmatsuoka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IT support in libxmp is generally innacurate and problems are fixed as they're reported, so if this change fixes issues or makes replay sound "less wrong" it should be good to go. (There are many hacks in IT replaying caused by IT support being added after the common replay code was already in place -- in the future we should refactor the common player to be more IT friendly, including adding proper bidirectional sample loops).

@AliceLR
Copy link
Contributor Author

AliceLR commented Nov 16, 2020

One of the tests on the master branch failed after this was merged: https://travis-ci.org/github/libxmp/libxmp/builds/743831472

I did a local build of libxmp and the tests on my iMac with GCC with no issue so that's not the problem. Is this maybe an issue on Travis' end?

edit: maybe related: https://travis-ci.community/t/macos-build-an-error-occurred-while-generating-the-build-script/7684

@AliceLR AliceLR deleted the fix-it-sample-volume-vibrato branch November 16, 2020 01:12
@sezero
Copy link
Collaborator

sezero commented Nov 16, 2020

One of the tests on the master branch failed after this was merged: https://travis-ci.org/github/libxmp/libxmp/builds/743831472

I did a local build of libxmp and the tests on my iMac with GCC with no issue so that's not the problem. Is this maybe an issue on Travis' end?

Seems like it

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.

.it sample vibrato rate and depth are off .it global sample volume is broken
3 participants