SFML community forums

General => SFML projects => Topic started by: BruceJohnJennerLawso on January 08, 2014, 10:29:38 pm

Title: Ignition Engine
Post by: BruceJohnJennerLawso on January 08, 2014, 10:29:38 pm
Hello!

I'm new to this forums, and a little green to SFML, but I wanted to post what I have so far in terms of a SFML engine. A lot of the code is from scratch, but a few parts like the camera class are based off of a tutorial I found on dreamincode:

Ignition engine:

#pragma once
#include <SFML\Graphics.hpp>
#include <exception>

////////////////////////////////////////////////////////////////////////////////////////////////
// Ignition 2D Engine v 1.01 ///////////////////////////////////////////////////////////////////
// Written in C++ & SFML by John Lawson ////////////////////////////////////////////////////////
////////////////////////////////////////////////////////////////////////////////////////////////


namespace Ignition
{       class Texture_Manager
        {       public:
                Texture_Manager();
                private:
                std::vector<sf::Texture*> Texture_list;
                public:
                void Add_texture(sf::Texture * iTexture);
                void Smooth_texture(unsigned int index, bool smooth);
            sf::Texture * Get_texture(unsigned int index);
                ~Texture_Manager();
        };

        class CSprite
        {       public:
                CSprite(sf::Texture * texture, float iox, float ioy);
                CSprite(sf::Texture * texture, float iox, float ioy, unsigned int a, unsigned int b, unsigned int c, unsigned int d);
                protected:
                sf::Sprite * Object_sprite;
                virtual void Frame(float dt);                           // Frame updater to be defined in derived classes.
                public:                                                                         // Similar virtual for checking whether the object is in       
                virtual bool In_render(sf::IntRect rBounds);    // the area to be drawn
                float Get_theta();
                sf::Vector2i Get_pos();
                float Get_x();
                float Get_y();
                void Set_theta(float itheta);
                void Set_position(float ix, float iy);
                void Colour_sprite(int red, int green, int blue, int alpha);
                void Colour_sprite(int red, int green, int blue);
                void Draw_sprite(int x, int y, sf::RenderWindow * rwindow);
                void Draw_sprite(sf::RenderWindow * rwindow);
                ~CSprite();    
        };

        class Camera
        {       public:
                Camera(unsigned int h, unsigned int w, unsigned int pofs_x, unsigned int pofs_y);
                Camera(unsigned int h, unsigned int w, unsigned int pofs_x, unsigned int pofs_y, CSprite * iTgt);
                Camera(unsigned int h, unsigned int w, float ispeed, unsigned int pofs_x, unsigned int pofs_y);
                Camera(unsigned int h, unsigned int w, float ispeed, unsigned int pofs_x, unsigned int pofs_y, CSprite * iTgt);
                void Translate_camera(int x,int y);
                void Translate_camera_center(int x, int y);
                void Camera_goto(int x,int y);                                                  // dont worry, not that kind of goto
                void Camera_center_goto(int x, int y);                                  // Sends the camera to a coordinate in "the world"
                void Update_camera();
                void Set_cam_tgt(sf::Vector2i iPos);
                sf::Vector2i* Get_cam_pos();
                sf::Vector2i* Get_cam_size();
                sf::Vector2i* Get_Cam_port_ofs();
                sf::IntRect Get_cam_window();
                CSprite * this_cTgt;
                private:
                sf::Vector2f * Cam_pos;
                sf::Vector2i * Cam_size;
                sf::Vector2f * Cam_tgt;
                sf::Vector2i * Cam_port_ofs;    // Offset from standard render position in the portal window
                float speed;                            // Not strictly necessary, but I may want the camera to have a "lag" motion behind the target, probably great for ConquerSpace
                public:                                         // velocity, speed, dammit. Must respect physics, augh!!!
                friend class Ignition_Engine;
                ~Camera();
        };

        class Portal
        {       public:
                Portal(unsigned int h, unsigned int w, int bpp, std::string wTitle);    // The cake is a lie, apparently.
                void Add_camera_view(Camera * iCam);    // Least, nobody ever gave me any for coding anything.
                sf::RenderWindow * pPortal;    
                std::vector<Camera*> Cam_views;
                public:
                ~Portal();                      // But this is a triumph :)
        };
       
        struct Portal_arg
        {       unsigned int h, w;
                int bpp;
                std::string ptitle;
        };

        class Ignition_Engine
        {       public:
                Ignition_Engine(void (*function)(Ignition_Engine* ithis));
                void Ignition(std::vector<std::string> Load_paths, unsigned int h, unsigned int w, int bpp, std::string wTitle);
                void Ignition(std::vector<std::string> Load_paths, std::vector<Portal_arg> Portal_args, unsigned int h, unsigned int w, int bpp, std::string wTitle);
            Texture_Manager Manager;
                std::vector<Camera*> Engine_cams;
                Portal * Main_portal;
                std::vector<Portal*> Sub_portals;
            void Load_texture(std::string file_path);
                void Register_sprite(CSprite * iSprite);             
                std::vector<CSprite*> Sprite_Objects;
                CSprite * eTgt;                 // The primary engine target (ie, usually the current player input that we want to follow);
                private:
                bool Start(unsigned int h, unsigned int w, int bpp, std::string wTitle);                                // Engine Start (The Engines are on!)          
                void Loop();                            // Main Loop()
                void Render_frame();            // {    Renders the graphical output
                void Process_input();           //              Handle user input
                void Update();                          //              Update program internals
                public:                                         // }
                ~Ignition_Engine(void);
        };
       
        void Construct_portal(unsigned int h, unsigned int w, int bpp, std::string wTitle, Ignition_Engine * this_engine);
}

#include "Ignition_Engine.h"
#include <math.h>
#include <vector>


// Ignition Engine ///////////////////////////////////////////////////////////
//////////////////////////////////////////////////////////////////////////////

Ignition::Ignition_Engine::Ignition_Engine(void (*function)(Ignition_Engine* ithis))
{       (*function)(this);      // Pass in the new arguments to the engine starting up
}

void Ignition::Ignition_Engine::Ignition(std::vector<std::string> Load_paths, unsigned int h, unsigned int w, int bpp, std::string wTitle)
{       if (!Start(h, w, bpp, wTitle))
        {       throw "Ignition_Failure";
        }
        else
        {       for (std::vector<std::string>::iterator it = Load_paths.begin(); it != Load_paths.end(); ++it)
                {       Load_texture(*it);
                }      
                Loop();
        }
}       // Lights this candle. Yeah baby. Yeah.

void Ignition::Ignition_Engine::Ignition(std::vector<std::string> Load_paths, std::vector<Portal_arg> Portal_args, unsigned int h, unsigned int w, int bpp, std::string wTitle)
{       if (!Start(h, w, bpp, wTitle))
        {       throw "Ignition_Failure";
        }
        else
        {       for (std::vector<std::string>::iterator it = Load_paths.begin(); it != Load_paths.end(); ++it)
                {       Load_texture(*it);
                }
                for (std::vector<Portal_arg>::iterator itArg = Portal_args.begin(); itArg != Portal_args.end(); ++itArg)
                {       Construct_portal(itArg->h, itArg->w, itArg->bpp, itArg->ptitle);
                }
                Loop();
        }
}       // Lights this candle. Yeah baby. Yeah.

void Ignition::Ignition_Engine::Load_texture(std::string file_path)
{       sf::Texture * Tex;      // Trustworthy files of course
        Tex = new sf::Texture();
        Tex->loadFromFile(file_path);
        Manager.Add_texture(Tex);
}       // Load a texture into the managed list

void Ignition::Ignition_Engine::Register_sprite(CSprite * iSprite)
{       Sprite_Objects.push_back(iSprite);
}       // Register a sprite... for iteration of members inheriting from parent class CSprite

bool Ignition::Ignition_Engine::Start(unsigned int h, unsigned int w, int bpp, std::string wTitle)
{       Main_portal = new Portal(Engine_cams, h, w, bpp, wTitle);
        if(!(Main_portal->pPortal))
        {       return false;
        }
        else
        {       return true;
        }
}

void Ignition::Ignition_Engine::Loop()
{       while(Main_portal->pPortal->isOpen())
        {       Process_input();
                Update();
                Render_frame();
        }
}

void Ignition::Ignition_Engine::Render_frame()
{       for (std::vector<Portal*>::iterator it = Sub_portals.begin(); it != Sub_portals.end(); ++it)
        {       (*it)->pPortal->clear();
        }       Main_portal->pPortal->clear();
        sf::Vector2i * rend_ofs;
        for (std::vector<Portal*>::iterator itPortal = Sub_portals.begin(); itPortal != Sub_portals.end(); ++itPortal)
        {       for (std::vector<Camera*>::iterator itCamera = (*itPortal)->Cam_views.begin(); itCamera != (*itPortal)->Cam_views.end(); ++itCamera)
                {       if(!((*itCamera)->Cam_tgt))
                        {       (*itCamera)->this_cTgt = eTgt;
                                (*itCamera)->Cam_tgt = new sf::Vector2f(eTgt->Get_pos());
                        }
                        for (std::vector<CSprite*>::iterator itSprites = Sprite_Objects.begin(); itSprites != Sprite_Objects.begin(); ++itSprites)
                        {       if ((*itSprites)->In_render((*(*itCamera)->Get_cam_window())) == true)
                                {       rend_ofs = new sf::Vector2i( *((*itCamera)->Get_Cam_port_ofs()) + ((*itSprites)->Get_pos()) );
                                        (*itSprites)->Draw_sprite((rend_ofs)->x, (rend_ofs)->y, (*itPortal)->pPortal);
                                        delete  rend_ofs;
                                }      
                        }
                }
        }
}

void Ignition::Ignition_Engine::Process_input()
{       sf::Event Input_Events;
        while(Main_portal->pPortal->pollEvent(Input_Events))
        {       if (Input_Events.type == sf::Event::Closed)
                {       this->~Ignition_Engine();
                }
        }
}

void Ignition::Ignition_Engine::Update()
{
}

Ignition::Ignition_Engine::~Ignition_Engine(void)
{       for (std::vector<CSprite*>::iterator it = Sprite_Objects.begin(); it != Sprite_Objects.end(); ++it)
        {       delete *it;
        }       Sprite_Objects.clear();
        Manager.~Texture_Manager();
        for (std::vector<Portal*>::iterator itPortal = Sub_portals.begin(); itPortal != Sub_portals.end(); ++itPortal)
        {       delete (*itPortal);
        }       delete Main_portal;
}




// Ignition::Texture manager class ///////////////////////////////////////////
//////////////////////////////////////////////////////////////////////////////

Ignition::Texture_Manager::Texture_Manager()
{
}

void Ignition::Texture_Manager::Add_texture(sf::Texture * iTexture)
{       Texture_list.push_back(iTexture);
}

void Ignition::Texture_Manager::Smooth_texture(unsigned int index, bool smooth)
{       Texture_list[index]->setSmooth(smooth);
}

sf::Texture * Ignition::Texture_Manager::Get_texture(unsigned int index)
{       return Texture_list[index];    
}

Ignition::Texture_Manager::~Texture_Manager()
{       for(std::vector<sf::Texture*>::iterator it = Texture_list.begin(); it != Texture_list.end(); ++it)
        {       delete *it;
        }       Texture_list.clear();
}




// Ignition::Sprite parent class /////////////////////////////////////////////
//////////////////////////////////////////////////////////////////////////////

Ignition::CSprite::CSprite(sf::Texture * texture, float iox, float ioy)
{       Object_sprite = new sf::Sprite();
        Object_sprite->setTexture(*texture);
        Object_sprite->setOrigin(iox, ioy);     // Reset the origin by given coordinates
}       // Constructs the Tile object and assigns it the texture that it uses

Ignition::CSprite::CSprite(sf::Texture * texture, float iox, float ioy, unsigned int a, unsigned int b, unsigned int c, unsigned int d)
{       Object_sprite->setTexture(*texture);
        Object_sprite->setTextureRect(sf::IntRect(a, b, c, d)); // Explicitly defining the texture coordinates of the sprite in the given texture
        Object_sprite->setOrigin(iox, ioy);     // Reset the origin by given coordinates
}       // Constructs the Tile object and assigns it the texture that it uses

float Ignition::CSprite::Get_theta()
{       return (Object_sprite->getRotation());
}       // Get the rotation of the sprite as float.

sf::Vector2i Ignition::CSprite::Get_pos()
{       sf::Vector2i pos((int)Get_x(),(int)Get_y());
        return pos;
}

float Ignition::CSprite::Get_x()
{       return (Object_sprite->getPosition().x);
}

float Ignition::CSprite::Get_y()
{       return (Object_sprite->getPosition().y);
}

void Ignition::CSprite::Set_theta(float itheta)
{       Object_sprite->setRotation(itheta);
}

void Ignition::CSprite::Set_position(float ix, float iy)
{       Object_sprite->setPosition(ix, iy);
}

void Ignition::CSprite::Colour_sprite(int red, int green, int blue, int alpha)
{       Object_sprite->setColor(sf::Color(red, green, blue, alpha));
}

void Ignition::CSprite::Colour_sprite(int red, int green, int blue)
{       Object_sprite->setColor(sf::Color(red, green, blue));
}

void Ignition::CSprite::Draw_sprite(int x, int y, sf::RenderWindow * rwindow)
{       Object_sprite->setPosition(x, y);
    rwindow->draw(*Object_sprite);
}

void Ignition::CSprite::Draw_sprite(sf::RenderWindow * rwindow)
{       rwindow->draw(*Object_sprite);
}

Ignition::CSprite::~CSprite()
{       delete Object_sprite;
}



// Ignition::Portal class ////////////////////////////////////////////////////
//////////////////////////////////////////////////////////////////////////////

Ignition::Portal::Portal(unsigned int h, unsigned int w, int bpp, std::string wTitle)
{       pPortal = new sf::RenderWindow(sf::VideoMode(w, h, bpp), wTitle);
}

void Ignition::Portal::Add_camera_view(Camera * iCam)
{       Cam_views.push_back(iCam);
}

Ignition::Portal::~Portal()
{       Cam_views.clear();
        pPortal->close();
        delete pPortal;
}      




// Ignition::Camera class ////////////////////////////////////////////////////
//////////////////////////////////////////////////////////////////////////////

Ignition::Camera::Camera(unsigned int h, unsigned int w, unsigned int pofs_x, unsigned int pofs_y)
{       Cam_size = new sf::Vector2i(w, h);
        Cam_pos = new sf::Vector2f(0.000, 0.000);
        Cam_port_ofs = new sf::Vector2i(pofs_x, pofs_y);
        speed = 1.000;
}

Ignition::Camera::Camera(unsigned int h, unsigned int w, unsigned int pofs_x, unsigned int pofs_y, CSprite * iTgt)
{       Cam_size = new sf::Vector2i(w, h);
        Cam_pos = new sf::Vector2f(0.000, 0.000);
        Cam_port_ofs = new sf::Vector2i(pofs_x, pofs_y);
        speed = 1.000;
        this_cTgt = iTgt;
        Cam_tgt = new sf::Vector2f((this_cTgt->Get_pos()));
}

Ignition::Camera::Camera(unsigned int h, unsigned int w, float ispeed, unsigned int pofs_x, unsigned int pofs_y)
{       Cam_size = new sf::Vector2i(w, h);
        Cam_pos = new sf::Vector2f(0.000, 0.000);
        Cam_port_ofs = new sf::Vector2i(pofs_x, pofs_y);
        speed = ispeed;
}

Ignition::Camera::Camera(unsigned int h, unsigned int w, float ispeed, unsigned int pofs_x, unsigned int pofs_y, CSprite * iTgt)
{       Cam_size = new sf::Vector2i(w, h);
        Cam_pos = new sf::Vector2f(0.000, 0.000);
        Cam_port_ofs = new sf::Vector2i(pofs_x, pofs_y);
        speed = ispeed;
        this_cTgt = iTgt;
        Cam_tgt = new sf::Vector2f((this_cTgt->Get_pos()));
}

void Ignition::Camera::Translate_camera(int x, int y)
{       Cam_pos->x = (float)x;
        Cam_pos->y = (float)y;
        Cam_tgt = Cam_pos;
}

void Ignition::Camera::Translate_camera_center(int x, int y)
{       x -= ((Cam_size->x)/(2));
        y -= ((Cam_size->y)/(2));
        Cam_pos->x = (float)x;
        Cam_pos->y = (float)y;
        Cam_tgt = Cam_pos;
}

void Ignition::Camera::Camera_goto(int x, int y)
{       Cam_tgt->x = (float)x;
        Cam_tgt->y = (float)y;
}

void Ignition::Camera::Camera_center_goto(int x, int y)
{       x -= ((Cam_size->x)/(2));
        y -= ((Cam_size->y)/(2));
        Cam_tgt->x = x;
        Cam_tgt->y = y;
}

void Ignition::Camera::Update_camera()
{       struct coords
        {       float l, dl;
        }x, y, d;
        x.l = (float)(Cam_tgt->x - Cam_pos->x);
        y.l = (float)(Cam_tgt->y - Cam_pos->y);
        if (d.l <= 1)
        {       Cam_pos = Cam_tgt;
        }       // The snap-to case when the locations are close enough. Not really sure if this is worth it, but maybe overshooting is a problem otherwise
        else
        {       d.l = sqrt((pow(x.l, 2))+(pow(y.l, 2)));
                d.dl = (((d.l)*(speed))/60);
                if (d.dl < 1.0f)
                {       d.dl = 1.0f;
                }
                x.dl = ((x.l)*(d.dl/d.l));              // Similar triangles method
                y.dl = ((y.l)*(d.dl/d.l));              // to get v components
                Cam_pos->x += x.dl;
                Cam_pos->y += y.dl;
        }
}

void Ignition::Camera::Set_cam_tgt(sf::Vector2i iPos)
{       Cam_tgt->x = iPos.x;
        Cam_tgt->y = iPos.y;
}

sf::Vector2i* Ignition::Camera::Get_cam_pos()
{       sf::Vector2i * output = new sf::Vector2i((int)((Cam_pos)->x),(int)((Cam_pos)->y));
        return output;
}

sf::Vector2i* Ignition::Camera::Get_cam_size()
{       return Cam_size;
}

sf::Vector2i* Ignition::Camera::Get_Cam_port_ofs()
{       return Cam_port_ofs;
}

sf::IntRect Ignition::Camera::Get_cam_window()
{       sf::IntRect cWindow((Cam_pos->x),(Cam_pos->y),(Cam_size->x),(Cam_size->y));
        return cWindow;
}

Ignition::Camera::~Camera()
{       delete Cam_tgt;
        delete Cam_size;
        delete Cam_pos;
        delete Cam_port_ofs;
}




// Ignition Engine - Functions() /////////////////////////////////////////////
//////////////////////////////////////////////////////////////////////////////

void Ignition::Construct_portal(unsigned int h, unsigned int w, int bpp, std::string wTitle, Ignition_Engine * this_engine)
{       Portal * iPort = new Portal(h, w, bpp, wTitle);
        this_engine->Sub_portals.push_back(iPort);
}

Any thoughts on the progress so far are welcome :)
Title: Re: Ignition Engine
Post by: Grimshaw on January 08, 2014, 10:41:15 pm
I think its more valuable to post small videos and demonstrations of what you can do with it, rather than posting all that code! But by all means keep working ^^

Its like, no one wants to look at an engine of a car, without the actual car it goes inside :p
Title: Re: Ignition Engine
Post by: BruceJohnJennerLawso on January 08, 2014, 10:47:26 pm
Fair enough, I just wanted to see if anyone had any thoughts on the quality of the work so far. Most of it I feel confident about, but its still untested. I'll see if I can get something running in a little bit.
Title: Re: Ignition Engine
Post by: Nexus on January 08, 2014, 10:54:10 pm
Since you only show code, that's all we can judge -- and to be honest, there are several flaws: Memory leaks, unnecessary copies, no use of RAII (why don't you copy 2D vectors?), ignoring the rule of three, hungarian notation, inconsistent naming, code duplication...

I recommend to have a look at a list of C++ advice (http://en.sfml-dev.org/forums/index.php?topic=13977.msg97950#msg97950) I recently put together. By reading the tips, you can improve your code a lot.
Title: Re: Ignition Engine
Post by: BruceJohnJennerLawso on January 08, 2014, 11:00:33 pm
Leaks? Where? I thought I had that covered completely, but apparently not.

Thanks for the link, will read  :)
Title: Re: Ignition Engine
Post by: Nexus on January 08, 2014, 11:08:33 pm
The caller has to invoke delete when using your getters. That's never a good idea.

If you have to think whether you have covered leaks, then you have already done something wrong. The intent behind RAII is to automate memory management completely, so that the programmer doesn't have to wrap his head around it and can instead focus on important things.
Title: Re: Ignition Engine
Post by: BruceJohnJennerLawso on January 08, 2014, 11:20:24 pm
Ack, yes

How does this sound:

sf::Vector2i Ignition::Camera::Get_cam_size()
{       return *Cam_size;
}
Title: Re: Ignition Engine
Post by: Nexus on January 09, 2014, 12:24:13 am
The return type is better, but the member variable is still bad. There is no reason to use pointers in the first place. And your method should be const-qualified.

Please read about RAII (e.g. on Wikipedia) and automatic memory management, it's a crucial part of C++.
Title: Re: Ignition Engine
Post by: BruceJohnJennerLawso on January 10, 2014, 10:22:13 pm
Reading up about RAII, but very confused about mutex. I kinda get what std::unique_ptr does by self deleting once out of scope, but mutex locks something then releasing what... ???  :o

Is there anything that can be done to salvage the engine at this point, or is it a lost cause? I would really like to get it to a marginally useable state in order to play with it just once.
Title: Re: Ignition Engine
Post by: Grimshaw on January 11, 2014, 04:50:55 am
Please try not to overuse the word game engine :p Right now its just a couple of files, I am sure you can refactor whats needed and make it smooth! Keep learning and improving it or start again if you prefer. No big losses yet! :)
Title: Re: Ignition Engine
Post by: Matt Guerrette on January 11, 2014, 06:14:29 am
Just some input. You should really be using const qualified getters. something like

const sf::Vector2i& Ignition::Camera::Get_cam_size() const
{  
     return *Cam_size;
}
 

Thats just my two cents. That way the return value cannot be modified by the user and your intentions are clear as to what this getter is for. *im assuming just to return the size for info not for modification*

Using pointers is dangerous, so I would recommend return by const correct reference wherever you can and think it would make since.

Write set methods for actually setting the values.

Also, i noticed this line

Manager.~Texture_Manager();

Just call delete...or use
std::uniqure_ptr<Texture_Manager> Manager
and it will
auto delete when out of scope.

Here put this namespace somewhere in your engine for use. Its what I use for memory management of
pointers in replace of delete

namespace Memory
{
        //Due to comments below, I've removed the check for value before delete
        template <class T> void SafeDelete(T& t)
        {
                delete t;
                t = nullptr;
        }

       
        template <class T> void SafeRelease(T& t)
        {
                if(t)
                {
                        t->Release();
                        t = nullptr;
                }
        }
}



There is more, but i think that will give you a start. Cheers!
Title: Re: Ignition Engine
Post by: Nexus on January 11, 2014, 02:51:39 pm
I don't recommend the SafeDelete() function template, because it's a weak solution to a non-existing problem and because it hides logic errors. If you attempt to delete a pointer multiple times, that's a bug which should be noticed, not ignored. Plus, the if (t) has absolutely no effect.

Anyway, I don't see a reason why you don't use std::unique_ptr -- it's almost always superior to new and delete. Even in the case of Release() you can construct a custom deleter and thereby apply the RAII idiom.
Title: Re: Ignition Engine
Post by: Matt Guerrette on January 11, 2014, 05:55:11 pm
I don't recommend the SafeDelete() function template, because it's a weak solution to a non-existing problem and because it hides logic errors. If you attempt to delete a pointer multiple times, that's a bug which should be noticed, not ignored. Plus, the if (t) has absolutely no effect.

Anyway, I don't see a reason why you don't use std::unique_ptr -- it's almost always superior to new and delete. Even in the case of Release() you can construct a custom deleter and thereby apply the RAII idiom.

Exactly, that why i specifically said "in replace of delete", it would be much preferred he use smart pointers seeing as its 2014 now...not 1995.

Also, just so you know. It is completely safe to delete a nullptr object from what I can tell. Multiple deletions of a nullptr will do nothing. It is not an error in modern c++.
Title: Re: Ignition Engine
Post by: FRex on January 11, 2014, 06:39:32 pm
Quote
Also, just so you know. It is completely safe to delete a nullptr object from what I can tell. Multiple deletions of a nullptr will do nothing. It is not an error in modern c++.
We know but the question is why do you post code that checks for null before deleting?
Title: Re: Ignition Engine
Post by: Matt Guerrette on January 11, 2014, 07:10:01 pm
Quote
Also, just so you know. It is completely safe to delete a nullptr object from what I can tell. Multiple deletions of a nullptr will do nothing. It is not an error in modern c++.
We know but the question is why do you post code that checks for null before deleting?

Just a habit I picked up when I started learning C++ a while back. My background is in DX programming and you'll find rather quickly why that matters. Most of the resources for learning DirectX from the past decade used either the template functions I posted, or a SAFE_DELETE macro such as this

#define SAFE_DELETE(x) { if(x) delete x; x=NULL; }

These books are written by professionals who coded long before smart pointers, and it was a time where deleting a null value WAS actually dangerous and ill advised hence the check. Nowadays the C++ standard removes need for unnecessary checks.

I only keep these template functions around for DX programming because from what I can tell, it is not safe to use C++11 smart pointers due to reference counting issues, and I don't want to clutter my code with both C++11 smart pointers and COM smart pointers.


That is all. Sorry I wasn't clear before.
Title: AW: Re: Ignition Engine
Post by: eXpl0it3r on January 11, 2014, 08:09:36 pm
I only keep these template functions around for DX programming because from what I can tell, it is not safe to use C++11 smart pointers due to reference counting issues, and I don't want to clutter my code with both C++11 smart pointers and COM smart pointers.
The COM objects have their own reference counting, that's why a C++11 smart pointer can't be used directly. There are apparently ways, but why make it complicated if there are already dedicated "smart pointers"/RAII containers for it?

You should whatever is appropriate for the givrn situation, however excluding one because the others gets used doesn't make sense.
Use the COM smart pointer for COM objects and the C++11 smart pointers where ever there's no dedicated RAII mechanism. ;)

As for the topic, the process of learning to program, requires that a lot of code gets thrown away and rewritten newly. Refactoring is a must if you want to get a better programmer. This doesn't mean you can't reuse a lot of already written code, but it means one has to make fundamental changes. ;)
Title: Re: Ignition Engine
Post by: Nexus on January 12, 2014, 02:12:19 pm
Nowadays the C++ standard removes need for unnecessary checks.
Not nowadays, but since more than one and a half decade. C++ was standardized in 1998.

I only keep these template functions around for DX programming because from what I can tell, it is not safe to use C++11 smart pointers due to reference counting issues
Written in such a general way, that's wrong. I don't see a problem with reference counting, and not all smart pointers use it anyway. std::unique_ptr is a simple pointer wrapper that is just as fast as performing manual new/delete, and it's faster and much safer than your SafeDelete() function.

"Safe delete" is an abomination which inspired a lot of C++ developers to write questionable code, and unfortunately it has survived a long time, even though the original problems have disappeared long ago. One could even call it an anti-idiom; there's absolutely no sane reason to use it. This has nothing to do with C++11, smart pointers and RAII have been around for much longer (boost::scoped_ptr, std::auto_ptr). Even if those are not ideal nowadays and std::unique_ptr should be preferred, they're still much better than any way of managing memory or other resources manually.

If you're interested, read the RAII thread (http://en.sfml-dev.org/forums/index.php?topic=9359.msg63566), we all tried to explain why RAII is such a powerful idiom that should be applied wherever possible.
Title: Re: Ignition Engine
Post by: Matt Guerrette on January 13, 2014, 10:08:26 am
Nowadays the C++ standard removes need for unnecessary checks.
Not nowadays, but since more than one and a half decade. C++ was standardized in 1998.

I only keep these template functions around for DX programming because from what I can tell, it is not safe to use C++11 smart pointers due to reference counting issues
Written in such a general way, that's wrong. I don't see a problem with reference counting, and not all smart pointers use it anyway. std::unique_ptr is a simple pointer wrapper that is just as fast as performing manual new/delete, and it's faster and much safer than your SafeDelete() function.

"Safe delete" is an abomination which inspired a lot of C++ developers to write questionable code, and unfortunately it has survived a long time, even though the original problems have disappeared long ago. One could even call it an anti-idiom; there's absolutely no sane reason to use it. This has nothing to do with C++11, smart pointers and RAII have been around for much longer (boost::scoped_ptr, std::auto_ptr). Even if those are not ideal nowadays and std::unique_ptr should be preferred, they're still much better than any way of managing memory or other resources manually.

If you're interested, read the RAII thread (http://en.sfml-dev.org/forums/index.php?topic=9359.msg63566), we all tried to explain why RAII is such a powerful idiom that should be applied wherever possible.

Again I would highly advise against using std::unique_ptr with DirectX COM objects.
http://msdn.microsoft.com/en-us/library/hh279683.aspx
illustrates the idea.

But to each his own I guess. I've already read up on RAII, and yes it can be a great idiom, but it really does depend on your target platform.

For example, I know some developers tend to stay away from RAII designs for embedded systems due to exception overhead.

Also, please stop re-iterating what I clearly already know, and have stated my reasons for keeping it around. Its not going to change the templates, really. Thanks!  :D
Title: Re: Ignition Engine
Post by: eXpl0it3r on January 13, 2014, 10:56:56 am
Written in such a general way, that's wrong. I don't see a problem with reference counting, and not all smart pointers use it anyway. std::unique_ptr is a simple pointer wrapper that is just as fast as performing manual new/delete
I think there could be a way to apply RAII to the whole COM stuff with unique_ptr and custom deleters, but I don't have any experience with it. From Microsoft however it's clear that they recommend using their COM smart pointers (see also here (http://msdn.microsoft.com/en-us/library/hh279674.aspx)).

But to each his own I guess. I've already read up on RAII, and yes it can be a great idiom, but it really does depend on your target platform.

For example, I know some developers tend to stay away from RAII designs for embedded systems due to exception overhead.
Yes there are always exceptions to "rules", but exceptions are called exceptions because they are being used only in rare cases. Thus, avoiding RAII just because of such a possible exception is a bit ignorant to all the other cases. It's a bit like avoiding flying (and instead driving by car), because planes can crash, while ignoring the fact that chances for a plane crash are way lower than chances of having a car accident. ;)

Also, please stop re-iterating what I clearly already know, and have stated my reasons for keeping it around. Its not going to change the templates, really.
Fair enough, it's your code, so you can do with it whatever you want, but in return please stop recommending your personal hacks to the public, since you seem to know the things we're discussing here and know that RAII and a like are actually better and SAFE_DELETE is just a useless legacy thing. :)
Title: Re: Ignition Engine
Post by: Nexus on January 13, 2014, 06:57:55 pm
Sorry if you misunderstood that, I only meant it as good advice. You have made several statements where the exact opposite is true ("SAFE_DELETE is a good technique", "smart pointers are unsafe", "they incur always overhead", "exceptions are a reason not to use RAII").

To sum up: std::unique_ptr is exactly as efficient as plain new/delete. But it's safer and leads to less code, making your life easier.
Title: Re: Ignition Engine
Post by: BruceJohnJennerLawso on January 15, 2014, 04:32:56 am
But automatic memory management is supposed to add some performance overhead when used, right? As I understood it, the only issue with C# is that the automated resource cleanup is automated, & a wee bit slower as a result?
Title: Re: Ignition Engine
Post by: Nexus on January 15, 2014, 09:29:51 am
But automatic memory management is supposed to add some performance overhead when used, right?
No, wrong. That's what I'm trying to explain all the time.

Look at a simple RAII example (ignore rule of three for the moment):
template <typename T>
struct SmartPtr
{
    explicit SmartPtr(T* raw) : ptr(raw) {}
    ~SmartPtr() { delete ptr; }

    T* ptr;
}

There is absolutely no reason why SmartPtr<X> p(new X); should be slower than X* p = new X; delete p;. All the functions can be inlined and the object consists only of the pointer, so the compiler will generate the same code for both. This is how std::unique_ptr works. std::shared_ptr is a different story, it is slower because of reference counting and thread safety.

As I understood it, the only issue with C# is that the automated resource cleanup is automated, & a wee bit slower as a result?
Garbage collection is not RAII. GCs indeed incur overhead, but they shine in situations where a lot of allocations have to be done, or where they can collect long-time information about memory usage.
Title: Re: Ignition Engine
Post by: BruceJohnJennerLawso on January 16, 2014, 04:30:02 pm
Ah, that is good then. I guess you now have a new convert to smart pointers  :D

So if I define a smart pointer in a class, I need to initialize it in the constructor or some other method, and the pointer gets deleted when its scope ends, presumably the destructor of that object?

I assume rule of three means that I can only share the pointer between three different objects?
Title: Re: Ignition Engine
Post by: eXpl0it3r on January 16, 2014, 04:51:33 pm
So if I define a smart pointer in a class, I need to initialize it in the constructor or some other method, and the pointer gets deleted when its scope ends, presumably the destructor of that object?
I hope you're not talking about the smart pointer given by Nexus, because that's just an example OF a smart pointer, it doesn't show the USAGE of smart pointers.
A smart pointer can either point to a valid object or nullptr. If you initialize the smart pointer in the initialization list, it will directly point to an object, but you can also initialize it at a later point, but you'd have to make sure that the content is not a nullptr when you try to access the data.
For a unique_ptr the resource/memory gets freed as soon as the pointer runs out of scope. For shared_ptr the resource/memory gets freed only after all shared_ptr that reference the same object go out of scope.

I assume rule of three means that I can only share the pointer between three different objects?
No, he meant this (https://en.wikipedia.org/wiki/Rule_of_three_(C%2B%2B_programming)).