Climbing out of Pointer Hell

The Partridge Family were neither partridges nor a family. Discuss.
GracefulComet
Posts: 12
Joined: July 16th, 2017, 9:49 pm

Climbing out of Pointer Hell

Post by GracefulComet » July 24th, 2017, 4:50 pm

So im trying to update some code in an attempt to fix a memory leak i introduced( real bad i know)
after failing to find a normal solution with new and delete I found something called smart pointers
so i thought maybe those could help me fix this memory leak. i might have more than one :(

The code i started refactoring first is a bit complex( for me atleast)
Its a Messaging system to enable communication between different parts of code
it utilizes Polymorphism with a base msg class and will have multiple derivatives (such as PhysicsMSG or AnimationMSG)

it worked pretty great at first i thought. there is a MSGdispatcher class and a MSGreciever class.
Message receiver has a std::vector <msg*> and a getMSG(msg* message) function
the Message Dispatcher has a std::vector<MSGreciever*> sendMSG(msg* message) that calls getMSG and passes along message to objects who have a matching Identifier.

at first it seem like it was working perfectly. i made a pair of objects to communicate with each other. one that handled behaviour, and connected that to my sprite.
but when i made more than one pair of objects it stopped working right. all kinds of bad memory stuff started happening. heap corruption, de-reference null_ptrs and all kinds of stuff.

i started at first with just messages befor i change anything else. and now im telling it to use std::shared_ptr<msg> instead of msg.
after lots of researching and researching and eliminating error messages i have just 4 instances of 1 error in particular

Compiler Error C2259 : You cannot instantiate a class or structure with one or more pure virtual functions. To instantiate objects of a derived class, the derived class must override each pure virtual function.

but as far as i can tell i have overrode all virtual functions. so since this is my first time working with smart ptrs im not sure what i am doing wrong. I will link below the code i find relevant.

Message headers

Code: Select all

class msg{
public:
	msg();
	virtual~msg() = default;
	msg(ID target, ID sender);
    virtual void update(void * Variables)=0;

	ID getTargetID();
	ID getSenderID();
	ID m_TargetID;
	ID m_SenderID;
    MSGTYPE m_type;

};

class PhysicsMSG : public msg {
	public:
	
	PhysicsMSG();
	~PhysicsMSG() = default;	
    PhysicsMSG( Vec2DF velocity , float dt,  ID target, ID Sender  );
	PhysicsMSG(PhysicsMSG& Copy);
	void update (void* Variables)override;
    Vec2DF m_VecPayload;

	float m_DeltaTime;
	

};

class AnimationMSG : public msg{

public:

	AnimationMSG();
    ~AnimationMSG() = default;
    AnimationMSG(int curTile, int AnimateFrames, state desiredState , ID target, ID Sender );
	AnimationMSG(AnimationMSG& Copy);
    void update(void* Variables)override;
    int m_curFrameSet;
    int m_AnimationFramesSet;
    state m_state;
};

class MSGreciever{

	public:
	MSGreciever();
	MSGreciever(ID identity);
	~MSGreciever();
    bool handleMSG(void *passedvar);
    void handleMSGS(void *passedvar);
    void getMSGS(std::shared_ptr<msg> message );
    MSGTYPE peakatMSGS(int indextoPeak);
	void CheckForGarbage();



    std::vector < std::shared_ptr<msg> > que;
    ID SentTo;

};

class MSGdispatcher {

	public:
	MSGdispatcher();
        MSGdispatcher( MSGreciever* FirstTarget );
	~MSGdispatcher();
//	void sendMSG(msg Message);
	void registerMSGER (MSGreciever* listener );
    void sendMSG(std::shared_ptr<msg> Message);



	std::vector<MSGreciever*> m_Listeners;
};

};
Message .cpp implementation

Code: Select all

msg::msg()
{
}

msg::msg(ID target, ID sender)
{  
	m_TargetID = target;
	m_SenderID = sender; 
	m_type = MSGTYPE::Failed;
}


ID msg::getTargetID(){return m_TargetID;}

ID msg::getSenderID()
{
	return m_SenderID;
}



PhysicsMSG::PhysicsMSG(){

}




PhysicsMSG::PhysicsMSG(Vec2DF velocity, float dt, ID target, ID Sender ){

	m_VecPayload = velocity;
	m_type = MSGTYPE::Physics;
    m_TargetID = target;
	m_SenderID = Sender;
	m_DeltaTime = dt;	

}

PhysicsMSG::PhysicsMSG(PhysicsMSG & Copy)
{
	this->m_VecPayload = Copy.m_VecPayload;
	this->m_type = Copy.m_type;
	this->m_TargetID = Copy.m_TargetID;
	this->m_SenderID = Copy.m_SenderID;
	this->m_DeltaTime = Copy.m_DeltaTime;
}

void PhysicsMSG::update( void* Variables ){
    if((Variables) == nullptr){
    std::cout << " Got Null" <<std::endl;
    }else{

    ((Vec2DF* )Variables )->x = m_VecPayload.Approach(m_VecPayload.x,((Vec2DF*)Variables)->x, ( m_DeltaTime  * 0.15f) );
    ((Vec2DF* )Variables )->y = m_VecPayload.Approach(m_VecPayload.y,((Vec2DF*)Variables)->y, ( m_DeltaTime ) * 0.15f);
  ((Vec2DF*)Variables)->operator+= (m_VecPayload *= 0.15f );
    }
}


AnimationMSG::AnimationMSG()
{
}



AnimationMSG::AnimationMSG(int curTile, int AnimateFrames , state desiredState, ID target, ID Sender ){
    m_type = MSGTYPE::Animation;
    m_curFrameSet = curTile;
    m_AnimationFramesSet = AnimateFrames;
    m_TargetID = target;
	m_SenderID = Sender;
    m_state = desiredState;
}
AnimationMSG::AnimationMSG(AnimationMSG & Copy)
{
	this->m_type = Copy.m_type;
	this->m_curFrameSet = Copy.m_curFrameSet;
	this->m_AnimationFramesSet = Copy.m_AnimationFramesSet;
	this->m_TargetID = Copy.m_TargetID;
	this->m_SenderID = Copy.m_SenderID;
	this->m_state = Copy.m_state;

}
void  AnimationMSG::update(void* Variables){
    if((Variables) == nullptr){
    std::cout << " Got Null" <<std::endl;
    }else{

    ((TileMap*)Variables)->setCurTile(m_curFrameSet);
    ((TileMap*)Variables)->setNumAnimFrames(m_AnimationFramesSet);
    ((TileMap*)Variables)->setState(m_state);

    }

}


MSGdispatcher::MSGdispatcher(){}
MSGdispatcher::MSGdispatcher( MSGreciever* FirstTarget ){

	registerMSGER(FirstTarget);

}
MSGdispatcher::~MSGdispatcher(){}

MSGreciever::MSGreciever(){}

MSGreciever::MSGreciever(ID Owner ){
SentTo = Owner; 
que.clear();
}
MSGreciever::~MSGreciever(){}

void MSGdispatcher::sendMSG(std::shared_ptr<msg> Message){
	for (auto i = 0; i < m_Listeners.size() ; i++){
	m_Listeners[i]->getMSGS(Message);
	}
}

void MSGdispatcher::registerMSGER(MSGreciever* listener ){
m_Listeners.push_back(listener);
}

void MSGreciever::getMSGS(std::shared_ptr<msg> message) {
	que.push_back(message);
}

MSGTYPE MSGreciever::peakatMSGS(int indextoPeak){

if(indextoPeak < que.size()){
   return que[indextoPeak]->m_type;
          }
}



void MSGreciever::handleMSGS(void * passedvar){

    for (unsigned int i =0; i < que.size(); i++){
    if(  SentTo.matchMyID( que[i]->getTargetID()) == true )	{
        que[i]->update(passedvar);
        }
    }
    if ( que.empty() == false){
	
    que.pop_back();
    }
}

bool MSGreciever::handleMSG(void * passedvar){

    if(que.empty() == false){
        if(SentTo.matchMyID( que[que.size()]->getTargetID()) == true){
            que[que.size()]->update(passedvar);
			 que.pop_back();

        }
 
    }
return que.empty();
}




and the allocation code inside of the behaviour class looks like
m_mailman.sendMSG is a Message Dispatcher
i have tried useing std::make_shared<msg>( new PhysicsMSG(...)) and that didnt work either

Code: Select all

	m_mailman.sendMSG(std::make_shared<msg>(PhysicsMSG(Velocity, m_time.getDelta(), m_Target, m_Mine)));
    m_mailman.sendMSG(std::make_shared<msg>(AnimationMSG(15, 3, state::animated, m_Target, m_Mine)));

PS: if it looks like i dont know what i am doing. i feel like i don't either.
Attachments
2017-07-24.png
Screenshot of the Errors
(203.42 KiB) Not downloaded yet
GE.zip
To compile you need to:
open .sln
successfully compile(not working right now) RelWithDebugInfo x64 build
copy all files from inside "lib/x64" into "RelWithDebugInfo" folder
copy Assets folder into "RelWithDebugInfo"
maybe copy the Background.sp and player.sp into RelWithDebugInfo? that one isnt always required.
(9.24 MiB) Downloaded 132 times

albinopapa
Posts: 4373
Joined: February 28th, 2013, 3:23 am
Location: Oklahoma, United States

Re: Climbing out of Pointer Hell

Post by albinopapa » July 24th, 2017, 7:06 pm

Didn't catch this first few times around, but you SHOULD be doing std::make_shared<ChildType> instead of std::make_shared<ParentType>.

for example

Code: Select all

	m_mailman.sendMSG( std::make_shared<PhysicsMSG>( Velocity, m_time.getDelta(), m_Target, m_Mine ) );
	m_mailman.sendMSG( std::make_shared<AnimationMSG>( 3, 3, state::animated, m_Target, m_Mine ) );
If you think paging some data from disk into RAM is slow, try paging it into a simian cerebrum over a pair of optical nerves. - gameprogrammingpatterns.com

GracefulComet
Posts: 12
Joined: July 16th, 2017, 9:49 pm

Re: Climbing out of Pointer Hell

Post by GracefulComet » July 24th, 2017, 7:27 pm

Thanks. <child> fixed that portion. looks like all this effort has not fixed the memory leak. still appears i am de-referencing a null_ptr. one less obstacle in my way.

feeling kind of stupid because im pretty sure i had it as <child> instead of <parent> but after running into an error i saw someone say the opposite on StackOverflow.


EDIT:

So after finding all of the pointers and converting them to smart_Ptrs i no longer am de referencing null_ptr's( yay :D )

but now the i am getting a empty shared ptr returned to me instead. which while is not leaking memory, not useful at all... i swear ptrs will be the death of me

albinopapa
Posts: 4373
Joined: February 28th, 2013, 3:23 am
Location: Oklahoma, United States

Re: Climbing out of Pointer Hell

Post by albinopapa » July 26th, 2017, 12:23 am

I take it you are using linux. I had to make a few changes to the properties like use relative paths instead of absolute paths. I also had to exclude all files from the project and import them back in.

I did have to do the rest, but I used the debug mode instead of the RefWith ones.

Glad to see you got the inheritance issue straightened out and the memory leak. I tried going through and using std::unique_ptr, but kept getting a lot of errors about Sprite(const Sprite &) is a deleted function and couldn't find where it was trying to be copied.

I notice you use a lot of pointers for things like parameters and return values instead of references or constant references. I would ask if you have a background in C, but you have a pretty good grasp on obect oriented programming.
If you think paging some data from disk into RAM is slow, try paging it into a simian cerebrum over a pair of optical nerves. - gameprogrammingpatterns.com

GracefulComet
Posts: 12
Joined: July 16th, 2017, 9:49 pm

Re: Climbing out of Pointer Hell

Post by GracefulComet » July 26th, 2017, 1:05 pm

Yes i am using Linux :).

I did attempt to get it working is VS 2015 for the sake of simplicity for everyone.
i guess its just a case of "works on my machine" and the effort didn't seem to work for you. :(

Sorry if that was any trouble just trying to get it to work/open. :oops:

I technically have no background in any language. I started out with the Chili's old Beginner Videos. i stopped about about old Intermediate 6 tutorial . only because the videos were too long for my brain to properly absorb.

the c influence might be from watching too much "Hand Made Hero" a twitch/youtube show about making video games. he use's straight c and i learned somethings from him. his videos are too long for my liking though.

I find thinking of code as objects easy to reason with.

I try not to over use ptr's if i don't have to. sometimes i don't catch myself when i use them unnecessarily.
i know i do use a lot in my messaging stuff but that because i am using inheritance, cant have a const referance to a purely virtual object :lol:

I have been working on the code some more since i last edited my last post. after a few more tests. i was struggling to make progress.
i think its because i am trying to use a new tool that i don't understand. so i went and loaded a clean copy from my last commit ( https://github.com/GracefulComet/Graceful-Engine )
this means i reverted from smart ptr's back to raw ptr's ( for now. )

I read somewhere smart pointers were supposedly amazing and a easy fix and that i don't need to remember to call delete yada yada yada... this seems to be harmful to my thinking process right now because i think i have object lifetime/cycle problem and i SHOULD bethinking about remember to call delete properly how long these objects are alive.

so i went on youtube and loaded up a new Chili video( i am starting back up with those . Great Job @Chili for making tutorials shorter :D ) and saw him stubbing the constructors and destructors with std::cout and that gave me an idea

so i started adding statements stating what objects were being constructed what ones were being destroyed.
and that just made debugging alot easier. i can see what has been done and what happened just before

so now i don't think it is leaking any more with the raw pointers( i will go back with smart pointers once i think this code is more resilient.)
but i still am de-referncing null once i have to deal with more than one pair of objects sending messages to each other.

what i have planed to is adding a way for Message Listeners to say what messages they are interested in and who. and ignore the rest. probably going to want to have objdestroyed messages to stop future problems of trying to send or recieve messages from objects that dont exist.

i dont have a plan yet to fix my current problem. just going to go through code line by line till i see where i fucked up.

ill post some of the new code soon.

PS: i was having the same problem with the Sprite(const Sprite &)
i dont think i have a Copy or Move Constructor whoops.


EDIT: i came up with a plan. I'm going to review the Constructors , Destructors, Move and Copy Constructors. and make sure everything is good in those locations.
going to do this for the following object types, msg , MSGdispatcher, MSGreciever, PhysicsMSG, AnimationMSG,
Sprite, GOfactory . as well as and setter functions they might contain too.

albinopapa
Posts: 4373
Joined: February 28th, 2013, 3:23 am
Location: Oklahoma, United States

Re: Climbing out of Pointer Hell

Post by albinopapa » July 26th, 2017, 5:29 pm

GracefulComet wrote: I did attempt to get it working is VS 2015 for the sake of simplicity for everyone.
i guess its just a case of "works on my machine" and the effort didn't seem to work for you. :(

Sorry if that was any trouble just trying to get it to work/open. :oops:
No worries, and I wouldn't have bothered if it was any trouble.
GracefulComet wrote:the c influence might be from watching too much "Hand Made Hero" a twitch/youtube show about making video games. he use's straight c and i learned somethings from him. his videos are too long for my liking though.
Yeah, I've watched a vid or two of his. If I remember correctly, he doesn't like object oriented languages. The thing I find odd about people who prefer C over C++ is you can still program only using functions and data structs in C++, but still use the nice/safer type system and of course the smart pointers. You don't have to use OOP if you don't want. In visual studio you can make it so it compiles only C. I did this and converted chili's framework over. I found it to be a horrible experience. C++ has spoiled me.
GracefulComet wrote: I try not to over use ptr's if i don't have to. sometimes i don't catch myself when i use them unnecessarily.
i know i do use a lot in my messaging stuff but that because i am using inheritance, cant have a const referance to a purely virtual object :lol:
Yes, you can have a const Parent & and still get the child overridden functions. I wouldn't use a vector<Parent &> for instance, but for returning from a function or passing to a function for use, you most certainly can.

Code: Select all

class Parent
{
public:
     virtual void DoSomething() = 0;
};
class Child : public Parent
{
public:
     void DoSomething() override;
};
class MyClass
{
public:
     void Dispatch( Parent &P )
     {
          P.DoSomething();  // Will call Child::DoSomething()
     }
     const Parent &GetElement( int Idx )const
     {
          return *(pElements[ Idx ]);
     }
private:
     std::vector<std::unique_ptr<Parent>> pElements;
};
I'm actually doing something similar to this code in another project, so I know you can use ref and const ref for inheritance.
GracefulComet wrote: I have been working on the code some more since i last edited my last post. after a few more tests. i was struggling to make progress.
i think its because i am trying to use a new tool that i don't understand. so i went and loaded a clean copy from my last commit ( https://github.com/GracefulComet/Graceful-Engine )
this means i reverted from smart ptr's back to raw ptr's ( for now. )

I read somewhere smart pointers were supposedly amazing and a easy fix and that i don't need to remember to call delete yada yada yada... this seems to be harmful to my thinking process right now because i think i have object lifetime/cycle problem and i SHOULD bethinking about remember to call delete properly how long these objects are alive.
I was wondering about how you pass objects around. In some cases you use copy by value and am wondering if that is a cause for the issues. I would suggest passing by const ref or just ref for the classes that hold the vector of shared_ptr/unique_ptr/raw pointers. Use const ref when you don't need to change the item being passed in and a regular ref otherwise.
If you think paging some data from disk into RAM is slow, try paging it into a simian cerebrum over a pair of optical nerves. - gameprogrammingpatterns.com

albinopapa
Posts: 4373
Joined: February 28th, 2013, 3:23 am
Location: Oklahoma, United States

Re: Climbing out of Pointer Hell

Post by albinopapa » July 26th, 2017, 5:52 pm

One thing I might suggest is make your classes have more specific purpose. For instance, your Sprite class.

Code: Select all

class Sprite // small graphics object
{
public:
	Sprite() {};
	Sprite( int width, int height, int nCol, int nRow, std::string textureID );
	Sprite( std::string FileName, SDL_Renderer *Render );
	Sprite( std::string FileName, SDL_Renderer *Render, state stat, int id );

	void LoadFromFile( std::string FileName, SDL_Renderer *Render );
	void SaveToFile( std::string FileName, std::string TextureFilename,
					 std::string textureID );
	void draw( SDL_Renderer *pRenderer );
	void update( float DeltaTime );
	void animate( float DurPerFrame, int NumOfFrames, bool Cycle );
	void setCurTile( int CurT );
	void Nanimate( float DurPerFrame, int NumOfFrames, bool Cycle );
	void SetPos( float x, float y );
	Vec2DF* getPosition();
	TileMap* getTiles();
	Vec2DF* getVec();
	void HandleMSG();
	MSGreciever* getListener();
	int getID();


protected:
	TileMap m_tiles;
	bool reverse;                          // m_reverse = false;
	Vec2DF m_position;                // m_position = {0.f, 0.f};
	Vec2DF m_movespeed;           // m_movespeed = {0.f, 0.f};
	MSGreciever m_messenger;
	std::string m_textureID;
	int m_animOffset;                   // m_animOffset = 0;
	TimerF m_timer;
	int m_ID;                               // int m_ID = 0;
};
I feel this class has too many obligations or job functions to perform. I've come to realize something about sprites ( resources in general ), you probably want to keep their interface pretty simple.

You've got four constuctors for instance and they don't address all the different members of the class. Doing this can work, but you might want to at least have some default initialization in the declarations as shown as comments above. What I'm really trying to get at is it looks like your Sprite class is trying to be more than a sprite.

It loads the images from your custom assets file.
It's a tile map loader/handler
It's an animated sprite and
It's a listener/MSGreceiver

As you can see, this sprite class can possibly be split up into four or five different classes. You have the member variables marked as protected, so I'm assuming you plan on making derivatives of this class, or perhaps just an oversight and you meant private. If you do plan on making this a polymorphic class, I'd definitely rethink what it's role is.

Just some food for thought. I know you're more worried about your pointer issues right now.
If you think paging some data from disk into RAM is slow, try paging it into a simian cerebrum over a pair of optical nerves. - gameprogrammingpatterns.com

albinopapa
Posts: 4373
Joined: February 28th, 2013, 3:23 am
Location: Oklahoma, United States

Re: Climbing out of Pointer Hell

Post by albinopapa » July 26th, 2017, 6:06 pm

Oh, there is another place I saw what might be a bug. When you are trying to get the last element in the que, you are saying que[que.size()]. The problem with this is indexing starts at 0, so if you have a vector of size 10, the elements go from 0 - 9. So asking for que[10] should throw an error, but it might also be possible that it's returning invalid data.
If you think paging some data from disk into RAM is slow, try paging it into a simian cerebrum over a pair of optical nerves. - gameprogrammingpatterns.com

GracefulComet
Posts: 12
Joined: July 16th, 2017, 9:49 pm

Re: Climbing out of Pointer Hell

Post by GracefulComet » July 26th, 2017, 6:54 pm

Lot to Absorb but, but all Very Helpful information.

I think most/all of your observations are Close or are absolutely correct.

Gonna Review some of my code some more with these points in mind.

:shock: I can Have Const& of pure virtual objects!?!!?!? :!: :?: :!:
*mind Blown*
i might need a few moments to pick my brains off the wall and research this.

i hate that vector.size() counts from 1 and everything else counts from 0. very easy to forget, has happened before too.
ill just wrap it like (vector.size -1) to fix that scenario. thanks for pointing out.


i need better names. TileMap actually provides calculations of all the possible crops location inside of a single image
that can later be paste on the games "canvas" or games screenbuffer that everyone actually see's i'll rename it to TileSet to be more accurate. i think that one might actually be important to keep.

i want Sprite's to be able to
1.Produce an animation onto the users screen.
2. Store and modify Internal State via Messages for Easy usability

because eventually the goal is to have a Behavior that does what GameObj's typically Do in games but instead of every Behavior owning a Sprite Obj and a SoundObj or mutiple SoundObjs plus what ever else a GameObj might need. The Behavior Object has a Messenger that Sends out Message to Sprite or Music or what ever it is that we need to update base off of world state that has updated. this way all Sprites for a level can be in a single vector and be contiguous in memory instead of have a for loop over vector of GameObj's that have have an individual ownership of a sprite and a sound file, etc...

i might elaborate more on this later but i really want to make some of the changes you suggested.

man having people (or person) to talk this through is super nice. i normally would just bang my head against a wall till it makes sense :mrgreen:

albinopapa
Posts: 4373
Joined: February 28th, 2013, 3:23 am
Location: Oklahoma, United States

Re: Climbing out of Pointer Hell

Post by albinopapa » July 26th, 2017, 10:38 pm

Woohoo! I finally got it running lol. No compile errors, no runtime errors.
If you think paging some data from disk into RAM is slow, try paging it into a simian cerebrum over a pair of optical nerves. - gameprogrammingpatterns.com

Post Reply