Back to For Beginners

What am I forgetting here? (Pointer Problem?)

For Beginners

Started by Niddles June 24, 2007 01:55 AM

5 comments, last by _Sigma 16 years, 10 months ago

Niddles

216

Author

June 24, 2007 01:55 AM

Hello, I am forgetting something, or doing something I shouldn't but I can't see it because it is late. I am calling this function:


void Light::SetAmbient(ObjVector3 amb)
{
    UseAmbient = true;
    if(Ambient == NULL)
        Ambient = new ObjVector3(amb);
    else if(Ambient != NULL)
    {
        delete Ambient;
        Ambient = new ObjVector3(amb);
    }
}

That controls the ambient light, but when I go to enable the light, it acts like the ambient has been magically deleted. Here is where I call for the ambient components:


void Light::EnableLight()
{
    if(!glIsEnabled(GL_LIGHTING))
        glEnable(GL_LIGHTING);
    switch(*LightNumber)
    {
        case 0:
            if(!glIsEnabled(GL_LIGHT0))
                glEnable(GL_LIGHT0);
            if(UseAmbient)
            {
                float amb[] = {Ambient->x, Ambient->y, Ambient->z, 1.0}; //HERE: Debugger shows these up as being 5.blahblahblah * 10^-39
                glLightfv(GL_LIGHT0, GL_AMBIENT, amb);
            }
            if(UseDiffuse)
            {
                float dif[] = {Diffuse->x, Diffuse->y, Diffuse->z, 1.0};
                glLightfv(GL_LIGHT0, GL_DIFFUSE, dif);
            }
            if(UseSpecular)
            {
                float spc[] = {Specular->x, Specular->y, Specular->z, 1.0};
                glLightfv(GL_LIGHT0, GL_SPECULAR, spc);
            }
            if(UsePosition)
            {
                float pos[] = {Position->x, Position->y, Position->z, 1.0};
                glLightfv(GL_LIGHT0, GL_POSITION, pos);
            }
            break;
...

Where I call these two functions is here if this helps:


Eng->GetLight(0).SetPosition(ObjVector3(0.0, 2.0, 0.0));
Eng->GetLight(0).SetAmbient(ObjVector3(0.2, 0.2, 0.2));
Eng->GetLight(0).SetDiffuse(ObjVector3(0.8, 0.8, 0.8));
Eng->GetLight(0).SetSpecular(ObjVector3(1.0, 1.0, 1.0));
Eng->GetLight(0).EnableLight();

Anybody see the problem? Thanks

_goat

804

June 24, 2007 02:19 AM

I don't know, but I do know that GL_LIGHT0 is just a constant (or a define) for a numeric value. 0, I think. That means you don't need a huge switch statement, you can just pass *LightNumber as the parameter.

[ search: google ][ programming: msdn | boost | opengl ][ languages: nihongo ]

_Sigma

792

June 24, 2007 02:22 AM

Nothing jumps out at me. A few thoughts:

(i) What does the constructor for ObjVector3 look like?
(ii) Do any of the functions called after SetAmbient modify Ambient at all?

On a side note you can turn

if(Ambient == NULL)        Ambient = new ObjVector3(amb);    else if(Ambient != NULL)    {        delete Ambient;        Ambient = new ObjVector3(amb);    }

into

if(Ambient == NULL)        Ambient = new ObjVector3(amb);    else    {        delete Ambient;        Ambient = new ObjVector3(amb);    }

And lose the extra conditional.

Driv3MeFar

1,080

June 24, 2007 02:53 AM

Quote:Original post by _Sigma
On a side note you can turn
*** Source Snippet Removed ***
into
*** Source Snippet Removed *** And lose the extra conditional.

Condensed even further, just do this:

        delete Ambient;        Ambient = new ObjVector3(amb);

Calling delete on a NULL pointer is guaranteed to be a safe operation, so the check is useless.

ToohrVyk

1,596

June 24, 2007 03:58 AM

This code is so... needlessly complicated!!

// light.hpp ============================================class Light{public:  // Name objects in problem space, not in solution space  typedef ObjVector3 Color;  typedef ObjVector3 Position;  typedef GLenum Mode;  // Pass arguments by constant reference to avoid copies  void SetAmbient(const Color & c);  // The "Light" suffix is redundant  void Enable();private:  // The vector for each property  std::map<Mode,ObjVector3> properties;  // No need to use a pointer  int LightNumber;};// light.cpp ============================================void Light::SetAmbient(const Light::Color & amb){  // Much shorter code!  this->properties[GL_AMBIENT] = amb;}void Light::EnableLight(){  // Also much shorter code!  glEnable(GL_LIGHTING);  const GLenum lights[] = {     GL_LIGHT0, GL_LIGHT1, GL_LIGHT2, GL_LIGHT3,     GL_LIGHT4, GL_LIGHT5, GL_LIGHT6, GL_LIGHT7  };  const GLenum my_light = lights[this->LightNumber];  glEnable(my_light);    for (std::map<Mode,Color>::iterator it = this->properties.begin();       it != this->properties.end(); ++it)  {    Color & color = it->second;    float color_arg[] = { color.x, color.y, color.z, 1.0f };    glLightfv(my_light,it->first,color_arg);  }}

No more memory leaks or hazardous pointer manipulation, and most repetitive operations have been factored out. If the error remains, check the copy constructor of ObjVector3.

Blog — Facebook

jhartelt

141

June 24, 2007 08:40 AM

In your debugger, you could set a watch on the ambient object, when you enter the scope of the class Light. Then, while stepping over the calls, keep an eye on the vector and see which call changed it. Examine that method further.

"Always code as if the guy who ends up maintaining, or testing your code will be a violent psychopath who knows where you live."

_Sigma

792

June 24, 2007 06:01 PM