Lost in a maze of nested functions!

Started by Zephyr, Wed 22/01/2020 11:20:53

Previous topic - Next topic

Zephyr

Hi again Everyone,

         OK, so I'm building my first "shop" in my game, but it's a little complex in that 2 separate transactions can take place by interaction with only 1 hotspot.  This is further complicated by the fact that each transaction involves having:
a). the correct amount of cash
b). the correct inventory item.

I've messed around for hours trying to sort out the necessary nested functions and my brain is about to explode!
Here's what my last attempt was:-
Code: ags

function hFont_UseInv()
{      
      if (cChristian.ActiveInventory == (iEmptyBottle))
{
         cChristian.Walk (225, 168, eNoBlock, eWalkableAreas);
}
      if (Money >= 5)
{
         cChristian.LoseInventory (iEmptyBottle);
         cChristian.AddInventory (iHolyWater);
         Money -= 5;
         Display ("You have a bottle of Holy Water.");
}
    else
{
         Display ("Not enough money.");
}
      if (cChristian.ActiveInventory == (iStMichael))
{
         cChristian.Walk (225, 168, eNoBlock, eWalkableAreas);
}
      if ((Money >= 5)  // error msg reads "end of input reached in middle of expression"
{
          cChristian.LoseInventory (iStMichael);
          cChristian.AddInventory (iBlessedStMichael);
          Money -= 5;
          Display ("Your medallion has been blessed.");
}
     else 
{
          Display ("Not enough money.");
}
}


Can anyone please tell me where I've gone wrong and (in simple terms) WHY it's wrong?
Thanks again,
- Zephyr   :confused:
<3 Zephyr <3

CaptainD

On my phone so can't see it all that well atm but in the "if money" line you have an extra "(" which isn't going to help.
 

Slasher

Not exactly the same but it may shed some usefulness

From a Dialog option.

Code: ags

   cTurner.SayBubble("1 thimble of ale, 1 bitters, 1 teaspoon of sherbert,");
   cTurner.SayBubble("...1 small sweet sherry, 1 shot of pineapple juice, 1 shot of lime cordial,");
   cTurner.SayBubble("...and a banana slice.");
   cTurner.SayBubble("Stirred not Shaken.");
   cChang.SayBubble("&6 That will be Â¥139 Yuans.");
   if(Money <=138.00){
   cTurner.SayBubble("Seems like I don't have enough money.");
   cTurner.SayBubble("I'll have to leave it for now.");
   cChang.SayBubble("OK, sir.");
   }
     else {
   cTurner.SayBubble("Here you are.");
   Money -=100.39;
   object[16].Graphic=2380;
   Wait(40);
   object[16].Visible=true;
   cChang.SayBubble("&7 Here you go, sir.");
   cTurner.SayBubble("Agh!");
   cTurner.SayBubble("This tastes disgusting!");
   cChang.SayBubble("&8 Maybe you got the recipe wrong, sir.");
   cTurner.SayBubble("You are probably right!");
   object[16].Visible=false;
   DRINKA=true;
     }



Crimson Wizard

When dealing with lots of conditions, the proper code indentation saves lifes!

Original code with correct indentation
Code: ags

function hFont_UseInv()
{      
    if (cChristian.ActiveInventory == (iEmptyBottle))
    {
        cChristian.Walk (225, 168, eNoBlock, eWalkableAreas);
    }
    if (Money >= 5)
    {
        cChristian.LoseInventory (iEmptyBottle);
        cChristian.AddInventory (iHolyWater);
        Money -= 5;
        Display ("You have a bottle of Holy Water.");
    }
    else
    {
        Display ("Not enough money.");
    }
    if (cChristian.ActiveInventory == (iStMichael))
    {
        cChristian.Walk (225, 168, eNoBlock, eWalkableAreas);
    }
    if (Money >= 5)  // error msg reads "end of input reached in middle of expression"
    {
        cChristian.LoseInventory (iStMichael);
        cChristian.AddInventory (iBlessedStMichael);
        Money -= 5;
        Display ("Your medallion has been blessed.");
    }
    else 
    {
        Display ("Not enough money.");
    }
} // ?



From the above code we can see that all the actions are performed in a straight sequence:
1. Check if active inventory is iEmptyBottle, and if yes then walk.
2. Check if has money, and if yes, then exchange iEmptyBottle for iHolyWater.
3. Check if active inventory is iStMichael, and if yes then walk.
4. Check if has money, and if yes, then exchange iStMichael for iBlessedStMichael.

Because they are performed in one sequence, there's no interconnection between them, in other words, money check will be done even if you don't have an item.

To get it proper you need to nest condition 2 inside condition 1, and nest condition 4 inside condition 3.

Code: ags

function hFont_UseInv()
{      
    if (cChristian.ActiveInventory == (iEmptyBottle))
    {
        cChristian.Walk (225, 168, eNoBlock, eWalkableAreas);
        if (Money >= 5)
        {
            cChristian.LoseInventory (iEmptyBottle);
            cChristian.AddInventory (iHolyWater);
            Money -= 5;
            Display ("You have a bottle of Holy Water.");
        }
        else
        {
            Display ("Not enough money.");
        }
    }
    if (cChristian.ActiveInventory == (iStMichael))
    {
        cChristian.Walk (225, 168, eNoBlock, eWalkableAreas);
        if ((Money >= 5)  // error msg reads "end of input reached in middle of expression"
        {
            cChristian.LoseInventory (iStMichael);
            cChristian.AddInventory (iBlessedStMichael);
            Money -= 5;
            Display ("Your medallion has been blessed.");
        }
        else 
        {
            Display ("Not enough money.");
        }
    }
}



The "end of input reached in middle of expression" error is because you have two opening brackets and 1 closing one. "if ((Money >= 5)" should be "if (Money >= 5).

Laura Hunt

#4
Things I've noticed:

- Extra bracket in line 22, like CaptainD said.

- Money checks need to go inside each inventory item check.

- Your indentation (or more like, lack thereof) also makes things hard to read. When you have conditions nested inside of conditions, this doesn't help figure out what's going on.

Here's the corrected code:


Code: ags
    
function hFont_UseInv()
{      
       if (cChristian.ActiveInventory == (iEmptyBottle))
        {
          cChristian.Walk (225, 168, eNoBlock, eWalkableAreas);
 
          if (Money >= 5)
            {
              cChristian.LoseInventory (iEmptyBottle);
              cChristian.AddInventory (iHolyWater);
              Money -= 5;
              Display ("You have a bottle of Holy Water.");
             }
          else
            {
              Display ("Not enough money.");
             }
         }  // this closes the first "if" condition (for iEmptyBottle)

       if (cChristian.ActiveInventory == (iStMichael))
        {
          cChristian.Walk (225, 168, eNoBlock, eWalkableAreas);
 
          if (Money >= 5)  // removed the extra parenthesis that was causing an error
            {
              cChristian.LoseInventory (iStMichael);
              cChristian.AddInventory (iBlessedStMichael);
              Money -= 5;
              Display ("Your medallion has been blessed.");
             }
          else 
            {
               Display ("Not enough money.");
             }
      } // this closes the second "if" (for iStMichael)
 }


Also, for one-line commands, you don't have to use curly brackets, which might add to your confusion. So instead of:

Code: ags
else
{
Display("Blah blah blah");
}


Just do
Code: ags

else Display("Blah blah blah");

Zephyr

 :-D
Thanks so much everyone!  And a special thanks to Crimson Wizard and Laura Hunt for your straightforward, simple explanations and guidance. 
They do say that the older you get, the harder it is to learn a new language - and THIS old lady can confirm that!   (laugh)
I'm learning so much from you all though, so thanks again - I really appreciate it!

- Zephyr   :-* :-*
<3 Zephyr <3

Laura Hunt

Quote from: Zephyr on Wed 22/01/2020 13:46:01
:-D
Thanks so much everyone!  And a special thanks to Crimson Wizard and Laura Hunt for your straightforward, simple explanations and guidance. 
They do say that the older you get, the harder it is to learn a new language - and THIS old lady can confirm that!   (laugh)

I think quite a few of us can confirm that it's never too late ;)

btw, another possible issue you might run into, is that cChristian never gets to the shopkeeper, or wherever his "walk" command is supposed to take him. This is because you've made it non-blocking, which means that the next lines in the script will be executed immediately, which in turn means that your message of "Success" or "You don't have enough money" will pop up before he's even had the chance to walk up to his destination. If this is not what you want, and you want him to walk all the way up to (225, 168) before the shop message pops up, you should make the Walk command eBlock rather than eNoBlock.


Zephyr

Hi Laura,

Thanks again for that.  I'm also getting confused with the block/no-block thing.  The thing is, I also have an object animation running in the background and I wasn't sure if  the block command would interfere with that.  Could you maybe explain a little please?

Thanks again!
- Zephyr   :-\
<3 Zephyr <3

CrashPL

One thing I found incredibly useful is the code reidentation in Notepad++ - AGScript files can be easily beautified and reidented as C code via the TextFX plugin!

Khris

#9
Quote from: Zephyr on Thu 23/01/2020 10:21:39I'm also getting confused with the block/no-block thing.  The thing is, I also have an object animation running in the background and I wasn't sure if  the block command would interfere with that.  Could you maybe explain a little please?

I'll give it a try:

Any non-blocking thing keeps running in the background, even when blocking things start to happen. Calling the same non-blocking command twice in quick succession (for instance  player.Walk()) will lead to the first command being "overwritten" / discarded because the second command will run in the very next microsecond.

A blocking thing halts the current function until it completes (for instance .Animate-ing something using both eRepeat and eBlock will make the game permanently unresponsive since the blocking thing never finishes).

Say you want three characters to walk across the screen, blocking. You'd tell #1 & #2 to walk using eNoBlock, then tell #3 to walk using eBlock. That way they will all walk at the same time, and the game will be unresponsive until the 3rd one has reached her goal.

Another common situation is where you want multiple things to happen in sequence, like a character walking somewhere, then animating, then saying something.
If this is part of a cutscene and can be blocking, you can simply use the blocking versions of the three according commands and it'll work fine.
However if this is supposed to happen without the game getting unresponsive, you need to "wait" until the character has reached their goal first before you call the Animate command. Then you need to wait until the animation is complete before calling SayBackground(). The only way to do that currently is to use a state variable and repeatedly_execute to a) keep track of what the character is currently doing and b) check if they have finished yet.

Edit: actually, you could also use SetTimer() to schedule the 2nd and 3rd command, in case the number of frames for the walking and animating is constant

Laura Hunt

#10
Quote from: Zephyr on Thu 23/01/2020 10:21:39
Hi Laura,

Thanks again for that.  I'm also getting confused with the block/no-block thing.  The thing is, I also have an object animation running in the background and I wasn't sure if  the block command would interfere with that.  Could you maybe explain a little please?

Thanks again!
- Zephyr   :-\

Blocking commands simply pause the script and wait until the blocking action is finished in order to execute the next line, but they won't interfere with animations that you have already going on in the background.

You would usually use blocking actions when you need an action to be completed before something else starts. For example, say you have a radio playing music and you want your character to walk up to it and turn it off:

- If you don't use blocking, the commands "walk to radio" and "turn radio off" will be executed immediately one after the other, so the radio will get turned off before the character even takes a single step.
- If you use blocking, then the character will start walking, the script will wait patiently until the walk command is finished, and then resume executing the next line in the script, which in this example would be turning the radio off.

Hope this clears things up a bit!

Edit: Khris' explanation is a bit more comprehensive than mine, but since I already wrote this, might as well click "publish" anyway :)

Zephyr

Thanks again, Everyone!

      You really are very patient teachers! I'm going to play about with block/no-block for a while now - seeing it "in action" will make it clearer, I think.
You've been amazing, all of you.

- Zephyr   (nod) :)
<3 Zephyr <3

Zephyr

Hi again Everyone,

Here I am again with another bloody nested function - this one's a bit more complex though.

Here's what I have so far:

Code: ags

bool IsBagFull = false;
function iDrawstringBag_UseInv()
{
      if (cChristian.ActiveInventory == (iGroundNettles))
{
         cChristian.LoseInventory (iGroundNettles);
         BagEmpty = false;
}   
      if (cChristian.ActiveInventory == (iGroundBarberry))
 {
         cChristian.LoseInventory (iGroundBarberry);
         BagEmpty = false;
 }
      if (cChristian.ActiveInventory == (iGroundDust))
  {
         cChristian.LoseInventory (iGroundDust);
         BagEmpty = false;
  }
      if (cChristian.ActiveInventory == (iGroundBasil))
   {
          cChristian.LoseInventory (iGroundBasil);
          BagEmpty = false;
   }
      if (cChristian.ActiveInventory == (iGroundMandrake))
    {
         cChristian.LoseInventory (iGroundMandrake);
         BagEmpty = false;
    }
      if (cChristian.ActiveInventory == (iGroundLadySlippers))
     {
         cChristian.LoseInventory (iGroundLadySlippers);
         BagEmpty = false;
     }
         IsBagFull = true;
         Display ("The bag is full.");
         cChristian.LoseInventory (iDrawstringBag);
         cChristian.AddInventory (iFullBag);
}


I honestly thought I'd cracked this one on my own, but on running the game I find that I'm getting the "bag full" conditions when only ONE ingredient has gone into the empty bag!
Grrr! It's so annoying!  I'm pretty sure I've done something/neglected something really simple, but I can't see it for the life of me.
Any white knights/noble ladies out there who can help please?

-
Zephyr. :~(
<3 Zephyr <3

Laura Hunt

#13
Quote from: Zephyr on Mon 24/02/2020 12:16:43
I honestly thought I'd cracked this one on my own, but on running the game I find that I'm getting the "bag full" conditions when only ONE ingredient has gone into the empty bag!

Well, you're not keeping track of how many items have gone in the bag, as far as I can see. The only thing your code is doing is checking what active item is being used, and then simply setting IsBagFull to 'true' without checking any conditions. You need to establish some sort of condition for the bag to be full, for example, adding +1 to an integer each time an item is added, and performing a check at the end, something like:

Code: ags
if (cChristian.ActiveInventory == (iGroundNettles))
{
         cChristian.LoseInventory (iGroundNettles);
         BagEmpty = false;
         numberofitemsinthebag++; //you need to have previously defined this int, of course
}   


etc etc etc for each item.

And to check whether all items have been added (assuming there's 6 of them):

Code: ags
if (numberofitemsinthebag == 6) {
    IsBagFull = true;
    Display ("The bag is full.");
    cChristian.LoseInventory (iDrawstringBag);
    cChristian.AddInventory (iFullBag);
}

We've also mentioned this before, but proper indentation helps a lot when working with nested functions. The Tab key is your friend!

Zephyr

Hi Laura,

Thanks again for your help. Yeah, that makes perfect sense - I'll do that.
About the indentation - I actually have indented everything differently following your advice.  I guess not enough, huh?  OK, I'll make it much more obvious in the future.  You're quite right - it will make my code much clearer and easier to spot errors!   (nod)

What would I do without you?  ;-D

-
Zephyr.
x
<3 Zephyr <3

Laura Hunt

Quote from: Zephyr on Mon 24/02/2020 13:13:15
About the indentation - I actually have indented everything differently following your advice.  I guess not enough, huh?  OK, I'll make it much more obvious in the future.

I noticed, but what you're doing is indenting a little bit further with each "if", and it's... kind of unorthodox :-D

The most common convention (that I'm aware of, at least!) is to vertically align each set of curly brackets with its own condition. Using an example from further up this same thread:

Code: ags
function hFont_UseInv()
{      
       if (cChristian.ActiveInventory == (iEmptyBottle))
        {
          cChristian.Walk (225, 168, eNoBlock, eWalkableAreas);
 
          if (Money >= 5)
            {
              cChristian.LoseInventory (iEmptyBottle);
              cChristian.AddInventory (iHolyWater);
              Money -= 5;
              Display ("You have a bottle of Holy Water.");
             }
          else
            {


As you can see, at the the start you have "function" and its opening curly bracket just underneath it. Then you have an "if" (indented a bit deeper to show that it's inside the function), and its curly bracket is also just below it. Then you have yet another "if" indented even deeper (because it's inside the previous "if"), and this one also has its curly bracket just below it, and so on (and of course, all the closing brackets would follow, I just copied a snip.)

Of course this is not The Law Set in Stone and everybody has their own little quirks, habits and preferences but this is the basic idea :)

Zephyr

Hi Laura,

Right, I gotcha!  I have been finding it much better to indent the way I was (yeah I guess it was a bit unorthodox!) but I have to admit the way you do it makes it REALLY stand out, so I can definitely see the method in your "madness".  Time to learn new habits maybe.

Thanks again,

-
Zephyr.
:-*
<3 Zephyr <3

Khris

#17
Here's my approach:

Preparations:
Code: ags
// top of script
#define IC 6
InventoryItem* ingredients[IC];
bool ingredient_added[IC];

  // in game_start
  ingredients[0] = iGroundNettles;
  ingredients[1] = iGroundBarberry;
  ingredients[2] = iGroundDust;
  ingredients[3] = iGroundBasil;
  ingredients[4] = iGroundMandrake;
  ingredients[5] = iGroundLadySlippers;


Useinv function:
Code: ags
function iDrawstringBag_UseInv() {
  InventoryItem* ii = player.ActiveInventory;
	
  int i, found = -1;
  for (i = 0; i < IC; i  ) {
    if (ii == ingredients[i]) found = i;
  }
  // if not an ingredient
  if (found == -1) {
    player.Say("I'm not going to put that in the bag.");
    return; // exit function
  }
  // if already added
  if (ingredient_added[found]) {
    player.Say(String.Format("I already put %s in the bag.", ii.Name));
    return;
  }
  // not already added:
  player.Say("In the bag it goes.");
  player.LoseInventory(ii);
  ingredient_added[found] = true;
  // check if all ingredients are in the bag
  for (i = 0; i < IC; i  ) {
    if (!ingredient_added[i]) return; // not all ingredients, exit function
  }
  Display("The bag is full.");
  player.LoseInventory(iDrawstringBag);
  player.AddInventory(iFullBag);
}


By checking conditions in order and exiting the function prematurely, I can mostly avoid nested blocks.


Edit: fixed code errors (had been using i instead of found in two places)
Edit2: added LoseInventory line

Snarky

Laura gives a good summary of some of the principles of indentation, but I think it's worth adding a couple of things.

Quote from: Laura Hunt on Mon 24/02/2020 13:24:29
Of course this is not The Law Set in Stone and everybody has their own little quirks, habits and preferences but this is the basic idea :)

Yes and no. All of this is "by convention", but the convention is pretty damn strong. To the point where, for example, if you submitted a coding sample that was clearly not "properly" indented when applying for a job, you probably would not be considered (just like you probably wouldn't get a job in journalism if you submitted writing samples full of spelling errors). Not out of snobbery or whatever, but because it has been shown to be really important for writing correct code, and for making code easily readable.

And while there are some well-known variations and individual quirks, most things are pretty fixed.

To take the sample and fix it up a bit more:

Code: ags
function hFont_UseInv()
{
  if (cChristian.ActiveInventory == (iEmptyBottle))
  {
    cChristian.Walk (225, 168, eNoBlock, eWalkableAreas);
 
    if (Money >= 5)
    {
      cChristian.LoseInventory (iEmptyBottle);
      cChristian.AddInventory (iHolyWater);
      Money -= 5;
      Display ("You have a bottle of Holy Water.");
    }
    else
    {
          // ...
    }
  // ...
  }
}


(I've added in the brackets that would appear elsewhere in the code for clarity.)

The "corrections" here are first that we always use the same number of spaces for each indentation level (in this case 2, which is the default for AGS, but if you prefer 4 that's fine as well; just change the editor setting so that pressing TAB gives you 4 spaces): before, the number of spaces for each level was inconsistent. And second that we don't indent the brackets themselves, but align them with the statements they belong to: notice how before, the bracket for the first if-statement was under the "f" in if, while the second was under the space after the if. Fixing this ensures that two things that are on the same logical level of the program always have the same indentation. For example, if you have an if-block and then a matching else-block, they'll have the same alignment. It's sort of like a bullet-point list with multiple levels. When you have a sub-list, you always use the same level of indentation.

Also, a tip for convenience: If you highlight a number bunch of lines and press TAB, it will indent all of them by one level. If you press SHIFT+TAB, it will unindent them by one level.

Laura Hunt

#19
Quote from: Snarky on Mon 24/02/2020 14:02:01

The "corrections" here are first that we always use the same number of spaces for each indentation level (in this case 2, which is the default for AGS, but if you prefer 4 that's fine as well; just change the editor setting so that pressing TAB gives you 4 spaces): before, the number of spaces for each level was inconsistent. And second that we don't indent the brackets themselves, but align them with the statements they belong to: notice how before, the bracket for the first if-statement was under the "f" in if, while the second was under the space after the if. Fixing this ensures that two things that are on the same logical level of the program always have the same indentation. For example, if you have an if-block and then a matching else-block, they'll have the same alignment. It's sort of like a bullet-point list with multiple levels. When you have a sub-list, you always use the same level of indentation.

Very true, I was sloppy here but obviously you always want to be consistent as to where you place the bracket (like you said, always under the "f", or under the "i", or wherever, but always consistently).

Also, by "little quirks" I was referring to stuff like placing the bracket at the end of the line or on the next one, or using brackets and a new line for a one-line statement vs. doing if (bla) dostuff; and stuff like that. For example, I do the same as Khris and tend to place my opening bracket in the same line as my if statement rather than below the "i" or the "f":

Code: ags
if (found == -1) {
    player.Say("I'm not going to put that in the bag.");
    return; // exit function
  }


But yes, all you say is true and it's great that you clarified it :)

SMF spam blocked by CleanTalk