-
Hi.
I know hardware level bits and bytes but the software part is lagging a bit.
I'm moving my little project from C/SDL to C++/SFML so I'm learning C++11 and SFML
at the same time and I'm stuck on this problem where I want a vector with multiple types,
to control the drawing order of the classes in the scene from the scene def section.
How can I create the classes so they fit inside a vector ?
I tried all last evening to fiddle with abstract base class and virtual function
but got stuck on compiler error (Function deleted) and lost the drive.
How can I do this?
class ANIMATION
{
public:
void Init();
void Render(sf::RenderWindow *Window);
void FreeUp();
void SetEnable(bool enable) { Enable = enable; };
bool GetEnable() { return Enable; };
void SetVisible(bool visible) { Visible = visible; };
bool GetVisible() { return Visible; };
//-- member functions unique for animation class
void SetAnimationSpeed(int speed) { AnimationSpeed = speed; };
private:
std::vector <sf::Sprite> SpriteVec;
std::vector <sf::Texture> TexVec;
}
class RAIN
{
public:
void Init();
void Render(sf::RenderWindow *Window);
void FreeUp();
//-- member functions as above
//-- member functions unique for Rain class
private:
sf::Sprite RainDropSprit1;
sf::Sprite RainDropSprit2;
sf::Sprite RainDropSprit3;
sf::Texture RainDropTex1;
sf::Texture RainDropTex2;
sf::Texture RainDropTex3;
}
class FOG
{
public:
void Init();
void Render(sf::RenderWindow *Window);
void FreeUp();
//-- member functions as above
//-- member functions unique for Fog class
private:
sf::Sprite FogSprit1;
sf::Texture FogTex1;
}
ANIMATION Anim;
RAIN Rain;
FOG Fog;
std::vector< ? > Scene;
// Scene 1 Def
Scene.push_back(Anim);
Scene[0].SetEnable(true);
Scene[0].SetVisible(true);
Scene[0].SetAnimationSpeed(0.5);
Scene[0].Init();
Scene.push_back(Rain);
Scene[1].Init();
Scene.push_back(Fog);
Scene[2].Init();
// Scene 2 Def
Scene.push_back(Fog);
Scene[0].Init();
Scene.push_back(Anim);
Scene[1].Init();
...
...
sf::RenderWindow Window;
// main loop
loop{ !++;
Scene [!] .Render(&Window);
}
-
Obligatory: You're supposed to learn C++ before trying to use libraries like SFML that are written in C++. It will save you a lot of future pain to read a proper C++ book cover to cover before trying anything non-trivial with SFML.
First off, you do not need Init() and FreeUp() methods. In C++, every struct/class comes with a "constructor" and a "destructor" that are automatically called when objects are created and destroyed. This is one of the best things about the language and it is absolutely mandatory that you understand them.
Anyway, without knowing the exact failing code and compiler error you were faced with, but it looks like you were trying to implement runtime polymorphism, so I can at least try to give you a hint with this:
class Drawable {
public:
virtual void Render(sf::RenderWindow*) = 0;
}
class ANIMATION: public Drawable {
public:
void Render(sf::RenderWindow*);
// other methods
private:
// data and private methods
}
// other Drawables
std::vector<Drawable*> Scene;
int main() {
// ...
ANIMATION a;
Scene.push_back(&a);
// ...
}
Of course you still need to read a proper C++ book that will tell you about polymorphism in great detail.
For instance, even that snippet makes the very strong assumption that all your drawables are created in main() and thus survive as long as you use the Scene vector. You will have to learn how ownership works in C++ and figure out whether this is good enough or if you need smart pointers or something else.
It may also be the case that an inheritance hierarchy of classes using polymorphism is not the best design, but that's also something you'll have to figure out for yourself.
-
Simple answer: a std::vector can only contain elements of a single type.
Maybe what you want is a vector of pointers to base class objects, maybe you really want multiple vectors holding different types - hard to tell.
What's easy to tell is that you need to learn more C++.
-
Ok my post did not come out right I see.
"First off, you do not need Init() and FreeUp() methods."
Yes I do, I have a full page of code in those functions, the instance of the class lives for the
game duration and gets cleared at scene change.
"In C++, every struct/class comes with a "constructor" and a "destructor"
I know.
The provided code was just sudo code to illustrate what I was trying to do.
"Obligatory: You're supposed to learn C++ before trying to use libraries like SFML that are written in C++."
I know but SFML is the driving force, SFML is the reason its worth for non pro-programmers to learn C++
It's a balance act between reading boring text and getting something on the screen for newcomers,
and attracting those seams to be an important subject in the all the videos from the speakers at GoingNative 2012,
CppCon 2014 and from Bjarne Stroustrup I've been watching lately while eating dinner.
btw. thanks to those videos I'm been removing all new and delete and naked pointers in my project.
"it looks like you were trying to implement runtime polymorphism, so I can at least try to give you a hint with this:"
Yes Thanks ! "runtime polymorphism" Something I will look up in my C++ books
btw I have Bjarne Stroustrup: The C++ Programming Language C++11 Edition
Charles River C++ for Game Programmers
SFML Game Development.
But need time to read them, I'm 35% into sfml game development, 5% into Game programmers and 0,001 % into The c++ programming language, it's more fun to program :-)
I tried last night to get your example to work but not without adding functions to Drawable class to mirror those in the animation and Rain class, -a super class.
So its clear that I need to read up on that subject before going down that road in my 'C with classes' project.
And from what your guys are saying I guess that looking into boost:Any/variant is not necessary,
just learn C++ and I can do what I wanted?
-
"First off, you do not need Init() and FreeUp() methods."
Yes I do, I have a full page of code in those functions, the instance of the class lives for the
game duration and gets cleared at scene change.
No you don't ;)
The reason why you currently need those methods is because you have a questionable design, not an inherent constraint in game logic. Why not tie the lifetime of your C++ objects to the lifetime of the actual game objects they're representing? What's the point of reusing instances? Hint: performance is not, because the major work is done in constructor/destructor or init/free methods. Which one of them you choose does not have a great influence on speed. Init/free methods may even lead to a slowdown, because all the variables are initialized to some unneeded default value just to be overwritten in the init function.
Apart from that, such methods are bad design, because you have no class invariants and cannot guarantee correct initialization and destruction. Instead, you rely on manual mechanisms (the user calling those functions) -- and this is exactly the same reason why new/delete should be avoided.
I know but SFML is the driving force, SFML is the reason its worth for non pro-programmers to learn C++
It's a balance act between reading boring text and getting something on the screen for newcomers
If you want to get quick results, then there are definitely more effective ways than learning C++. You've picked the most complex mainstream programming language, with pitfalls and unintuitive rules all over the place. It can be worth paying this price if you really exploit the freedom and the variety of paradigms C++ gives you, but it requires several years. That's not exaggerated, because learning the language is only one part; knowing modern idioms and how to apply and combine different techniques is at least as important, if not more. And this does not only require modern literature, but also experience in multiple projects.
I don't want to discourage you, but you must be aware that learning C++ is a long journey, not something you do by the way to get some games done.
-
" Why not tie the lifetime of your C++ objects to the lifetime of the actual game objects they're representing? What's the point of reusing instances?"
Good question, I'd had no reason to do that at this time, and the clear function is only stuff like string.clear() and vector.clear(). my scene switching loading time is focused on texture loading sprite creation stuff, witch seams
more important by a factor companied to string.clear
But its true that what you are suggesting seams a more object oriented style.
If you want to get quick results, then there are definitely more effective ways than learning C++. You've picked the most complex mainstream programming language, with pitfalls and unintuitive rules all over the place. It can be worth paying this price if you really exploit the freedom and the variety of paradigms C++ gives you, but it requires several years. That's not exaggerated, because learning the language is only one part; knowing modern idioms and how to apply and combine different techniques is at least as important, if not more. And this does not only require modern literature, but also experience in multiple projects.
I don't want to discourage you, but you must be aware that learning C++ is a long journey, not something you do by the way to get some games done.
Properly all true but as I heard Bjarne Stroustrup say, don get lost in all the advanced features take what you will, when it subject is C > C++
So right now I'm in the C with classes and a few vectors, map, template, std::thread, place floating, looking for a container that can handle multiple types, so I can avoid inheritance/polymorphism witch I have no need for right now and find boring. :-)
But off course if I have 20 classes and all of them have Set/GetEnable() and Set/GetVisible()
then having one class that take care of that would be really nice.
-
I just got this working, and it is testing out great.
I see chapter one in C++ for Game programmers is
Inheritance - polymorph - virtual function ...
and says is very important to learn and for the rest of the book ,
I promise I will look into that soon. ;D
class Drawable {
public:
virtual void Render(sf::RenderWindow*) = 0;
virtual ~Drawable(){};
};
class AnimCLASS : public Drawable {};
class CloudCLASS: public Drawable {};
int main()
{
bool exit=false;
sf::RenderWindow WINDOW(sf::VideoMode(800, 600), "SFML window");
AnimCLASS Anim;
Anim.SetEnable(true);
Anim.SetVisible(true);
Anim.SetImageFileName("textures/Art01");
Anim.Init();
CloudCLASS Clouds;
Clouds.SetEnable(true);
Clouds.SetVisible(true);
Clouds.NumberOfClouds = 120;
Clouds.Xmin = -8000;
Clouds.Xmax = 10000;
Clouds.Ymin = -6000;
Clouds.Ymax = 100;
Clouds.Zval = 1000;
Clouds.Xpos = 0;
Clouds.Ypos = -200;
Clouds.Speed = 8;
Clouds.Size = 110;
Clouds.SetColor(255,255,255,255);
Clouds.Init();
Drawable * DrawAnim;
Drawable * DrawCloud;
DrawAnim = &Anim;
DrawCloud = &Clouds;
Drawable* SCN[2] = { DrawAnim, DrawCloud };
do{
SCN[0]->Render(&WINDOW);
SCN[1]->Render(&WINDOW);
WINDOW.display();
if (sf::Keyboard::isKeyPressed(sf::Keyboard::S)) { Clouds.Speed = 20;}
if (sf::Keyboard::isKeyPressed(sf::Keyboard::Escape)) { exit=true; }
sf::sleep(sf::milliseconds(10));
} while(!exit);
return 0;
}
-
And made into using a vector, so now I have exactly what I was looking for.
int main()
{
bool exit=false;
sf::Event event;
sf::RenderWindow WINDOW(sf::VideoMode(800, 600), "SFML window");
std::vector<Drawable*> Scene;
CloudCLASS Clouds;
Drawable * DrawCloud;
DrawCloud = &Clouds;
Clouds.SetEnable(true);
Clouds.SetVisible(true);
Clouds.NumberOfClouds = 120;
Clouds.Xmin = -8000;
Clouds.Xmax = 10000;
Clouds.Ymin = -6000;
Clouds.Ymax = 100;
Clouds.Zval = 1000;
Clouds.Xpos = 0;
Clouds.Ypos = -200;
Clouds.Speed = 8;
Clouds.Size = 110;
Clouds.SetColor(255,255,255,255);
Clouds.Init();
Scene.push_back(DrawCloud);
AnimCLASS Anim;
Drawable * DrawAnim;
DrawAnim = &Anim;
Anim.SetEnable(true);
Anim.SetVisible(true);
Anim.SetImageFileName("textures/menu");
Anim.Init();
Scene.push_back(DrawAnim);
do{
WINDOW.pollEvent(event);
for(uint32_t i=0; i<Scene.size(); i++)
Scene[!]->Render(&WINDOW);
if (sf::Keyboard::isKeyPressed(sf::Keyboard::S)) { Clouds.Speed = 20;}
if (sf::Keyboard::isKeyPressed(sf::Keyboard::Escape)) { exit=true; }
WINDOW.display();
sf::sleep(sf::milliseconds(10));
} while(!exit);
return 0;
}
-
CloudCLASS Clouds;
Drawable * DrawCloud;
DrawCloud = &Clouds;
Scene.push_back(DrawCloud);
You don't need to create a pointer in this case. Just do this:
CloudCLASS Clouds;
Scene.push_back(&Clouds);
do{
// ...
} while(!exit);
`do while` is considered bad style. You should consider using normal `while` instead.
WINDOW.pollEvent(event);
Why are you polling for events when you don't handle any events?
for(uint32_t i=0; i<Scene.size(); i++)
Scene[!]->Render(&WINDOW);
You have put an exclamation mark instead of i in the subscript operator.
And in any case you should use range based for-loop when iterating all the elements of a container, like this:
for (auto& item : container) {
// do something with item
}
-
`do while` is considered bad style.
Who says that and why? There are not many applications for do while, but in some cases it's the simplest approach, and rewriting to while would just complicate code unnecessarily.
Otherwise I agree. Rasmussen, you should also choose a naming convention and apply it consistently. I would especially avoid CAPS_LOCK style except for macros and maybe constants. And you should definitely read again the SFML tutorials about event handling. Don't mix realtime input with events.
And please use C++ code tags, not quotes.
-
CloudCLASS Clouds;
Drawable * DrawCloud;
DrawCloud = &Clouds;
Scene.push_back(DrawCloud);
You don't need to create a pointer in this case. Just do this:
CloudCLASS Clouds;
Scene.push_back(&Clouds);
Roger!, thanks.
do{
// ...
} while(!exit);
`do while` is considered bad style. You should consider using normal `while` instead.
Can't see why at the moment, but I keep it in mind.
WINDOW.pollEvent(event);
Why are you polling for events when you don't handle any events?
The window froze and got greyed out like style and the default windows 7 mouse cursor
chanced to rotating busy, without it in the testbench code, even while using sf::sleep.
for(uint32_t i=0; i<Scene.size(); i++)
Scene[!]->Render(&WINDOW);
You have put an exclamation mark instead of i in the subscript operator.
And in any case you should use range based for-loop when iterating all the elements of a container, like this:
for (auto& item : container) {
// do something with item
}
That was on purpose , "[ i ]" seam to vanish, I thought it was a forum BB code or something.
Thanks for the suggestions.
-
Otherwise I agree. Rasmussen, you should also choose a naming convention and apply it consistently. I would especially avoid CAPS_LOCK style except for macros and maybe constants. And you should definitely read again the SFML tutorials about event handling. Don't mix realtime input with events.
And please use C++ code tags, not quotes.
I'm in the creative phase, I'll beautified the code on days when I'm not on fire so to speak.
"Don't mix" this:
if (sf::Keyboard::isKeyPressed(sf::Keyboard::Escape)) { exit=true; }
with this:
WINDOW.pollEvent(event);
?
In the real main loop the mouse uses the event.
Roger on the tags.
-
do{
// ...
} while(!exit);
`do while` is considered bad style. You should consider using normal `while` instead.
do while is guaranteed to run at least once, even if the condition is always false. A standard while loop will not run at all if the condition is false.
-
Mission accomplished, darn nice to go from this static drawing order and mess
void Render(sf::RenderWindow *window)
{
Layer_1_Anim.Render(window);
WX.Rings.Render(window);
Layer_2_Anim.Render(window);
Layer_3_Anim.Render(window);
WX.Fog.Render(window);
Layer_4_Anim.Render(window);
Particle.Render(window);
Particle2.Render(window);
WX.Render(window);
WX.Cloud.Render(window);
WX.RAIN.Render(window);
FireAnim.Render(window);
Text.Render(window);
Layer_5_Anim.Render(window);
Layer_6_Anim.Render(window);
Layer_7_Anim.Render(window);
Layer_8_Anim.Render(window);
};
To this dynamic beauty..
void Render(sf::RenderWindow *window)
{
for (auto& item : DrawVec) {
item->Render(window);
}
}
-
Who says that and why? There are not many applications for do while, but in some cases it's the simplest approach, and rewriting to while would just complicate code unnecessarily.
Can you demonstrate one or more examples where do while is unequivocally the correct construct?
On a related note, there exists a lot of cases where goto would be "the simplest approach", but I'm not sure you would recommend its usage to people, especially beginners.
Even if you are able to make an argument for why there exists some scenarios where do while is logically the right choice, I will argue that people should stick to normal while anyway because of consistency and simplicity. while is more readable and and easier to reason about.
-
do{
// ...
} while(!exit);
`do while` is considered bad style. You should consider using normal `while` instead.
do while is guaranteed to run at least once, even if the condition is always false. A standard while loop will not run at all if the condition is false.
What seams logic to me is using
while(!Exit){
}
if Exit can be activated before the the loop, and
do{
}while(!Exit);
if it is only activated inside
-
Who says that and why? There are not many applications for do while, but in some cases it's the simplest approach, and rewriting to while would just complicate code unnecessarily.
Can you demonstrate one or more examples where do while is unequivocally the correct construct?
On a related note, there exists a lot of cases where goto would be "the simplest approach", but I'm not sure you would recommend its usage to people, especially beginners.
Even if you are able to make an argument for why there exists some scenarios where do while is logically the right choice, I will argue that people should stick to normal while anyway because of consistency and simplicity. while is more readable and and easier to reason about.
You should read what I posted before. do while will ALWAYS run once, no matter what, even if the condition is false.
-
Can you demonstrate one or more examples where do while is unequivocally the correct construct?
No problem, here's a typical example:
std::string input;
do
{
std::cout << "Enter input: ";
std::getline(std::cin, input);
} while (!validate(input));
But in general, it's your task to support your claim with arguments, not ours to disprove it. ;)
Especially since this is not a widespread opinion in the C++ community, as opposed to goto for example. So, why is do while bad style? You're aware that you can't simply replace it with while without altering semantics?
-
But in general, it's your task to support your claim with arguments, not ours to disprove it. ;)
I was asking for examples to support this claim:
"[...] in some cases it's the simplest approach, and rewriting to while would just complicate code unnecessarily."
My point being that same could be said in support of goto, of using global variables, #define, manual new and delete, etc.
So, why is do while bad style?
Because it's less readable than normal while. If your intent is to do something once and then loop if a condition is met, then it's better to do so explicitly instead of using an unusual control structure just because it's possible to use it. The logic of why normal while should be preferred is exactly the same as for why you should use break and continue in favour of goto.
You may also take into consideration what Bjarne Stroustrup has written on the topic:
In my experience, the do-statement is a source of errors and confusion. The reason is that its body is always executed once before the condition is evaluated. However, for the body to work correctly, something very much like the condition must hold even the first time through. More often than I would have guessed, I have found that condition not to hold as expected either when the program was first written and tested, or later after the code preceding it has been modified. I also prefer the condition "up front where I can see it." Consequently, I tend to avoid do-statements.
You're aware that you can't simply replace it with while without altering semantics?
I am aware. I am arguing that sticking to while keeps code cleaner and simpler.
-
You should read what I posted before. do while will ALWAYS run once, no matter what, even if the condition is false.
I'm aware what it does, thank you for clarifying that.
-
Let's get concrete when talking about readability: how would you write my example?
-
do-while is not "unusual" or "confusing" except maybe for newbie C++ programmers. It clearly has its use when you want to do something at least once and then as long as some condition holds. It is useful and dead simple and I for one have never heard it be called bad practice or similar before.
-
Even if do-while was somehow inferior, it's a very tiny and irrelevant issue compared to all the other things we could say about that code. Ignoring what's already been said, there's still:
- His main loop renders stuff, then handles keyboard input, then actually displays the rendered stuff. This is a strange and confusing order in which to do those operations (and it's probably doing something bad like forcing all input to lag one frame).
- He's using real-time input when he probably wants events. The Escape to close behavior in particular is almost certainly better as an event.
- The "exit" sentinel variable appears to contain no special meaning or information. He could have just as easily used while(true) {} with break;, or the SFML convention while(window.isOpen()) {} with window.close();, depending on whether or not he wants a "quick exit" from the loop. Or the do-while equivalents of those. None of these requires declaring an additional variable at the top of main().
- He's not using any Vector2s or Vector3s in his drawable classes, forcing him to set each x,y,z coordinate individually rather than group them together.
- He's giving his classes Init() methods instead of proper constructors.
- His classes need to be explicitly "enabled" and made "visible". Is there a reason the classes aren't like this by default after construction?
We should be pointing out stuff like this instead of arguing about do-while.
-
Even if do-while was somehow inferior, it's a very tiny and irrelevant issue compared to all the other things we could say about that code. Ignoring what's already been said, there's still:
- His main loop renders stuff, then handles keyboard input, then actually displays the rendered stuff. This is a strange and confusing order in which to do those operations (and it's probably doing something bad like forcing all input to lag one frame).
- He's using real-time input when he probably wants events. The Escape to close behavior in particular is almost certainly better as an event.
- The "exit" sentinel variable appears to contain no special meaning or information. He could have just as easily used while(true) {} with break;, or the SFML convention while(window.isOpen()) {} with window.close();, depending on whether or not he wants a "quick exit" from the loop. Or the do-while equivalents of those. None of these requires declaring an additional variable at the top of main().
- He's not using any Vector2s or Vector3s in his drawable classes, forcing him to set each x,y,z coordinate individually rather than group them together.
- He's giving his classes Init() methods instead of proper constructors.
- His classes need to be explicitly "enabled" and made "visible". Is there a reason the classes aren't like this by default after construction?
We should be pointing out stuff like this instead of arguing about do-while.
You could, and I'm sure there would be something to learn from it, but it would be misdirected
and way out of proportion, when considering the purpose of the testbech code.
Anyway, I've now gone away from static instantiation of the Anim class to dynamic storing them in a vector.
and then the DrawAbleVector get a reference too that. To get that to work I had to use vector.reserve()
without it after the 2nd. push the vector resized and chanced memory location and the ref was invalid.
So now I have perfect drawing order control from the scene definition file,
and can move the blocks up and down the layer stack without altering any code.
And I've started making overloaded constructors for the basic stuff, and I have removed
a huge amount of boiler plate code - 'I think it's called', as a benefit from the effort of restructuring the code.
Old class Scene{
public:
AnimCLASS Anim_layer1
...
AnimCLASS Anim_layer8
New class Scene{
public:
std::vector<DrawableCLASS*> DrawVec;
std::vector<AnimCLASS> DrawAnimVec;
Scene.Define(int SN) {
if(SN==Scene_Intro) {
uint32_t index=0;
{ // Full screen Background single frame pos 0,0 scale 1,1
AnimCLASS Anim(true,true,"textures/Art01");
DrawAnimVec.push_back(Anim);
DrawVec.push_back(&DrawAnimVec[index]);
index++; }
// Fog -old C function/class on the todo list..
WX.FOG.SetEnable(true);
WX.FOG.SetVisible(true);
WX.FOG.SetBandsColor(205,205,205,30);
WX.FOG.NumOfBands = 50 ;// 50 optimal
WX.FOG.BandYmin = -300;
WX.FOG.BandYmax = 600;
WX.FOG.BandSize = 2;
WX.FOG.BandSpeed = 2;
DrawVec.push_back(&WX.FOG);
{// Actor sidding
AnimCLASS Anim(true,true,"textures/girl4");
Anim.SetPos(808,463);
Anim.SetScale(0.18,0.18);
Anim.SetClickPoint(true, true, "Dynamic", "Aixy", "ACTOR_NO" ,0 , "Cursor_Red", 808,463, 50, 88);
DrawAnimVec.push_back(Anim);
DrawVec.push_back(&DrawAnimVec[index]);
index++; }
{// Fire animation 24 frames
AnimCLASS Anim(true,true,"textures/Particles/Fireplace");
Anim.SetFrameCount(23);
Anim.SetCounterType(UP_Counter);
Anim.SetPos(484,394);
Anim.SetScale(0.26,0.26);
Anim.SetColor(255,255,245,115);
Anim.SetAnimationSpeed(0.6);
Anim.SetLayerName("Fire");
AnimVec.push_back(Anim);
DrawVec.push_back(&DrawAnimVec[index]);
index++; }
-
You could, and I'm sure there would be something to learn from it, but it would be misdirected
and way out of proportion, when considering the purpose of the testbech code.
Which is why I didn't bother with it before, but the do-while stuff was much sillier. It looks like you've already started tackling some of the items I listed anyway =)
Anyway, I've now gone away from static instantiation of the Anim class to dynamic storing them in a vector.
and then the DrawAbleVector get a reference too that. To get that to work I had to use vector.reserve()
without it after the 2nd. push the vector resized and chanced memory location and the ref was invalid.
This is a major "code smell", i.e. it implies there's a significant design flaw in this code you'll probably regret later (no matter how small this program is supposed to be). Yes, it's bad enough to be worth bringing up despite everything that's been said above.
reserve() should only be a performance optimization; relying on it for correct behavior like this means you're misusing std::vector. In this case, you appear to be assuming your std::vector will never exceed a certain fixed size, so you might as well use a std::array (or raw array) to actually enforce a fixed size if that's what you want. Or use a node-based container like std::list or std::map if you want variable size and stable references. Or use only one container to store all your drawable objects so this reference validation issue can't even come up. Or have two containers that are mutually exclusive (one for animations and one for the other drawables) so one doesn't have to find a way to reference the other. Any of these would probably be an improvement.
-
Anyway, I've now gone away from static instantiation of the Anim class to dynamic storing them in a vector.
and then the DrawAbleVector get a reference too that. To get that to work I had to use vector.reserve()
without it after the 2nd. push the vector resized and chanced memory location and the ref was invalid.
This is a major "code smell", i.e. it implies there's a significant design flaw in this code you'll probably regret later (no matter how small this program is supposed to be). Yes, it's bad enough to be worth bringing up despite everything that's been said above.
reserve() should only be a performance optimization; relying on it for correct behavior like this means you're misusing std::vector. In this case, you appear to be assuming your std::vector will never exceed a certain fixed size, so you might as well use a std::array (or raw array) to actually enforce a fixed size if that's what you want. Or use a node-based container like std::list or std::map if you want variable size and stable references. Or use only one container to store all your drawable objects so this reference validation issue can't even come up. Or have two containers that are mutually exclusive (one for animations and one for the other drawables) so one doesn't have to find a way to reference the other. Any of these would probably be an improvement.
True, I've now chanced it to std::array size 20, so its workable for now, until I learn some more and change it to something better, safer and cleaner.
I'm expecting to use 1-12 anim classes simultaneously
Otherwise I'm open to better ways of doing it.
const uint32_t MAX_ANIM_ARRAY =20;
class Scene {
public:
std::vector<DrawableCLASS*> DrawVec;
std::array<AnimCLASS, MAX_ANIM_ARRAY> DrawAnimArray;
void Define(int SN);
}
Scene::Define(int sn) {
uint32_t index=0;
if(sn==Scene_Intro) {
{// layer
AnimCLASS Anim("Forground 1",true,true,"textures/Art03");
DrawAnimArray[index]=Anim;
DrawVec.push_back(&DrawAnimArray[index]);
index++; }
}
};
-
This code cannot work. When Scene::Define is called, your Anim object is created and pushed into the vector, but when the function ends, your Anim pointer will become invalidated and your program will most likely crash. You are better off using
std::vector<DrawableCLASS> DrawVec;
And changing the Scene::Define code to something like:
AnimCLASS Anim("Forground 1",true,true,"textures/Art03");
DrawAnimArray[index]=Anim;
DrawVec.push_back(Anim);
It also solves your unnecessary indexing operation (Where you use []).
-
This code cannot work.
When Scene::Define is called, your Anim object is created and pushed into the vector, but when the function ends, your Anim pointer will become invalidated and your program will most likely crash.
Its working alight.
The short lived static instance of AnimCLASS get killed at the end of the block scope,
after the initialized object has been copied into the AnimArray
DrawableVector takes a pointer to a Drawable Class, and can't handle the object by value.
And remember that I got many section with the same name:
Scene::Define {
{// layer 1
AnimCLASS Anim("Forground 1",true,true,"textures/Art03");
DrawAnimArray[index]=Anim;
DrawVec.push_back(&DrawAnimArray[index]);
index++; }
{// layer 2
AnimCLASS Anim("Forground 2",true,true,"textures/Art03");
DrawAnimArray[index]=Anim;
DrawVec.push_back(&DrawAnimArray[index]);
index++; }
}
So if Anim lasted to the end of Scene::Define scope there would be name conflicting.
I tried this std::vector<DrawableCLASS> DrawVec but could not get it to work with.
-
Sorry I just realised I misread what was going on (I had just woken up when I posed that). Instead of a vector of pointers, you might consider a vector of std::reference_wrapper.
-
The short lived static instance of AnimCLASS get killed at the end of the block scope,
after the initialized object has been copied into the AnimArray
DrawableVector takes a pointer to a Drawable Class, and can't handle the object by value.
So you know what causes the problem... You must make sure that objects live as long as they're referenced. Don't store pointers variables that will fall out of scope.
And there is no static instance; objects with static storage category wouldn't be destroyed at the end of block. Make sure you use the terms correctly or you'll confuse people.
And remember that I got many section with the same name:
Another reason why you should redesign your code. Avoid code duplication whenever possible. Use functions in this case.
Instead of a vector of pointers, you might consider a vector of std::reference_wrapper.
I have no idea how this would help... Since it internally stores pointers, the invalidation problem persists. Although std::reference_wrapper can be stored in STL containers, it's usually used in combination with std::bind() through std::ref/cref()... And that's rather advanced stuff, I would advise Rasmussen to learn about basic topics like scopes, functions, STL container first.
-
I wasnt suggesting it as a fix, more because the pointer should never be null, so shouldnt be a pointer but a reference, but since you cant store references in containers, I suggested to use std::reference_wrapper.
-
Everything works great and as expected,
And since I had an array filled with anim classes anyway there where no reason to use
the block scope and temporary instantiation of the anim class.
const uint32_t MAX_ANIM_ARRAY =20;
class Scene {
public:
std::vector<DrawableCLASS*> DrawVec;
std::array<AnimCLASS, MAX_ANIM_ARRAY> DrawAnimArray;
void Define(int SN);
void Setup() {
for (auto& item : DrawVec)
item->Init();
}
void Render(sf::RenderWindow &window) {
for (auto& item : DrawVec)
item->Render(window);
}
}
Scene::Define(int sn) {
uint32_t index=0;
if(sn==Scene_Intro) {
// - layer 0
DrawAnimArray[index].Define("Background", 0, true, true, 1,"textures/Art01", 0,0, 1.0f,1.0f,0);
DrawVec.push_back(&DrawAnimArray[index]);
index++;
// - layer 1
DrawAnimArray[index].Define("Forground", 0, true, true, 1,"textures/Art02", 0,0, 1.0f,1.0f,0);
DrawVec.push_back(&DrawAnimArray[index]);
index++;
// - layer 2
DrawAnimArray[index].Define("Actor sprite", 0, true, true, 1,"textures/Actor02", 0,0, 1.0f,1.0f,0);
DrawAnimArray[index].SetClickPoint(true, true, "Dynamic", "ActorName", "ACTOR_YES" ,0 , "Cursor_Green", 748,380, 48, 72);
DrawVec.push_back(&DrawAnimArray[index]);
index++;
if(index> MAX_ANIM_ARRAY) throw std::Kitchen::Sink;
}
};
I played with the refrence wrapper and got it to work by going though an extra function to un-wrap before it could be rendered, so if I really wanted to get rid of std::vector<DrawableCLASS*> DrawVec; I could use that.
-
Anyway, I've now gone away from static instantiation of the Anim class to dynamic storing them in a vector.
and then the DrawAbleVector get a reference too that. To get that to work I had to use vector.reserve()
without it after the 2nd. push the vector resized and chanced memory location and the ref was invalid.
reserve() should only be a performance optimization; relying on it for correct behavior like this means you're misusing std::vector. In this case, you appear to be assuming your std::vector will never exceed a certain fixed size, so you might as well use a std::array (or raw array) to actually enforce a fixed size if that's what you want. Or use a node-based container like std::list or std::map if you want variable size and stable references. Or use only one container to store all your drawable objects so this reference validation issue can't even come up. Or have two containers that are mutually exclusive (one for animations and one for the other drawables) so one doesn't have to find a way to reference the other. Any of these would probably be an improvement.
Reading in The C++ Programming Language page 898 STL containers - size and capacity:
When changing the size or capacity, the elements may be moved to new storage location.
That implies that iterators(and pointers and references) to elements may become invalid(i.e. points to the old element location.
[..]
It is tempting to assume that reserve() improves performance, but the standard growth strategies for vector are
so effective that performance is rarely a good reason to use reserve().
Instead, see reserve() as a way of increasing the predictability of performance and for
avoiding invalidation of iterators.
In C++ for game programmers page 206:
There is directly instructions to use vector.reserve() to avoid relocation and invalidation of pointer and references.
So I'm considering going back to using a vector since reserve() does not change to value reported by
.size() where a std::array[20] report a size of 20 no matter if I actually use 20 or not.
-
I've now got std::map to work as a reference holder, by looking at the use of map in the sfml book code
resourceholder, with I allready use in my menu and map classes.
And I've placed the instantiation of Animclass map & Drawable vector
in the scene change loop so they destroy by them self.
As has be suggested.
The structure now look like this.
file:Define_Intro.cpp
// - layer 0
{ AnimCLASS tmp("Background", 0, true, true, 1,0, "textures/Art01", 0,0, 1,1);
AnimMap.insert(std::make_pair(id, tmp));
auto found = AnimMap.find(id);
DrawVec.push_back(&found->second); id++}
void SceneCLASS::SetUp(AudioCLASS &audio, MouseCLASS &mouse, int SN
, std::vector<DrawableCLASS*> &DrawVec, std::map<int,AnimCLASS> &AnimMap )
{
int id = 0;
if (SN==Intro){ #include "Define_Intro.cpp" }
//
//
for (auto& item : DrawVec) {
item->Init();
if(item->GetClickPoint().Enable) {
// push Dynamic clickpoints
CPSceneVec.push_back(item->GetClickPoint() );
}
}
};
void GameCLASS::Start()
{ bool exit_loop = false; int SceneNumber = 0;
do{
if( SCENE.NextScene == SceneNumber)
{
std::map<int,AnimCLASS> AnimMap;
std::vector <DrawableCLASS*> DrawableVec;
SCENE.SetUp(Audio,Mouse,SceneNumber,DrawableVec, AnimMap);
Run(DrawableVec, SceneNumber);
}
SceneNumber++;
if(SceneNumber>=NumberofScenes) SceneNumber=0;
if(SCENE.NextScene == 99) exit_loop=true;
}while(!exit_loop);
};
int main()
{
GameCLASS GAME;
GAME.SetUp();
GAME.Start();
return 0;
}