Fix merge conflicts and logical errors in engine sound modules

[barkleesanders]

This commit resolves issues arising from a previous merge in the engine sound system.

Key changes:

• Corrected EngineSoundGenerator.py to use sound file paths from GlobalConstants.VEHICLE_ASSETS (i.e., under static/sounds/) instead of a local ./Sounds directory. It now maps its internal sound names to engineSoundIDs from GlobalConstants to fetch the correct sound path.
• Fixed an incorrect constant usage in Main.py for EngineSoundPitchShifter initialization. It now correctly uses GC.MC_LAREN_F1 (from GlobalConstants) instead of ESPS.MC_LAREN_F1.
• Verified that GlobalConstants.validate_assets() functions correctly, confirming consistent asset path expectations.
• Reintegrated EngineSoundGenerator into Main.py's integration_test():
  • Uncommented the import.
  • Added a basic instantiation and functionality test.
• Ensured both EngineSoundGenerator and EngineSoundPitchShifter can be instantiated and attempt to load sounds without critical errors in Main.py's integration_test(). Playback errors due to missing audio hardware in test environments are handled gracefully.
• Commented out esps.unit_test() in Main.py as it's unsuitable for automated testing due to its interactive nature and pynput dependencies.

[gemini-code-assist]

Hello @barkleesanders, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

Hello team,

Gemini here, providing a summary of this pull request authored by barkleesanders. The primary goal of this PR, as indicated by the title and description, is to resolve merge conflicts and logical errors that arose in the engine sound modules (EngineSoundGenerator.py and EngineSoundPitchShifter.py) following a previous merge. The changes focus on standardizing the use of GlobalConstants for sound asset paths and IDs, improving error handling around audio device availability and file loading, and updating the integration tests in Main.py to reflect these changes and better handle test environments without audio hardware.

Highlights

• Standardized Asset Paths and IDs: The EngineSoundGenerator has been updated to use sound file paths defined in GlobalConstants.VEHICLE_ASSETS and map its internal sound names to the corresponding engineSoundIDs from GlobalConstants, moving away from hardcoded local paths.
• Improved Audio Error Handling: Both EngineSoundGenerator and EngineSoundPitchShifter now include more robust error handling during initialization and sound loading/playback, specifically addressing scenarios where sound files are missing or audio devices are unavailable (common in CI/test environments) by logging warnings and attempting graceful fallback.
• Updated Integration Tests: The integration_test function in Main.py has been updated to correctly initialize EngineSoundPitchShifter using GlobalConstants and now includes a basic test for EngineSoundGenerator. The interactive EngineSoundPitchShifter.unit_test() has been commented out as it's not suitable for automated testing.
• Enhanced Unit Tests for EngineSoundGenerator: The EngineSoundGenerator.unit_test() has been significantly expanded to cover initialization with valid/invalid keys, attempting to set sounds with missing files, and attempting to set invalid keys after initialization, including assertions and logging for each step.

Changelog

Click here to see the changelog

• EngineSoundGenerator.py
  • Imported GlobalConstants (line 10).
  • Updated engine_sounds_dict to use GlobalConstants IDs instead of integers (lines 32-39).
  • Initialized base_audio_filename, engine_sound_wave_object, and engine_sound_id to None in __init__ (lines 41-44).
  • Added logic in __init__ to attempt loading a default sound (mc_laren_f1) if the initial sound fails to load, logging warnings (lines 48-59).
  • Added a check in start_audio_loop to ensure engine_sound_wave_object is loaded before attempting to play (lines 74-76).
  • Refactored set_engine_sound to find sound paths in GC.VEHICLE_ASSETS based on the sound ID (lines 97-102).
  • Added checks in set_engine_sound for file existence before loading (lines 108-110).
  • Added try...except block in set_engine_sound to catch exceptions during wave file loading, logging warnings and keeping the current sound (lines 106-119).
  • Improved logging in set_engine_sound for successful changes, file not found, path not found in assets, and invalid sound keys (lines 117, 109, 123, 127).
  • Expanded unit_test with multiple test cases covering valid/invalid initialization and sound setting attempts, including assertions and error handling for audio playback (lines 140-222).
• EngineSoundPitchShifter.py
  • Initialized self.stream to None and self.running to True in __init__ (lines 81-82).
  • Refactored stream setup in __init__ to move stream.start() into the try block (lines 85-87).
  • Added specific except sd.PortAudioError for handling environments without audio devices, logging a message (lines 89-93).
  • Added a general except Exception for unexpected errors during stream setup (lines 97-99).
  • Updated cleanup to check if self.stream is not None before attempting to stop/close it (line 105).
  • Added print statements in cleanup for clarity (lines 108, 110).
  • Added a check for status in audio_callback and prints it (lines 115-116).
  • Added a check for self.stream is not None in the main logic of audio_callback (line 118).
• Main.py
  • Imported GlobalConstants as GC (line 22).
  • Uncommented and imported EngineSoundGenerator (line 36).
  • In integration_test, changed ESPS initialization to use GC.MC_LAREN_F1 instead of ESPS.MC_LAREN_F1 (line 47).
  • Commented out the call to esps.unit_test() (line 48).
  • Added a section to test EngineSoundGenerator, including instantiation, assertion, playback attempt with a short delay, stopping, and error handling for playback issues (lines 50-71).
  • Removed the devDB parameter from the integration_test function definition (line 43).
  • Removed the devDB argument from the call to integration_test in the main function (line 105).
  • Removed a commented-out line GC.validate_assets() (line 94).

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

---

Audio files fixed,
Constants now guide the way,
Sounds play, or not.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request effectively addresses several issues, including merge conflicts, incorrect constant usage, and file path problems in the engine sound modules. The changes significantly improve the robustness of EngineSoundGenerator by adding better error handling for sound file loading and more comprehensive unit tests. The integration of EngineSoundGenerator into Main.py's integration test is also a good step forward. The code is clearer and more maintainable with the use of GlobalConstants.

Overall, great work on these fixes and enhancements!

Summary of Findings

• Clarity of fallback string in log messages: In EngineSoundGenerator.py (lines 122, 126), the fallback string for current_sound_display, "current (or None if not initialized)", is a bit verbose. This is a minor point and the current implementation is explicit, but it could potentially be simplified for conciseness in log messages. (Not commented due to review settings: low severity)
• Exception type checking in unit tests: In EngineSoundGenerator.py unit tests (e.g., line 154), checking exception types using e.__class__.__name__ == "SimpleaudioError" is functional but can be fragile if the exception class name changes in future versions of the simpleaudio library. If simpleaudio's specific error types are importable, using isinstance(e, SimpleaudioErrorType) or a direct except SimpleaudioErrorType: would be more robust. This is a minor concern, especially within test code. (Not commented due to review settings: low severity)
• Logging consistency in EngineSoundPitchShifter.py: EngineSoundPitchShifter.py uses print() for logging, while EngineSoundGenerator.py uses the logging_utils module. While the changes in this PR are consistent with the existing style of EngineSoundPitchShifter.py, standardizing logging across modules using logging_utils could be considered for future enhancements. (Not commented due to review settings: low severity, and potentially out of scope for this PR)

Merge Readiness

The pull request introduces valuable fixes and improvements. The code is generally in good shape. I recommend addressing the medium-severity issue raised regarding the logging level for critical messages in EngineSoundGenerator.py to ensure consistency and appropriate error signaling. After that, the PR should be in a strong position for merging. As a reviewer, I am not authorized to approve the pull request; please ensure further review and approval as per your team's policy.

[gemini-code-assist]

The log message here indicates a "CRITICAL" failure, but it's being logged with log_warning. While this will make the message visible, using log_error would more accurately reflect the severity of the situation within the logging system, especially since log_error is available in logging_utils.py. This helps in filtering and prioritizing logs based on severity levels.

Could log_error be more appropriate here to align the log function with the message's stated severity?

Suggested change

	log_warning(f"CRITICAL: Default sound '{self.mc_laren_f1}' also failed to load. EngineSoundGenerator may not function correctly.")
	log_error(f"CRITICAL: Default sound '{self.mc_laren_f1}' also failed to load. EngineSoundGenerator may not function correctly.")