AGS4 Best Scripting Practices [Ben304 devlog case]

[eri0o]

[Mod note: This thread was split off from I started a devlog]

Hey, you should definitely throw an update here as I didn't knew you were still writing them. Anyway, I only read the screenshot of the editor code in the last one, and I don't understand the while clause there, because there's no wait and you aren't animating each 1 take/give of energy, if you don't plan to do it this way, then you can just calculate the amount to take and take in one go instead of using while.

Ah, since you are using ags4, which we haven't formally made a proper manual yet, I think these are two texts that may be useful about the new compiler

https://github.com/adventuregamestudio/ags/wiki/New-compiler%27s-end-user-cheat-sheet

https://github.com/adventuregamestudio/ags/wiki/Features-of-the-new-Script-Compiler

[ThreeOhFour]

I figured it would be a little annoying if I endlessly bumped the thread myself with nobody else replying. But maybe I can make a habit of that. Currently I'm posting weekly, and I've been updating the first post here when I do. I've put up this week's post just this hour.

The while loop is probably a strange way of doing things! Edmundo already told me this was not the best approach, and given me an alternate way of approaching things. I haven't tried his way yet, but I like it when I can follow a train of logic step by step in my head, and this is the way that allowed me to do that here. I know I am not a very competent programmer. 

Very good to have those texts posted here, too. Considering how many years it's been since I tried building a game in AGS I have been referring to the manual regularly for even the most basic things, let alone these nice new things, but I'll make sure I bookmark these. It took me quite a while to figure out how to remove walkable areas with AGS 4, but most of my other issues have been that I'm still just figuring things out.

[Snarky]

If we compare:

Code: ags

  while(thisEnergy < thisMaxEnergy && playerEnergy > 0)
  {
    thisEnergy = this.ChangeProperty(ENERGY, 1);
    playerEnergy = player.ChangeProperty(ENERGY, -1);
  }

(Which is how lines 114-118 in the sample Ben showed might look after other suggested improvements.)

With this:

Code: ags

  int energyChange = min(thisMaxEnergy-thisEnergy, playerEnergy);
  thisEnergy = this.ChangeProperty(ENERGY, energyChange);
  playerEnergy = this.ChangeProperty(ENERGY, -energyChange);

(Which is one way to do it without a loop, using a min() helper function.)

... I'm not convinced it's much of an improvement. It might be less work for the computer, but it was harder to write, and I think harder to follow. And the only lines saved are the {}s.

[ThreeOhFour]

In the interests of the discussion, here's the function as it stands now:

Code: ags

function Flux(this Hotspot*)
{
  //======================= initialisation=======================================================================================||
  int player_energy = player.GetProperty(ENERGY);
  int hotspot_energy = this.GetProperty(ENERGY);
  int player_max = player.GetProperty(MAX_ENERGY);
  int hotspot_max = this.GetProperty(MAX_ENERGY);
  int x_coord = this.GetProperty(X_COORDINATE);
  int y_coord = this.GetProperty(Y_COORDINATE);
  ButtonReset(); //turn off the gui controls we have currently active and all related effects
  ButtonLabel.Text = " "; //get rid of the button label text
  this.HotspotWalk(); //walk to the hotspot, face the correct direction, apply any appropriate offsets and fire the default animation if one has been set
  if (this.GetProperty(PROTECTED) == true) this.TakeProtection(); // first check if the hotspot has been protected from interference and remove if so
  //=============================================================================================================================||
  
  
  //======================== end the function if there is no energy==============================================================||
  if (hotspot_energy == 0 && player_energy == 0)
  {
    zNoEnergy(); 
  }
  //=============================================================================================================================||
  
  
  //======================== energy transfer for hotspot has energy==============================================================||
  else if (hotspot_energy != 0) //if the hotspot DOES have energy
  {
    if (player_energy == player_max && hotspot_energy == hotspot_max)
    {
      zPowerFull(); //first make sure the player can take it
    }
    else if (player_energy == player_max && hotspot_energy != hotspot_max)//if player's at energy limit, put energy into the hotspot even if the hotspot has some energy
    {
      DrawPowerParticles(x_coord, y_coord, Give);
      this.GivePower();
      while (hotspot_energy != hotspot_max && player_energy != 0) //keep giving it until the player's energy is empty OR the hotspot can take no more power
      {
        this.ChangeProperty(ENERGY, 1);
        hotspot_energy++;
        player.ChangeProperty(ENERGY, -1);
        player_energy--;
      }
    }
    else
    {
      this.TakePower();//I think the reason I'm not handling DrawPowerParticles() within TakePower() & GivePower() is that the order has to be different to look right? I forget. Seems right.
      DrawPowerParticles(x_coord, y_coord, Take);
      while (hotspot_energy != 0 && player_energy != player_max) //then keep taking it until the player's energy is full OR the hotspot has no power left
      {
        this.ChangeProperty(ENERGY, -1);
        hotspot_energy--;
        player.ChangeProperty(ENERGY, 1);
        player_energy++;
      }
    }
  }
  //=============================================================================================================================||
  
  
  //======================== energy transfer for player has energy===============================================================||
  else if (player_energy != 0) // the hotspot doesn't have some energy but the player does
  {
    if (hotspot_energy == hotspot_max) zAlreadyPowered(); // First check that the hotspot can take power
    else
    {
      DrawPowerParticles(x_coord, y_coord, Give);
      this.GivePower();
      while (hotspot_energy != this.GetProperty(MAX_ENERGY) && player_energy != 0) //then keep giving it until the player's energy is empty OR the hotspot can take no more power
      {
        this.ChangeProperty(ENERGY, 1);
        hotspot_energy++;
        player.ChangeProperty(ENERGY, -1);
        player_energy--;
      }
    }
  }
  //=============================================================================================================================||
  
  
  //====================================================== reset ================================================================||
  this.FinishHotspot();//Plays the interaction animation backwards, removes any offsets applied, resets the player's loop
  //=============================================================================================================================||
}

For reference, the maximum amount that this transfer will ever process at this point is a value of 3. This may change at some point, but it's never going to require many cycles of the loop in any event.

Apologies to cat if this is considered to be getting off-topic but I don't have any specific questions to ask about it, I'm mostly interested in talking about it!

[Crimson Wizard]

... I'm not convinced it's much of an improvement. It might be less work for the computer, but it was harder to write, and I think harder to follow. And the only lines saved are the {}s.

TBH I'm very much surprised by such view, I have a completely opposite opinion on this.
For me it's not even the question of performance, but a question of clearly declaring a purpose. IMO the second variant has a more clear intent than the second.
For the same reason, it would be easier to write for me personally, I would just do that automatically by reflex.

But I would suggest to make a function that does "Transfer energy" from object 1 to object 2.

[Snarky]

TBH I'm very much surprised by such view, I have a completely opposite opinion on this.
For me it's not even the question of performance, but a question of clearly declaring a purpose. IMO the second variant has a more clear intent than the second.
For the same reason, it would be easier to write for me personally, I would just do that automatically by reflex.

To me it seems that the intent is "transfer as much energy as possible from player to hotspot; i.e. so that player is empty or hotspot is full." In the first version, that intent is clearly expressed in the loop condition, while in the second it is not apparent to me anywhere in the code: I have to mentally perform the same calculation as the computer in order to determine that that will be the outcome. ("Right, so there are two cases. (1) If thisMaxEnergy-thisEnergy is smaller, then thisEnergy becomes... thisEnergy + thisMaxEnergy - thisEnergy, so just thisMaxEnergy, and playerEnergy becomes... something. (Greater than 0? Let's hope so!) (2) If playerEnergy is smaller, playerEnergy becomes 0, and thisEnergy becomes thisEnergy + playerEnergy, so that's pretty straightforward at least. OK, so what this chunk of code does is...")

The ChangeProperty stuff makes it harder to follow, but even with plain variables I find this:

Code: ags

  while(thisEnergy < thisMaxEnergy && playerEnergy > 0)
  {
    thisEnergy++;
    playerEnergy--;
  }

Easier to understand than this:

Code: ags

  int energyChange = min(thisMaxEnergy-thisEnergy, playerEnergy);
  thisEnergy += energyChange;
  playerEnergy -= energyChange;

But I would suggest to make a function that does "Transfer energy" from object 1 to object 2.

If these are the only three instances in the game where that happens, I'd probably not break it out. Though I might refactor the function logic to reduce it to two.

[Crimson Wizard]

If a line with "min" looks too complicated to read, then it may be replaced with another helper function with speaking name.

For a quick example:

Code: ags

int takeMostToFill(int reserve, int currentLevel, int maxLevel);

int energyChange = takeMostToFill(playerEnergy, thisEnergy, thisMaxEnergy);

[eri0o]

I just agree with CW because in my head I try to run the code and the while loop will take a while for me to grasp it and the min I can look and get the result in a single go. Like, in my head.

Spoiler

If used the while it would be solely for the purpose of also doing some graphical feedback - and that while would then have like a wait there. But even then I think a single go is better because I can then not couple the logic with the animation timing. But overall the more interesting aspect of this is how different people perceive code differently.

Seeing this code I would refactor for something completely different, I don't have time right now, but in my head I have like an intermediary managed struct per entity with all things necessary and it's this intermediary managed struct that I would pass around to account for the lack of generics, it would be a bit verbose but I would try to repeat myself a lot only where logic is simple - something that sets this managed struct and something that gets from it and sets the things back, and then take my time polishing the intermediate code that does the animation - things that are visual require/can have pretty much infinite tweaking. So I would make so that changing any code that modify actual visuals as easy as possible.

[close]

[Snarky]

If a line with "min" looks too complicated to read, then it may be replaced with another helper function with speaking name.

IMO the logic is still less transparent, even aside from quibbling over whether the function name is easy to understand (I would suggest "maxPossibleTransfer"), and now you have a function with a bunch of non-obvious parameters that you have to inspect. Instead I'd add a comment to the energyChange declaration to explain the intent of this variable and the logic of the expression:

Code: ags

  // As much energy as we can possibly transfer, either to drain player completely or fill this hotspot completely
  int energyChange = min(playerEnergy, thisMaxEnergy-thisEnergy);

But all this is a lot of work to fix something that I don't really see as a problem in the first place. The loop is, to me, probably the most intuitive and obvious expression of the intent possible. The only drawback is the computer engineer's bias against using loops to do calculations that can be solved analytically (and, it sounds like for @eri0o, that expectation causing confusion).

[Crimson Wizard]

Well, I disagree, and furthermore this is frustrating to me to the point when my head begins to hurt, so I'd rather just not continue this conversation.
I will try to restrain myself from posting in these discussions further.

[ThreeOhFour]

Seeing this code I would refactor for something completely different, I don't have time right now, but in my head I have like an intermediary managed struct per entity with all things necessary and it's this intermediary managed struct that I would pass around to account for the lack of generics, it would be a bit verbose but I would try to repeat myself a lot only where logic is simple

When I started trying to figure out how I would approach all of this I was debating whether to use custom properties or structs. In the end it seemed to me that custom properties were a much nicer approach - in order to set a hotspot up I can click directly on it in the room editor and assign the necessary values while looking at it, which is very easy and convenient.

While this might look at little clumsy, I will hopefully never have to edit this function again for the duration of developing the game, while I will be adding hotspots for months to come. Having to populate a struct with information each time is quite doable, but feels like it would be less enjoyable. I'm very happy to take the advice, I know my weaknesses in terms of writing code, but from a development point of view this approach seemed to be the one with the least friction for me while implementing gameplay.

And apologies for sparking a debate, definitely not my intent! I appreciate everybody's input. I have very little experience doing this kind of work.

[eri0o]

You kidding? This is all super interesting conversation. I am super happy both reading and writing, I don't know how to convey my face when writing  .

But I would still use CustomProperties as you did because I think it's cleaner - plus it will be even better right in the property grid soon  . What I meant is I would internally populate it from the custom property. I will see if I can come up with a minor refactor for this but probably only tomorrow morning for you which will be during midnight for me 

[Snarky]

When I started trying to figure out how I would approach all of this I was debating whether to use custom properties or structs.

While this might look at little clumsy, I will hopefully never have to edit this function again for the duration of developing the game

I'd forgotten that you were using AGS4, actually. In that case I would wrap these custom properties in extender attributes, which cuts out a bunch of the boilerplate elsewhere and solves most of the custom property drawbacks. I've never actually used it, but from the documentation I guess it should look like this?

Code: ags

// Module Header:

import attribute int Energy(this Character);
import attribute int MaxEnergy(this Character);
import attribute int Energy(this Hotspot);
import attribute int MaxEnergy(this Hotspot);
import attribute int X(this Hotspot);
import attribute int Y(this Hotspot);

// Module Script:

// Character.Energy
int get_Energy(this Character)                { return this.GetProperty(ENERGY); }
void set_Energy(this Character, int value)    { this.SetProperty(ENERGY, value); }
// Hotspot.Energy
int get_Energy(this Hotspot)                  { return this.GetProperty(ENERGY); }
void set_Energy(this Hotspot, int value)      { this.SetProperty(ENERGY, value); }
// Character.MaxEnergy
int get_MaxEnergy(this Character)             { return this.GetProperty(MAX_ENERGY); }
void set_MaxEnergy(this Character, int value) { this.SetProperty(MAX_ENERGY, value); }
// Hotspot.MaxEnergy
int get_MaxEnergy(this Hotspot)               { return this.GetProperty(MAX_ENERGY); }
void set_MaxEnergy(this Hotspot, int value)   { this.SetProperty(MAX_ENERGY, value); }
// Hotspot.X
int get_X(this Hotspot)                       { return this.GetProperty(X_COORDINATE); }
void set_X(this Hotspot, int value)           { this.SetProperty(X_COORDINATE, value); }
// Hotspot.Y
int get_Y(this Hotspot)                       { return this.GetProperty(Y_COORDINATE); }
void set_Y(this Hotspot, int value)           { this.SetProperty(Y_COORDINATE, value); }

(The documentation doesn't actually say to use import for the attribute, but that seems most consistent with how other things work.)

Then you can simply use player.Energy and this.Energy (etc.) without needing either local variables or lots of manual calls to SetProperty/GetProperty. Instead of a ChangeProperty helper function and player.ChangeProperty(ENERGY,1) you can just do player.Energy++, for example.

[ThreeOhFour]

This looks very nice to use. I will give this a try tomorrow and see how I get on, cheers!

[ThreeOhFour]

I tried setting an attribute today, following that example, but got this error:

"Attributes.ash(2): Error (line 2): expected variable or function after import, not 'attribute'"

I tried a few adjustments but as I am ignorant of the correct way to use these I didn't accomplish much other than getting different errors, so I figured I would post here!

Seeing as this is a conversation about best practices, I may as well ask about other ways of doing a single action to multiple elements. Let's say I want to draw l number of lines, using a function call that calculates their various positions based on an incrementing variable. Is there an alternative way of doing this to either writing it all out line by line or calling:

Code: ags

for (int l = 0; l != 17; l++)
{
  DrawLine(l);
}

I was thinking about this today, and genuinely do not know another way of handling this. Is there something obvious I am missing?

[Crimson Wizard]

Make sure you are using the new v4 script compiler. There's a Script compiler selection in General Settings -> Compiler.

[Snarky]

Seeing as this is a conversation about best practices, I may as well ask about other ways of doing a single action to multiple elements. Let's say I want to draw l number of lines, using a function call that calculates their various positions based on an incrementing variable. Is there an alternative way of doing this to either writing it all out line by line or calling:

Code: ags

for (int l = 0; l != 17; l++)
{
  DrawLine(l);
}

Does this example have anything to do with the (very cool) lightning arc effect?



A loop is the usual way to apply actions to multiple elements in AGS. Some other languages have a "map" function that allows you to apply another function to all members of a collection directly, but AGS doesn't have anything like that.

But even if you use a loop, there are different ways to do it. The loop could go inside the Drawline function and you call it with an array of line segments, for example.

Another approach could be a recursive function (one that calls itself). You'd have to restructure things a bit so you don't have 17 elements defined in advance, but your Drawline(l) keeps subdividing the problem by calling itself, splitting it up until it reaches the most basic operation. (So, you might call it with the start and end points of the whole arc, and it would check the distance and decide whether to split it up into sections, calling itself to generate each section, until the sections are short enough to just draw a line.) Sometimes this can be an elegant way to solve a problem.

Be careful about recursion, though: it can easily grow out of control on larger problems, with thousands of function calls that can overwhelm the engine.

There's also something called "tail recursion." An example of that would be something like:

Code: ags

managed struct LineSegment
{
  Point startPoint;
  Point endPoint;
  LineSegment* nextSegment;

  import void Draw(DrawingSurface* canvas);
};

void LineSegment::Draw(DrawingSurface* canvas)
{
  // TODO: Draw a line from startPoint to endPoint

  if(nextSegment!=null)
    nextSegment.Draw();
}

With this we can have a linked list of LineSegments, with each segment having a link (a pointer) to the next one. (This structure doesn't really make sense if we want the segments to connect, since the endPoint of one segment needs to be the same as the startPoint of the next, but we'll ignore that for this example.) And you see that the last line of LineSegment.Draw() is another call to LineSegment.Draw(). So if firstSegment is our first segment and we call firstSegment.Draw(), it will in turn call firstSegment.nextSegment.Draw() (the second segment), and so on, until it reaches the end.

[Crimson Wizard]

Note that with the linked list you do not have to do recursion, you may do a classic loop over linked list; pseudo-code:

Code: ags

segment = firstSegment;
while (segment != null)
{
    DrawSegment(segment);
    segment = segment.nextSegment;
}

or same with the "for" loop:

Code: ags

for (LineSegment *segment = firstSegment; segment != null; segment = segment.nextSegment)
{
    DrawSegment(segment);
}

[ThreeOhFour]

Make sure you are using the new v4 script compiler. There's a Script compiler selection in General Settings -> Compiler.

That was the solution, thank you very much! And now I can just call Character.Energy, what a superbly useful feature this is!

Does this example have anything to do with the (very cool) lightning arc effect?

I did this one with a for loop, but was planning on writing another effect today which is a little more difficult for me to conceptualise because I'm not very good at planning things around modular arithmetic. Though I was very grateful earlier today when I was setting sprites in a loop based on a calculation and found that AGS supports the % remainder operator. But I figured I should investigate what other options there are before I start trying to build the effect!

I have heard of recursion in code but knew absolutely nothing about it, that's fascinating! This in particular sounds incredibly empowering:

"it would check the distance and decide whether to split it up into sections, calling itself to generate each section, until the sections are short enough to just draw a line"

Time for me to do some more study. I am learning so much lately.

Note that with the linked list you do not have to do recursion, you may do a classic loop over linked list; pseudo-code:

This is a new concept to me, and it seems very useful. Thank you!

[eri0o]

Hey, if you put the lighting strike in an Overlay, you can change it's color with tint or at least fade it's transparency - I don't remember if tween module has an ags4 version that supports these things, if it does it will be easy, if it doesn't it will require a bit more of code. @edmundito ?