Simple virtual piano

Question

The application is a virtual piano with only one octave for simplicity.

What can I improve with this code?

#include <iostream>
#include <SFML/Audio.hpp>
#include <SFML/Graphics.hpp>
#include <math.h>

static const int SAMPLE_RATE = 441000;
static const int WIDTH = 400;
static const int HEIGHT = 150;
static const int KEYS = 7;
static const float KEY_WIDTH = float(WIDTH) / KEYS;
static const float KEY_THICKNESS = 1.0f;
enum NOTES {
    DO3 = 261,
    REB3 = 277,
    RE3 = 293,
    MIB3 = 311,
    MI3 = 329,
    FA3 =  349,
    SOLB3 = 369,
    SOL3=  391,
    LAB3 = 415,
    LA3 =  440,
    SIB3= 466,
    SI3 =  493
};
float map(float n, float x1, float x2, float y1, float y2)
{
    float m = (y2 - y1) / (x2 - x1);
    return y1 + m * (n - x1);
}
sf::Int16 squarewave(float t, float freq)
{
    int output = std::floor(2 * freq * t);
    return 32767 * (output & 1 ? -1 : 1);
}
sf::Int16 sawtooth(float t, float freq)
{
    float T = 1.0f / freq;
    float start = T * std::floor(t / T);
    float end = start + T;
    return 32767 * map(t, start, end, -1.0f, 1.0f);
}
class Tone : public sf::Sound
{
public:
    
    Tone(float freq)
    {
        std::vector<sf::Int16> samples;
        float t = 0;
        float dt = 1.0f / SAMPLE_RATE;
        int nsamples = SAMPLE_RATE * (1.0f / freq);
        for (int i = 0; i < nsamples; i++)
        {
            sf::Int16 sample = sawtooth(t, freq);
            t += dt;
            samples.push_back(sample); //left channel
            samples.push_back(sample); //right
        }
        buffer.loadFromSamples(samples.data(), samples.size(), 2, SAMPLE_RATE);
        setBuffer(buffer);
    }
private:
    sf::SoundBuffer buffer;
};
class PianoKey
{
public:
    PianoKey(float x, float y, NOTES note, sf::Keyboard::Key k) :  tone(note) {
        rect.setFillColor(sf::Color::White);
        rect.setSize({ KEY_WIDTH, HEIGHT });
        rect.setPosition(x, y);
        rect.setFillColor(sf::Color::White);
        rect.setOutlineThickness(KEY_THICKNESS); 
        rect.setOutlineColor(sf::Color::Black);
        key = k;
    }
    PianoKey(NOTES note, sf::Keyboard::Key k) : tone(note)
    {
        rect.setFillColor(sf::Color::Black);
        rect.setOutlineThickness(KEY_THICKNESS);
        rect.setOutlineColor(sf::Color::Black);
        isFlatKey = true;
        key = k;
    }
    void render(sf::RenderTarget& rt)
    {
        rt.draw(rect);
    }
  
    void update() {
        if (isPressed())
        {
            if (tone.getStatus() != Tone::Playing)
            {
                tone.play();
                tone.setLoop(true);
                rect.setFillColor(sf::Color::Green);
            }
        }
        else
        {
            tone.setLoop(false);
            if (isFlatKey)
            {
                rect.setFillColor(sf::Color::Black);
            }
            else {
                rect.setFillColor(sf::Color::White);
            }
        }
    }
    sf::Vector2f getPos()
    {
        return rect.getPosition();
    }
    void putAboveOf(PianoKey &other) //this function is used for put the flat (sharp?) keys above of the white keys
    {
        float rightTop = other.getPos().x + KEY_WIDTH;
        rect.setSize({ KEY_WIDTH / 2, HEIGHT / 2 });
        rect.setPosition(rightTop - rect.getSize().x / 2, 0);
    }
private:
    bool isPressed()
    {
        return sf::Keyboard::isKeyPressed(key);
    }
    sf::RectangleShape rect;
    sf::Keyboard::Key key;
    bool isFlatKey = false;
    Tone tone;
};
class Piano
{
public:
    ~Piano()
    {
        for (int i = 0; i < keys.size(); i++)
        {
            delete keys[i];
        }
    }
    Piano() {
        std::vector<NOTES> notes{ DO3, RE3, MI3, FA3, SOL3, LA3, SI3 }; //white keys
        std::vector<sf::Keyboard::Key> keyboardkeys{sf::Keyboard::Z, sf::Keyboard::X, sf::Keyboard::C, sf::Keyboard::V,
        sf::Keyboard::B, sf::Keyboard::N, sf::Keyboard::M};
        for (int i = 0; i < KEYS; i++)
        {
            PianoKey *key = new PianoKey(i * (KEY_WIDTH + KEY_THICKNESS), 0, notes[i], keyboardkeys[i]);
            keys.push_back(key);
        }
        
        keys.push_back(new PianoKey(REB3,  sf::Keyboard::S));//flat keys
        keys.push_back(new PianoKey(MIB3,  sf::Keyboard::D));
        keys.push_back(new PianoKey(SOLB3, sf::Keyboard::G));
        keys.push_back(new PianoKey(LAB3,  sf::Keyboard::H));
        keys.push_back(new PianoKey(SIB3,  sf::Keyboard::J));

        keys[7]->putAboveOf(*keys[0]);
        keys[8]->putAboveOf(*keys[1]);
        keys[9]->putAboveOf(*keys[3]);
        keys[10]->putAboveOf(*keys[4]);
        keys[11]->putAboveOf(*keys[5]);
    }
    void update()
    {
        for (auto& key : keys)
        {
            key->update();
        }
    }
    void render(sf::RenderTarget& rt)
    {
        for (auto& key : keys)
        {
            key->render(rt);
        }
    }
private:
    std::vector<PianoKey*> keys;
};
class App {
public:
    App() : window(sf::VideoMode(WIDTH, HEIGHT), "Simple piano")
    {
        window.setVerticalSyncEnabled(true);
    }
    void run()
    {
        while (window.isOpen())
        {
            handleEvents();
            update();
            render();
        }
    }
private:
    void handleEvents()
    {
        sf::Event ev;
        while (window.pollEvent(ev))

        {
            switch (ev.type)
            {
            case sf::Event::Closed:
                window.close();
                break;
            case sf::Event::GainedFocus:
                hasFocus = true;
                break;
            case sf::Event::LostFocus:
                hasFocus = false;
                break;
            default:
                break;
            }
        }
    }
    void update()
    {
        if (hasFocus)
        {
            piano.update();
        }
     
    }
    void render()
    {
        window.clear(sf::Color::White);
        piano.render(window);
        window.display();
    }
    bool hasFocus = true;
    sf::RenderWindow window;
    Piano piano;
};
int main()
{
    App app;
    app.run();
    return 0;
}

Answer 1

std::floor and other mathematical functions require inclusion of <cmath> rather than <math.h>. The latter may also define the std namespace versions, but is not required to, so a portable C++ program includes <cmath>.

---

I'm concerned about these owning pointers:

std::vector<PianoKey*> keys;

Because we're using raw pointers, we needed to write a destructor for Piano. Smart pointers take care of cleaning up owned resources, so we could use std::unique_ptr<PianoKey> instead of PianoKey*. However, I'm not convinced that we even need pointers there: I think std::vector<PianoKey> should be fine, or perhaps even a std::array<PianoKey, 12>.

---

These loops have an unnecessary level of indirection:

    for (auto& key : keys)
    {
        key->update();
    }

    for (auto& key : keys)
    {
        key->render(rt);
    }

Since the keys are pointers, it's sufficient to write for (auto *key: keys).

Of course, when we change the type of keys, this issue goes away, and we can return to using references.

---

At least one member function should not be changing its object, so should be declared const:

bool isPressed() const;
//               🔺🔺🔺🔺🔺

---

Hard-coding the assignment of letters to keys is probably a mistake. It will make playing the piano very difficult for those of us who don't share your (computer) keyboard's layout.

Answer 2

Toby Speight already commented on the C++ part, I'll focus on the audio generated by your code.

Most notes are slightly out of tune

While the middle A is defined to be 440 Hz, the other notes in an equal temperament octave cannot be represented by integers. So your frequencies will be off somewhat from the "right" frequencies. In the middle octave, the error is small enough (around 2 cents) that most humans cannot hear the difference, but if you would add lower octaves to your keyboard, then this error will become more audible.

However, this is not the only source of error. You also then use the slightly wrong frequency to set the buffer size. But at 440 Hz, the buffer is only 100 samples long, so now any rounding error in the buffer size will result in a frequency error of up to 20 cents, and this can indeed already be perceived as being out of tune. And if you would add higher octaves to your keyboard, then this error will become more audible.

The problems can be mitigated by not hardcoding the note frequencies as integers, and by using a higher sample rate. For the frequencies: if you want to keep enum NOTES, I would just make the enum values the number of semitones relative to the middle A, and have a function to convert from semitone to frequency:

enum NOTES {
    DO3 = -9;
    REB3 = -8;
    ...
    LA3 = 0;
    SIB3 = 1;
    SI3 = 2;
};

static const float semitone_ratio = std::pow(2.0, 1.0 / 12.0);
static constexpr float middle_A_frequency = 440.0;

float semitone2frequency(int semitone) {
    return middle_A_frequency * std::pow(semitone_ratio, semitone);
}

However, raising the sample rate increases the overhead and might not be possible. It would be better to not use buffers with pregenerated waveforms, but instead to generate the waveforms them on the fly, although that will introduce the problem of aliasing.