Jump to content
  • Advertisement
Sign in to follow this  
Niddles

What am I forgetting here? (Pointer Problem?)

This topic is 4138 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

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

Share this post


Link to post
Share on other sites
Advertisement
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.

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
Quote:
Original post by Driv3MeFar
Calling delete on a NULL pointer is guaranteed to be a safe operation...

I suppose I should have known this...but I didn't! Very cool.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!