0

To make the code looks clean, I declare a class MaterialTexture in namespace Material to store the light attributes for OpenGL. And I want to create instance(such as metal, plastic) for this class in the same namespace. I do some googling and find that a previous discussion shows that data members can't be assigned in the namespace. And the constructor is suggested to initialize the data members.

However, there're 12 data members in my case, and it would be tedious to pass 12 arguments to construct a instance. Instead, I want to use three functions (such as setDiffuse() ) to set the value of each instance.

namespace Material
{
    class MaterialTexture
    {
        private:
            float diffuse[4];
            float specular[4];
            float ambient[4];
        public:
            // Three functions below set the values for data members above. 
            void setDiffuse(float R, float G, float B, float A);
            void setSpecular(float R, float G, float B, float A);
            void setAmbient(float R, float G, float B, float A);

            /* the following function tell the OpenGL how to draw the 
                Texture of this kind of Material and is not important 
                here.                                                 */
            void setMaterial();
    };
    void MaterialTexture::setDiffuse(float R, float G, float B, float A)
    {
        diffuse[0]=R; diffuse[1]=G; diffuse[2]=B; diffuse[3]=A;
    }

    // Create instances plastic and metal
    MaterialTexture plastic;
    plastic.setDiffuse(0.6, 0.0, 0.0, 1.0);
    ...

    MaterialTexture metal;
    metal.setDiffuse(...);
    ...

}; // end of namespace

Thus, to create a red-plastic like sphere, I only need to type following codes in the display call back:

Material::MaterialTexture::plastic.setMaterial();
glutSolidSphere(...);

Compile the above code with g++, it gives the error:

error: ‘plastic’ does not name a type

and

error: ‘metal’ does not name a type

in the line of the three setting functions (such as setDiffuse() ) of each instance.

Thus it seems not only assignment directly in namespace, but functions contain assignment are not allowed... Am I right? Is there any other way to fix this? Or, are there some other way to facilitate the OpenGL programing?

Community
  • 1
  • 1
WhiteRivers
  • 93
  • 1
  • 5
  • Statements such as `metal.setDiffuse(...);` can only go inside of functions. – juanchopanza Jul 06 '14 at 14:33
  • `Material::plastic.setMaterial();` shouldn't this be `Material::MaterialTexture::plastic.setMaterial();`? – πάντα ῥεῖ Jul 06 '14 at 14:33
  • _'and it would be tedious to pass 16 arguments to construct a instance. Instead'_ What about using a [builder](http://en.wikipedia.org/wiki/Builder_pattern) or [prototype](http://en.wikipedia.org/wiki/Prototype_pattern) pattern? – πάντα ῥεῖ Jul 06 '14 at 14:37
  • Dear πάντα ῥεῖ 4, you're correct, it's Material::MaterialTexture::plastic.setMaterial(); sorry for the mistake. Since I had altered the source code to simplified the code, there would be some mistake here. – WhiteRivers Jul 06 '14 at 14:55

2 Answers2

0

You simply cannot call a function "in the middle of nowhere". This has nothing to do with namespaces. Consider this complete example:

void f() { }

f();

int main()
{
}

It won't compile.

A solution would be to add three different static member functions returning the three special instances, which may actually be created in another (private) static member function. Here is an example:

class MaterialTexture
{
private:
    float diffuse[4];
    float specular[4];
    float ambient[4];

    static MaterialTexture makePlastic()
    {    
        MaterialTexture plastic;
        plastic.setDiffuse(0.6, 0.0, 0.0, 1.0);
        return plastic;
    }

public:
    // Three functions below set the values for data members above. 
    void setDiffuse(float R, float G, float B, float A);
    void setSpecular(float R, float G, float B, float A);
    void setAmbient(float R, float G, float B, float A);

    static MaterialTexture &plastic()
    {
        static MaterialTexture plastic = makePlastic();
        return plastic;
    }
};

There's room for improvement, though, especially if you can use C++11:

  • Replace your raw arrays with std::array.
  • Consider using double instead of float.
  • Add a private constructor which allows you to get rid of the private static member functions and instead allows you to create the object directly.

Here is a complete improved example:

class MaterialTexture
{
private:
    std::array<double, 4> diffuse;
    std::array<double, 4> specular;
    std::array<double, 4> ambient;

    MaterialTexture(std::array<double, 4> const &diffuse, std::array<double, 4> const &specular,
        std::array<double, 4> const &ambient) : diffuse(diffuse), specular(specular), ambient(ambient) {}
public:
    // Three functions below set the values for data members above. 
    void setDiffuse(double R, double G, double B, double A);
    void setSpecular(double R, double G, double B, double A);
    void setAmbient(double R, double G, double B, double A);

    static MaterialTexture &plastic()
    {
        static MaterialTexture plastic(
            { 0.6, 0.0, 0.0, 1.0 },
            { 0.0, 0.0, 0.0, 0.0 },
            { 0.0, 0.0, 0.0, 0.0 }
        );
        return plastic;
    }
};
Christian Hackl
  • 27,051
  • 3
  • 32
  • 62
  • I don't think proposing to use `double` in this case is good advice. OpenGL generally operates with `float`, so using `double` in supporting classes will just result in a lot of type conversions later. I actually think using doubles over floats in general should only be done if the additional precision is really needed. – Reto Koradi Jul 06 '14 at 15:29
  • It's been quite some time since I actually used OpenGL, but generally, in C++ the default rule is to favour `double` over `float`. If OpenGL normally uses `float`, then that's OK, of course. – Christian Hackl Jul 06 '14 at 15:40
  • Sort of a side note, but where does the "favor double over float" come from? It uses twice the memory, and is slower. So that seems like a very questionable guideline. I have used C++ for a long time, and never heard that recommendation. – Reto Koradi Jul 06 '14 at 15:54
  • It can also be faster than float, and it's more precise in any case. See http://stackoverflow.com/questions/3426165/is-using-double-faster-than-float (including a Stroustrup citation). – Christian Hackl Jul 06 '14 at 16:07
0

Beyond the immediate problem with your code (all executable code has to be within a function, which is not the case in your example), how to structure this best is quite subjective. Based on that, read the paragraphs below with an implied "IMHO". Here is my proposal for a simple solution that should get you going.

I'll stick with your naming to keep things consistent. Even though I don't like the name MaterialTexture for this class. In the context of OpenGL, the name suggests that the class encapsulates a texture, which it's not.

First of all, classes should have constructors that initialize all members. This makes sure that even if the class is not used as expected, no class member is ever in an uninitialized state, which could otherwise result in undefined behavior. I agree that having a constructor with 12 float arguments would be awkward in this case. I would have a default constructor that initializes all members to a reasonable default, e.g. matching the defaults of the old fixed pipeline:

MaterialTexture::MaterialTexture()
{
   diffuse[0] = 0.8f;
   diffuse[1] = 0.8f;
   diffuse[2] = 0.8f;
   diffuse[3] = 1.0f;

   ambient[0] = 0.2f;
   ambient[1] = 0.2f;
   ambient[2] = 0.2f;
   ambient[3] = 1.0f;

   specular[0] = 0.0f;
   specular[1] = 0.0f;
   specular[2] = 0.0f;
   specular[3] = 1.0f;
}

Your setDiffuse(), setSpecular() and setAmbient() methods look fine.

Now the question is where you create the specific materials (plastic, metal). Using static methods in this class is one option, but I don't think it's a clean design. This class represents a generic material. Putting knowledge of specific materials in the same class mixes up responsibilities that should be separate.

I would have a separate class that provides specific materials. There's a lot of options on how that could be designed and structured. The simplest is to have methods to create the specific materials. For a slightly nicer approach that allows you to add materials without changing any interfaces, you could reference them by name:

class MaterialLibrary
{
public:
    MaterialLibrary();

    const MaterialTexture& getMaterial(const std::string& materialName) const;

private:
    std::map<std::string, MaterialTexture> m_materials;
}

MaterialLibrary::MaterialLibrary()
{
    MaterialTexture plastic;
    plastic.setDiffuse(...);
    ...
    m_materials.insert(std::make_pair("plastic", plastic));

    MaterialTexture metal;
    metal.setDiffuse(...);
    ...
    m_materials.insert(std::make_pair("metal", metal));
}

const MaterialTexture& MaterialLibrary::getMaterial(const std::string& materialName) const
{
    return m_materials.at(materialName);
}

You could easily change this to read the list of materials from a configuration file instead of having it in the code.

Now you just need to create and use one instance of MaterialLibrary. Again, there's a multiple ways of doing that. You could make it a Singleton. Or create one instance during startup, and pass that instance to everybody who needs it.

Once you have a MaterialLibrary instance lib, you can now get the materials:

const MaterialTexture& mat = lib.getMaterial("plastic");

You could get fancier and make the MaterialLibrary more of a factory class, e.g. a Template Factory. This would separate the responsibilities even further, with the MaterialLibrary only maintaining a list of materials, and providing access to them, but without knowing how to build the list of specific materials that are available. It's your decision how far you want to go with your abstraction.

Reto Koradi
  • 53,228
  • 8
  • 93
  • 133