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

tests: CompParam::readCurrentValue() & getFinalValue() #2068

Conversation

nikodemus
Copy link
Contributor

  • Pull out the guts for testing.

  • Rename standard menu item roundtrip test to "scaling test", since it tests a few specific values in addition to roundtripping.

- Pull out the guts for testing.

- Rename standard menu item roundtrip test to "scaling test",
  since it tests a few specific values in addition to roundtripping.
Copy link
Contributor

github-actions bot commented Jun 1, 2024

Test Results

36 tests  +1   36 ✅ +1   0s ⏱️ ±0s
 7 suites ±0    0 💤 ±0 
 7 files   ±0    0 ❌ ±0 

Results for commit f17dadf. ± Comparison against base commit 96a78f8.

This pull request removes 1 and adds 2 tests. Note that renamed tests count towards both.
ValueScalingTest ‑ standardMenuItemValueRoundTrip
ValueScalingTest ‑ compParamMenuItemValueScaling
ValueScalingTest ‑ standardMenuItemValueScaling

auto value = (int64_t)soundEditor.currentParamManager->getUnpatchedParamSet()->getValue(getP());
this->setValue((value * (kMaxMenuValue * 2) + 2147483648) >> 32);
}
// comp params aren't set up for negative inputs - this is the same as osc pulse width
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this accurate? If so the function should probably be named for unipolar param instead of for comp param

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's another PR coming that deals with pulse width and does just that :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

A bunch of the unipolar params do use the full [-1,1) q31 range but are notionally (e.g. volume). HalfPrecisionUnipolar maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had InternallyUnipolar previously, but HalfPrecisionUnipolar is better!

@sapphire-arches sapphire-arches added this pull request to the merge queue Jun 2, 2024
Merged via the queue into SynthstromAudible:community with commit d5ab4ee Jun 2, 2024
6 checks passed
@nikodemus nikodemus deleted the comp-param-value-scaling branch June 2, 2024 16:50
tastycode pushed a commit to tastycode/DelugeFirmware that referenced this pull request Jul 3, 2024
…ible#2068)

- Pull out the guts for testing.

- Rename standard menu item roundtrip test to "scaling test",
  since it tests a few specific values in addition to roundtripping.
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.

None yet

3 participants