Skip to content

Conversation

@princetiwari1910
Copy link

Description

This PR fixes an issue where the temperament widget was playing incorrect notes for default temperaments (non-12EDO) such as 5EDO, 7EDO, 19EDO, and 31EDO. The widget was passing raw frequency values directly to the synth, which bypassed the temperament mapping system entirely.

Problem

When playing notes in the temperament widget for default temperaments:

  • Raw frequency values were passed directly to synth.trigger()
  • The synth's _getFrequency() method was bypassed
  • Temperament mapping via temperamentChanged() was not applied
  • This resulted in notes playing at incorrect frequencies that didn't match the temperament's interval structure

Solution

Modified the playNote function in js/widgets/temperament.js to:

  1. Set synth's temperament state before playing notes to ensure proper mapping
  2. Use note names (strings) instead of raw frequencies for default temperaments
  3. Convert note arrays to string format (e.g., ["C", 4]"C4")
  4. Handle Unicode sharps/flats conversion to ASCII equivalents
  5. Maintain backward compatibility for edit modes and custom temperaments

Changes Made

  • Updated playNote() function to set synth.inTemperament and synth.changeInTemperament before playing
  • Added logic to convert note arrays to string format for default temperaments
  • Added Unicode-to-ASCII conversion for sharps/flats (♯ → #, ♭ → b)
  • Preserved existing behavior for edit modes and custom temperaments

Technical Details

For default temperaments, the synth now:

  1. Receives note names as strings (e.g., "C4", "D#4")
  2. Calls _getFrequency() which triggers temperamentChanged()
  3. Uses the correct frequency mapping from noteFrequencies
  4. Plays notes at the correct frequencies according to the temperament's interval ratios

Testing

  • Verified syntax with Node.js syntax check
  • Code review confirms correct logic flow
  • Maintains backward compatibility with existing functionality
  • Handles edge cases (fallback to frequency if note format is unexpected)

Related Issue

Fixes #4033

Checklist

  • Code follows the project's style guidelines
  • Self-review of code performed
  • Comments added for complex logic
  • Documentation updated (if applicable)
  • No new warnings generated
  • Tests pass (if applicable)
  • Changes are backward compatible

Note: This fix ensures that default temperaments (5EDO, 7EDO, 19EDO, 31EDO, etc.) play notes correctly according to their interval structure, resolving the issue where notes were playing at incorrect frequencies.

Use note names instead of raw frequencies when playing notes in the
temperament widget for default temperaments (non-12EDO). This ensures
the synth applies the correct temperament mapping via _getFrequency()
instead of bypassing it with raw frequency values.

The fix:
- Sets synth's inTemperament and changeInTemperament before playing
- Converts note arrays to string format (e.g., ["C", 4] -> "C4")
- Handles Unicode sharps/flats conversion to ASCII
- Maintains backward compatibility for edit modes and custom temperaments

Fixes sugarlabs#4033
@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@walterbender
Copy link
Member

@pikurasa can you please test?

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.

Temperment Widget playing weird notes

2 participants