Using map.emplace with sf::Shader

[KRMisha]

Hello,

I'm currently making a resource manager with which I can load, unload, or get a reference to a textures/fonts/soundbuffers/shaders. The problem is with the shaders: I can't seem to add them to an STL container (in my case, an std::unordered_map) because of the sf::NonCopyable. Even if I use .emplace(key, shader), my compiler doesn't accept it. How should I go about this?

Here is my current code:

const sf::Shader& ResourceManager::LoadShader(const std::string &rName, const std::string &rFilename, sf::Shader::Type type)
{
    // If a Shader is already loaded at the specified key, return the existing Shader
    std::unordered_map<std::string, sf::Shader>::const_iterator it = m_shaders.find(rName);
    if (it != m_shaders.cend())
    {
        return it->second;
    }
    // Otherwise, load the Shader
    sf::Shader shader;
    if (!shader.loadFromFile(rFilename, type))
    {
        std::cout << "ResourceManager error: Could not load Shader: \"" << rName << "\".\n";
        return m_shaders.at("ADDDEFAULTSHADER");
    }
    m_shaders.emplace(rName, shader); // Compiler error here because of deleted copy constructor
    return m_shaders.at(rName);
}
 

Thanks in advance!

[Hapax]

You are creating a shader and then passing it to the constructor of the new, emplaced shader, which would be the copy-constructor. Try emplacing it first and then loading it. If the load fails, you can always remove it.

[KRMisha]

I tried this and it doesn't compile either.

/// Otherwise, load the Shader
    m_shaders.emplace(rName, sf::Shader());
    if (!m_shaders.at(rName).loadFromFile(ResourcePath() + rFilename, type))
    {
        std::cout << "ResourceManager error: Could not load Shader: \"" << rName << "\".\n";
        m_shaders.erase(rName);
        return m_shaders.at("ADDDEFAULTSHADER");
    }
    return m_shaders.at(rName);
 

To confirm that this isn't just a problem with my implementation in this case, I used an sf::Texture instead, which compiled successfully:

/// Otherwise, load the Shader
    m_textures.emplace(rName, sf::Texture());
    if (!m_shaders.at(rName).loadFromFile(ResourcePath() + rFilename, type))
    {
        std::cout << "ResourceManager error: Could not load Shader: \"" << rName << "\".\n";
        m_shaders.erase(rName);
        return m_shaders.at("ADDDEFAULTSHADER");
    }
    return m_shaders.at(rName);
 

Correct me if I'm wrong, but wouldn't the sf::Shader need a move constructor, which would let us do std::move to place it in an STL container? We could then use it like this (which currently doesn't work because of sf::NonCopyable):

/// Otherwise, load the Shader
    sf::Shader shader;
    if (!shader.loadFromFile(ResourcePath() + rFilename, type))
    {
        std::cout << "ResourceManager error: Could not load Shader: \"" << rName << "\".\n";
        return m_shaders.at("ADDDEFAULTSHADER");
    }
    m_shaders.emplace(rName, std::move(shader));
    return m_shaders.at(rName);
 

[dabbertorres]

You're still copying.

I think you want:

m_shaders.emplace(rName);

[KRMisha]

You're still copying.

I think you want:

m_shaders.emplace(rName);

That isn't valid syntax; it's an std::map/std::unordered_map

[Hapax]

Doesn't just this:

m_shaders[rName];

create the shader in the map, ready to use?

[KRMisha]

Doesn't just this:

m_shaders[rName];

create the shader in the map, ready to use?

Unfortunately not, it needs an assignment operator after it (=), which itself would need to call the copy constructor of the sf::Shader.

[KRMisha]

Doesn't just this:

m_shaders[rName];

create the shader in the map, ready to use?

Unfortunately not, it needs an assignment operator after it (=), which itself would need to call the copy constructor of the sf::Shader.

No, sorry, I'm wrong. It works! Thanks a lot

[KRMisha]

But shouldn't SFML allow support for std::move with sf::Shader? Or is this not possible?

[Hapax]

Move constructors are C++11. SFML 2 is C++98.

[dabbertorres]

You're still copying.

I think you want:

m_shaders.emplace(rName);

That isn't valid syntax; it's an std::map/std::unordered_map

Ah, you're right, I was thinking of C++17's try_emplace.