How to delete an object which is a member of another object c++

Question

I have a function ( AddChild(child) ) which is meant to take an object as a parameter and adds this object as a child of the parent object which invokes the function.

Code creating the scene and invoking the function

Scene *scene = new Scene();

 // Create a light.
Light *light = new PointLight(vec3(10.0f));

light->SetPosition(vec3(-5.0f, 3.0f, 2.0f));
scene->AddChild(light);

So as you can see here, the scene should be adding a light as its child

In the AddChild(child) function, the Object is checked to see if it already has a parent, if it does, it needs to delete this parent (as later on in a different class there is a function that checks all objects in the heap and if the parent object isnt deleted it is still in the heap and causes errors) and update it's parent to be the object which invoked the function. You can see the code which will cause this heap error below:

void Raytracer::SimpleAccelerator::AddObject(const SceneObject *object)
{
    if (object->IsInstanceOf(SceneObjectType_PhysicalObject))
        physicalObjects.push_back((PhysicalObject *)object);

    if (object->IsInstanceOf(SceneObjectType_Light))
        lights.push_back((Light *)object);

    foreach_c (SceneObject *, child, object->GetChildren())
        AddObject(*child);
}

But the main problem I'm having is, when I check to see if the child has a parent, and the parent isnt the object invoking the function, then I want to delete the parent of this child and update accordingly. The only problem is when I try to delete the child's parent Visual Studio crashes and I debugged the code and its on the line indicated below that causes the error.

bool SceneObject::AddChild(SceneObject *child)
{   
    if (child == NULL) {
        return false;
    }
    else if (child->GetParent() != this) {
        delete child->parent;     <------------This is the line which breaks the code
        child->parent = this;
        children.push_back(child);
        child->UpdateTransformations();
        return true;
    }
    else {
        this->children.push_back(child);
        child->parent = this;
        child->UpdateTransformations();
        return true;
    }
}

child and parent are both members of the class SceneObject which you can see below

Class SceneObject
    {
    private:

        glm::mat4x4 globalTransformation;

    public:

        std::vector<SceneObject *> children;
        SceneObject * parent;
        SceneObject *GetParent() const;
        const std::vector<SceneObject *> &GetChildren() const;

    };

What would be the best method of deleting this member so that it and it's member variables are not present in the Heap during the Accelerator function?

MCVC

SceneObject.h

#ifndef RAYTRACER_SCENES_SCENEOBJECT_H
#define RAYTRACER_SCENES_SCENEOBJECT_H

#include <Raytracer/Scenes/SceneObjectType.h>

#include <vector>

namespace Raytracer
{
    namespace Scenes
    {
        class SceneObject
        {
        private:
            glm::mat4x4 globalTransformation;

            void UpdateTransformations();

        public:

            std::vector<SceneObject *> children;
            SceneObject * parent;

            SceneObject();

            virtual ~SceneObject();


            bool AddChild(SceneObject *child);

            const std::vector<SceneObject *> &GetChildren() const;

            const glm::vec3 GetGlobalPosition() const;

            const glm::mat4x4 &GetGlobalToLocal() const;

            const glm::mat4x4 &GetGlobalTransformation() const;

            SceneObject *GetParent() const;
        };
    }
}

#endif // RAYTRACER_SCENES_SCENEOBJECT_H

SceneObject.cpp

#define RAYTRACER_USE_FOREACH
#include <iostream>
#include <Raytracer/Raytracer.h>

using namespace glm;
using namespace Raytracer::Scenes;

SceneObject::SceneObject()
{
    SetGlobalTransformation(mat4x4(1.0f));
}

SceneObject::~SceneObject()
{
}

bool SceneObject::AddChild(SceneObject *child)
{
    if (child == NULL) {
        return false;
    }
    else if (child->GetParent() != this) {
        child->parent = nullptr;
        child->parent = this;
        this->children.push_back(child);
        child->UpdateTransformations();
        return true;
    }
    else {
        this->children.push_back(child);
        child->parent = this;
        child->UpdateTransformations();
        return true;
    }
}

void delete_pointed_to(SceneObject* const ptr)
{
    delete ptr;
}

const std::vector<SceneObject *> &SceneObject::GetChildren() const
{
    // TODO: Geben Sie eine Liste der Kindobjekte dieses Objekts zurück.
    static std::vector<SceneObject *> empty = this->children;
    return empty;
}

const mat4x4 &SceneObject::GetGlobalTransformation() const
{
    return globalTransformation;
}

SceneObject *SceneObject::GetParent() const
{
    // TODO: Geben Sie eine Zeiger auf das Elternobjekt zurück oder NULL, falls dieses Objekt kein
    // Elternobjekt hat.
    if (this->parent == NULL) {
        return NULL;
    }
    else {
        return this->parent;
    }
}

main.cpp

#include <stdio.h>

#include <vector>

#include <glm.hpp>
#include <gtc/matrix_transform.hpp> 

#include <Raytracer/Raytracer.h>

using namespace glm;
using namespace Raytracer;
using namespace Raytracer::Scenes;
using namespace Raytracer::Objects;

/**
 * Creates a transformation matrix for a rotation around the x axis.
 *
 * @param a The rotation angle in degrees
 */
mat4x4 RotationX(float a)
{
  return rotate(mat4x4(1.0f), a, vec3(1.0f, 0.0f, 0.0f));
}

/**
 * Creates a transformation matrix for a rotation around the y axis.
 *
 * @param a The rotation angle in degrees
 */
mat4x4 RotationY(float a)
{
  return rotate(mat4x4(1.0f), a, vec3(0.0f, 1.0f, 0.0f));
}

/**
 * Creates a transformation matrix for a rotation around the z axis.
 *
 * @param a The rotation angle in degrees
 */
mat4x4 RotationZ(float a)
{
  return rotate(mat4x4(1.0f), a, vec3(0.0f, 0.0f, 1.0f));
}


bool CreateSphereLevels(Sphere *parent, float childRadiusRatio, int levels,
            Material** materials, int firstMaterial, int materialCount)
{

// TODO: Implementieren Sie die Funktion nach Ihrer in Aufgabe 1 definierten Vorschrift.
// Der Methodenaufruf CreateSphereLevels(...) in dieser Form ist nur ein Vorschlag,
// Sie können ihn beliebig anpassen und eine andere Logik verwenden

  return true;
}


/**
 * Places a few spheres in the scene and adds some lights.
 *
 * @param scene The scene
 */
Scene *BuildScene(int depth, float aspect)
{
  const int materialCount = 6;

  vec3 colors[materialCount] =
  {
    vec3(1.0f, 0.0f, 0.0f),
    vec3(1.0f, 1.0f, 0.0f),
    vec3(0.0f, 1.0f, 0.0f),
    vec3(0.0f, 1.0f, 1.0f),
    vec3(0.0f, 0.0f, 1.0f),
    vec3(1.0f, 0.0f, 1.0f)
  };

  Material *materials[materialCount];

  for (int i = 0; i < materialCount; i++)
  {
    materials[i] = new Material();
    if (materials[i] == NULL)
      return NULL;

    vec3 ambient = colors[i] * 0.01f;
    materials[i]->SetAmbient(ambient);
    materials[i]->SetDiffuse(colors[i]);
    materials[i]->SetShininess(25.0f);
  }

  if (depth <= 0)
    return NULL;

  // Create the scene.
  Scene *scene = new Scene();
  if (scene == NULL)
    return NULL;

  // TODO: Erstellen Sie hier die Kugelwolke der Tiefe depth und fügen Sie sie in die Szene ein.
  // Verwenden Sie die Materialien in materials zum Einfärben der Kugeln.
  // Impelementieren Sie hierfür z.B. die Funktion CreateSphereLevels(...) (s.o.)




  // Create a light.
  Light *light = new PointLight(vec3(10.0f));
  if (light == NULL)
  {
    delete scene;
    return NULL;
  }

  light->SetPosition(vec3(-5.0f, 3.0f, 2.0f));
  scene->AddChild(light);

  // Create a camera.
  Camera *camera = new Camera(vec3(-2.0f, 2.0f, 4.0f), vec3(0.0f, 0.0f, 0.0f), 
    vec3(0.0f, 1.0f, 0.0f), Camera::DefaultFov, aspect);
  if (camera == NULL)
  {
    delete scene;
    return NULL;
  }
  scene->AddChild(camera);
  scene->SetActiveCamera(camera);

  return scene;
}

/**
 * Renders the scene and saves the result to a BMP file.
 *
 * @param fileName The name of the file
 * @param width The image width
 * @param height The image height
 */
void Render(const char *fileName, int width, int height)
{
  if (fileName == NULL || width <= 0 || height <= 0)
    return;

  SimpleRenderer renderer;

  renderer.SetAccelerator(new SimpleAccelerator());
  renderer.SetIntegrator(new PhongIntegrator());

  puts("Generiere Szene...");
  Scene *scene = BuildScene(3, (float)width / height);
  if (scene == NULL)
    return;

  puts("Rendere Bild...");
  Image *image = renderer.Render(*scene, width, height);
  if (image != NULL)
  {
    puts("Speichere Ergebnis...");
    image->SaveBMP(fileName, 2.2f);
    delete image;
  }

  delete scene;
}

/**
 * The main program
 */
int main()
{
  Render("image.bmp", 512, 512);
  return 0;
}

Simple Accelerator

#include <limits>

#define RAYTRACER_USE_FOREACH
#include <Raytracer/Raytracer.h>

using namespace Raytracer;
using namespace Raytracer::Scenes;

bool SimpleAccelerator::HitTest(const Ray &ray, RayHit *hit) const
{
    if (hit == NULL)
    {
        // Search for any intersection.
        foreach_c (PhysicalObject *, object, physicalObjects)
        {
            Ray objectRay = ray.Transform((*object)->GetGlobalToLocal());
            if ((*object)->HitTest(objectRay, NULL))
                return true;
        }

        return false;
    }
    else
    {
        // Search for the closest intersection.
        hit->Set(ray, 0, NULL);

        foreach_c (PhysicalObject *, object, physicalObjects)
        {
            Ray objectRay = ray.Transform((*object)->GetGlobalToLocal());
            RayHit objectHit;
            if ((*object)->HitTest(objectRay, &objectHit) &&
                (hit->GetObject() == NULL || objectHit.GetDistance() < hit->GetDistance()))
            {
                *hit = objectHit;
            }
        }

        return (hit->GetObject() != NULL);
    }
}

void Raytracer::SimpleAccelerator::SetScene(const Scenes::Scene *scene)
{
    this->scene = scene;
    physicalObjects.clear();
    lights.clear();

    if (scene == NULL)
        return;

    AddObject(scene);
}

void Raytracer::SimpleAccelerator::AddObject(const SceneObject *object)
{
    if (object->IsInstanceOf(SceneObjectType_PhysicalObject))
        physicalObjects.push_back((PhysicalObject *)object);

    if (object->IsInstanceOf(SceneObjectType_Light))
        lights.push_back((Light *)object);

    foreach_c (SceneObject *, child, object->GetChildren())
        AddObject(*child);
}

const std::vector<Light *> &SimpleAccelerator::GetLights() const
{
    return lights;
}

Renderer.cpp

#include <Raytracer/Raytracer.h>

using namespace glm;
using namespace Raytracer;
using namespace Raytracer::Scenes;

Renderer::Renderer()
{
    accelerator = NULL;
    integrator = NULL;
}

Renderer::~Renderer()
{
    delete accelerator;
    delete integrator;
}

vec3 Renderer::RenderPixel(const Camera &camera, float x, float y) const
{
    Ray ray;
    camera.SpawnRay(x, y, ray);
    return integrator->GetColor(ray, *accelerator);
}

IIntegrator *Renderer::GetIntegrator() const
{
    return integrator;
}

void Renderer::SetIntegrator(IIntegrator *integrator)
{
    delete this->integrator;
    this->integrator = integrator;
}

Accelerator *Raytracer::Renderer::GetAccelerator() const
{
    return accelerator;
}

void Raytracer::Renderer::SetAccelerator(Accelerator *accelerator)
{
    delete this->accelerator;
    this->accelerator = accelerator;
}
Image *Renderer::Render(const Scenes::Scene &scene, int width, int height)
{
    Camera *camera = scene.GetActiveCamera();

    if (camera == NULL || accelerator == NULL || integrator == NULL || width <= 0 || height <= 0)
        return NULL;

    Image *image = new Image(width, height);
    if (image == NULL)
        return NULL;

    accelerator->SetScene(&scene);

    RenderImage(*camera, image);

    return image;
}

PhongIntegrator.cpp

        Raytracer::PhongIntegrator::PhongIntegrator()
        {
            backgroundColor = vec3(0, 0, 0);
        }

Renderer.h

#ifndef RAYTRACER_RENDERER_H
#define RAYTRACER_RENDERER_H

#include <glm.hpp>

#include <Raytracer/Image.h>
#include <Raytracer/Scenes/Camera.h>
#include <Raytracer/Scenes/Scene.h>

namespace Raytracer
{
    /**
     * Renders an image of a scene. The renderer object is not thread-safe.
     */
    class Renderer
    {
    private:
        Accelerator *accelerator;
        IIntegrator *integrator;

    protected:
        /**
         * Renders a single pixel.
         *
         * @param camera The camera
         * @param x The normalized x coordinate of the pixel in the 0-1 range
         * @param y The normalized y coordinate of the pixel in the 0-1 range
         *
         * @return The color of the pixel
         */
        virtual glm::vec3 RenderPixel(const Scenes::Camera &camera, float x, float y) const;

        /**
         * Renders an image.
         *
         * @param camera The camera to use for the rendering
         * @param image The image to be filled with rendered pixels
         */
        virtual void RenderImage(const Scenes::Camera &camera, Image *image) const = 0;

    public:
        /**
         * Constructs a new Renderer object.
         */
        Renderer();

        /**
         * Destructs a Renderer object.
         */
        virtual ~Renderer();

        /**
         * Retrieves the IIntegrator interface used for rendering.
         *
         * @return The IIntegrator interface used for rendering. The Renderer object owns this
         *   interface.
         */
        IIntegrator *GetIntegrator() const;

        /**
         * Sets the IIntegrator interface used for rendering.
         *
         * @param integrator The IIntegrator interface used for rendering. The Renderer object
         *   takes ownership of this interface.
         */
        void SetIntegrator(IIntegrator *integrator);

        /**
         * Retrieves the Accelerator object used for rendering.
         *
         * @return The Accelerator object used for rendering. The Renderer object owns the
         *   accelerator.
         */
        Accelerator *GetAccelerator() const;

        /**
         * Sets the Accelerator object used for rendering.
         *
         * @param accelerator The Accelerator object used for rendering. The Renderer object takes
         *   ownership of the accelerator.
         */
        void SetAccelerator(Accelerator *accelerator);

        /**
         * Renders an image of a scene.
         *
         * @param scene The scene
         * @param width The image width in pixels
         * @param height The image height in pixels
         * @return The rendered image or NULL if the rendering failed
         * @remarks You cannot use the same Renderer object to render different scenes
         *   simultaneously.
         */
        Image *Render(const Scenes::Scene &scene, int width, int height); 
    };
}

#endif // RAYTRACER_RENDERER_H

SimpleAccelerator.h

#ifndef RAYTRACER_SIMPLEACCELERATOR_H
#define RAYTRACER_SIMPLEACCELERATOR_H

#include <vector>

#include <Raytracer/Accelerator.h>
// #include <Raytracer/Ray.h>

namespace Raytracer
{
    class Ray;
    class RayHit;

    namespace Scenes
    {
        class Light;
        class PhysicalObject;
        class Scene;
        class SceneObject;
    }

    /**
     * A simple accelerator that uses brute force testing of all objects to find intersections.
     */
    class SimpleAccelerator : public Accelerator
    {
    private:
        /**
         * A list of all physical objects in the scene
         */
        std::vector<Scenes::PhysicalObject *> physicalObjects;

        std::vector<Scenes::Light *> lights;

        void AddObject(const Scenes::SceneObject *object);

    public:
        virtual const std::vector<Scenes::Light *> &GetLights() const;

        virtual bool HitTest(const Ray &ray, RayHit *hit) const;

        virtual void SetScene(const Scenes::Scene *scene);
    };
}

#endif // RAYTRACER_SIMPLEACCELERATOR_H

Phong Integrator.h

#ifndef RAYTRACER_PHONGINTEGRATOR_H
#define RAYTRACER_PHONGINTEGRATOR_H

#include <glm.hpp>

#include <Raytracer/IIntegrator.h>

namespace Raytracer
{
namespace Scenes
{
    class Scene;
}

/**
 * Calculates the visible color seen by a ray using the Phong illumination model.
 */
class PhongIntegrator : public IIntegrator
{
private:
    /**
     * The background color seen by rays that exit the scene
     */
    glm::vec3 backgroundColor;

public:
    /**
     * Constructs a new PhongIntegrator object.
     */
    PhongIntegrator();

    glm::vec3 GetColor(Ray &ray, const Accelerator &accelerator);
};
}

#endif // RAYTRACER_PHONGINTEGRATOR_H

Light.cpp

#include <Raytracer/Raytracer.h>

using namespace Raytracer::Scenes;

bool Light::IsInstanceOf(SceneObjectType type) const
{
    return (type == SceneObjectType_Light);
}

Light.h

#ifndef RAYTRACER_SCENES_LIGHT_H
#define RAYTRACER_SCENES_LIGHT_H

#include <glm.hpp>

#include <Raytracer/Intersection.h>
#include <Raytracer/Scenes/SceneObject.h>

namespace Raytracer
{
    struct Sample;

    namespace Scenes
    {
        class Scene;

        /**
         * A light source
         */
        class Light : public SceneObject
        {
        public:
            /**
             * Computes the direct contribution of this light in the illumination of a surface
             * point.
             *
             * @param accelerator An accelerator object that can be used to trace secondary rays
             *   through the scene
             * @param intersection An intersection of a view ray with an object
             * @return A color value representing the direct contribution
             */
            virtual glm::vec3 ComputeDirectContribution(const Accelerator *accelerator,
                const Intersection &intersection) = 0;

            virtual bool IsInstanceOf(SceneObjectType type) const;
        };
    }
}

#endif // RAYTRACER_SCENES_LIGHT_H

Scene.h

#ifndef RAYTRACER_SCENES_SCENE_H
#define RAYTRACER_SCENES_SCENE_H

#include <Raytracer/Scenes/SceneObject.h>

namespace Raytracer
{
    class Ray;

    namespace Scenes
    {
        class Camera;

        class Scene : public SceneObject
        {
        private:
            /**
             * The camera that is to be used for rendering the scene
             */
            Camera *activeCamera;

        public:
            /**
             * Constructs a new Scene object.
             */
            Scene();

            /**
             * Retrieves the active camera, i.e. the camera object to be used for rendering the
             * scene.
             *
             * @return The active camera or NULL if there is no active camera
             */
            Camera *GetActiveCamera() const;

            virtual bool IsInstanceOf(SceneObjectType type) const;

            /**
             * Sets the active camera, i.e. the camera object to be used for rendering the scene.
             *
             * @param camera The new active camera or NULL to set no active camera
             */
            void SetActiveCamera(Camera *camera);
        };
    }
}

#endif // RAYTRACER_SCENES_SCENE_H

Scene.cpp

#define RAYTRACER_USE_FOREACH
#include <Raytracer/Raytracer.h>

using namespace Raytracer;
using namespace Raytracer::Scenes;

Scene::Scene()
{
    activeCamera = NULL;
}

Camera *Scene::GetActiveCamera() const
{
    return activeCamera;
}

bool Scene::IsInstanceOf(SceneObjectType type) const
{
    return (type == SceneObjectType_Scene);
}

void Scene::SetActiveCamera(Camera *camera)
{
    this->activeCamera = camera;
}

Answer 1

You need to tell the old parent that this object is no longer its child, and that parent must then update its list of children to no longer contain a pointer to this object.

For example:

bool SceneObject::AddChild(SceneObject *child)
{   
    if (child == NULL) {
        return false;
    }
    else if (child->GetParent() != this) {
        child->parent->RemoveChild(child);
        child->parent = this;
        children.push_back(child);
        child->UpdateTransformations();
        return true;
    }
    else {
        this->children.push_back(child);
        child->parent = this;
        child->UpdateTransformations();
        return true;
    }
}

and

bool SceneObject::RemoveChild(SomeObject* child)
{
  auto iter = std::find(children.begin(), children.end(), child);
  if (iter == children.end())
    return false;
  else
  {
    children.erase(iter);
    return true;
  }
}

Answer 2

If the parent is supposed to own the children, I would change the member to be

std::vector<std::unique_ptr<SceneObject>> children;

Then when the SceneObject falls out of scope, it will make sure to implicitly delete each of the children without any further effort on your part.

So you could change the AddChild method to be

bool SceneObject::AddChild(std::unique_ptr<SceneObject> child)

and to call it

scene->AddChild(std::make_unique<PointLight>(vec3(10.0f)));

Inside the AddChild method you can pass the pointer on such that this now owns that pointer

children.push_back(std::move(child));