vector with multiple classes | Drawing order

[Rasmussen]

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

[Gambit]

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.

[Nexus]

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?

[Mörkö]

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.

[Mörkö]

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.

[Nexus]

Let's get concrete when talking about readability: how would you write my example?

[Jesper Juhl]

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.

[Ixrec]

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.

[Rasmussen]

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++; }

 

[Ixrec]

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.

[Rasmussen]

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++; }
    }
};
 

[Gambit]

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

Code: [Select]

std::vector<DrawableCLASS> DrawVec;
And changing the Scene::Define code to something like:

Code: [Select]

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 []).

[Rasmussen]

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.

[Gambit]

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.

[Nexus]

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.