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

Audio tests update, consolidated into one app #2335

Merged
merged 40 commits into from
Oct 31, 2024

Conversation

richardeakin
Copy link
Collaborator

@richardeakin richardeakin commented Oct 5, 2024

This PR is a redux of the audio tests I developed when designing the ci::audio tools, which were difficult to keep working with so many project files. I've now refactored the tests into one main app that can construct tests based on a string, which is:

  • more maintainable (less project files and boilderplate code)
  • lends itself well to UI's (see the selectors in the screenshot below)
  • would be easy to automate testing if we decide to.

This also switched the UI from my DIY mini kit to Dear ImGui, which is now part of cinder core (it wasn't when ci::audio was initially developed).

Remaining Todo List

  • MSVC / Visual Studio updates
  • Mac / iOS updates - I need help here as I don't have a macbook anymore, please let me know if you can pitch in!
  • CMake / Linux updates
  • I've seen a crash related to the "Audio Context" ImGui window, I think it is related to audio::Context's auto-pulled nodes, still need to further investigate
  • The test suite relies on a utility class called mason::Factory that I've used in many projects to create sub-classed ('factory') classes based on a string. Need to figure out where this should live, I'd like it to be a header-only class in cinder core but there is more debate around how generic a factory tool within cinder core should be.
  • A bit more testing and some small UI additions since updating the many tests.
  • Address duped MonitorNodes after switching through tests multiple times

Here's what one of the tests (WaveTable) looks like:
image

@moldybeats
Copy link

@richardeakin I can try to help out with testing on Mac. It's been a few years since I've been involved with a Cinder (or C++) project so I might need some guidance, but let me know if you still need help here. I have a couple Apple devices available for testing.

@richardeakin
Copy link
Collaborator Author

Hi @moldybeats thanks for offering to help! Basically the steps I've taken in the past are:

  • generate a project with the correct name using tinderbox
  • move just the xcode and xcode_ios folders to the test/_audio/AudioTests/proj/ folder
  • open them up and correct the paths to libcinder
  • add the sources from test/_audioAudioTests/src to the project files
  • build and see what happens!

Also, here on some notes about cinder on OS X, they're a bit old now so there might be some minor mods necessary to get the libs and samples built on OS X but I couldn't tell you exactly what as I haven't used an Apple machine for many years now.

@richardeakin
Copy link
Collaborator Author

@andrewfb handed me the Mac OS X project files (thanks again!) and I just pushed them to this branch @moldybeats - testing is still welcome if you have the time. I also pushed a change that addressed the Audio Context UI window causing a rare crash, and some decent initial window position settings.

@moldybeats
Copy link

@richardeakin With those new project files, building & running the AudioTests Xcode project worked great for me. I submitted a PR with fixes for a couple really small issues. Here are a few other things I noticed while clicking around that might be a little harder to pinpoint:

  • If you move between Node Basic and Stress tests with audio enabled, you'll see the number of auto pulled nodes increasing. Not sure if that's expected or not. Seemed like I ran into this a few times, but this was the easiest way to reproduce it.
  • In the Node Effects > one test, the context graph begins with a DelayNode and a Pan2dNode. If you click "use sine", you lose the DelayNode (even if you uncheck the box) and the audio output is audibly different.
  • In the Node Effects > echo test, if Context Graph > expand all is checked, the app hangs and eventually crashes. The stacktrace seems like it's in a loop inside printNodeFn().

Hope that helps. Let me know if you need more info on any of these.

@richardeakin
Copy link
Collaborator Author

Thanks @moldybeats for testing! I've addressed the issues you found as well as what was left on my list. The believe I have a fix for the stray MonitorNodes in this commit, which is an oversight in the Context::disconnectAllNodes() helper function. I'll PR that separately after this gets merged, since it is a core change.

@richardeakin richardeakin marked this pull request as ready for review October 30, 2024 01:33
@richardeakin richardeakin merged commit 174c86b into cinder:master Oct 31, 2024
6 of 8 checks passed
@richardeakin richardeakin deleted the rte/audio_tests_update branch October 31, 2024 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants