adventuregamestudio / ags

AGS editor and engine source code
Other
697 stars 160 forks source link

Character.FollowCharacter implementation limits parameters to 1 byte #928

Open ghost opened 5 years ago

ghost commented 5 years ago

This is not documented anywhere afaik, but FollowCharacter's internal implementation clamps the "distance" parameter to 1 byte (because it has 2 variables packed in a 16-bit field). This maybe worked in old days but does not anymore with high-resolution games.

This should be remade to accomodate at least 2 bytes for a distance (but then, why not just a common 32-bit integer).

ericoporto commented 5 years ago
character_followinfo.diff

[character_followinfo.diff](https://gist.github.com/ericoporto/6a15950e4cd8ce9b1f73155b47a4196d) ```diff diff --git a/Common/ac/characterinfo.cpp b/Common/ac/characterinfo.cpp index dbcbcf39..2cfb1561 100644 --- a/Common/ac/characterinfo.cpp +++ b/Common/ac/characterinfo.cpp @@ -31,7 +31,7 @@ void CharacterInfo::ReadFromFile(Stream *in) wait = in->ReadInt32(); flags = in->ReadInt32(); following = in->ReadInt16(); - followinfo = in->ReadInt16(); + followinfo = in->ReadInt32(); idleview = in->ReadInt32(); idletime = in->ReadInt16(); idleleft = in->ReadInt16(); @@ -81,7 +81,7 @@ void CharacterInfo::WriteToFile(Stream *out) out->WriteInt32(wait); out->WriteInt32(flags); out->WriteInt16(following); - out->WriteInt16(followinfo); + out->WriteInt32(followinfo); out->WriteInt32(idleview); out->WriteInt16(idletime); out->WriteInt16(idleleft); diff --git a/Common/ac/characterinfo.h b/Common/ac/characterinfo.h index f3140d15..65ba1ccb 100644 --- a/Common/ac/characterinfo.h +++ b/Common/ac/characterinfo.h @@ -44,7 +44,7 @@ using namespace AGS; // FIXME later #define OCHF_SPEECHCOL 0xff000000 #define OCHF_SPEECHCOLSHIFT 24 #define UNIFORM_WALK_SPEED 0 -#define FOLLOW_ALWAYSONTOP 0x7ffe +#define FOLLOW_ALWAYSONTOP 0x7ffffffe struct CharacterExtras; // forward declaration // remember - if change this struct, also change AGSDEFNS.SH and @@ -57,7 +57,7 @@ struct CharacterInfo { int x, y, wait; int flags; short following; - short followinfo; + int followinfo; int idleview; // the loop will be randomly picked short idletime, idleleft; // num seconds idle before playing anim short transparency; // if character is transparent diff --git a/Engine/ac/character.cpp b/Engine/ac/character.cpp index 5eb3a21c..07c56cce 100644 --- a/Engine/ac/character.cpp +++ b/Engine/ac/character.cpp @@ -495,7 +495,7 @@ void Character_FollowCharacter(CharacterInfo *chaa, CharacterInfo *tofollow, int else chaa->following = tofollow->index_id; - chaa->followinfo=(distaway << 8) | eagerness; + chaa->followinfo=(distaway << 16) | eagerness; chaa->flags &= ~CHF_BEHINDSHEPHERD; diff --git a/Engine/ac/characterinfo_engine.cpp b/Engine/ac/characterinfo_engine.cpp index 8f6f1a5a..e16a69c7 100644 --- a/Engine/ac/characterinfo_engine.cpp +++ b/Engine/ac/characterinfo_engine.cpp @@ -388,7 +388,7 @@ void CharacterInfo::update_character_follower(int &aa, int &numSheep, int *follo } // not moving, but should be following another character else if ((following >= 0) && (doing_nothing == 1)) { - short distaway=(followinfo >> 8) & 0x00ff; + short distaway=(followinfo >> 16) & 0x00ffff; // no character in this room if ((game.chars[following].on == 0) || (on == 0)) ; else if (room < 0) { @@ -401,7 +401,7 @@ void CharacterInfo::update_character_follower(int &aa, int &numSheep, int *follo } } // wait a bit, so we're not constantly walking - else if (Random(100) < (followinfo & 0x00ff)) ; + else if (Random(100) < (followinfo & 0x00ffff)) ; // the followed character has changed room else if ((room != game.chars[following].room) && (game.chars[following].on == 0)) @@ -446,7 +446,7 @@ void CharacterInfo::update_character_follower(int &aa, int &numSheep, int *follo } else if ((abs(game.chars[following].x - x) > distaway+30) | (abs(game.chars[following].y - y) > distaway+30) | - ((followinfo & 0x00ff) == 0)) { + ((followinfo & 0x00ffff) == 0)) { // in same room int goxoffs=(Random(50)-25); // make sure he's not standing on top of the other man ```

I was curious about changing to a short... But I noticed this isn't all the picture. I guess in agsdefns.sh, that mysterious reserved_a array (on Character struct) is actually these values, and the properties under Strict defines is related to the OldCharacterInfo . I think the reserved_a needs to be increased (from 28 to 29) and the CharacterInfo.following property also needs to become an int32, if the reserved_a array is connected to the struct. This isn't scope of current version, I just always been curious about those reserved properties. They aren't mentioned on Engine data structure restrictions or A quick guide to Script API.

ghost commented 5 years ago

Yes, obviously, if we keep plugin compatibility, this will have to be added separately, maybe in CharacterExtras struct, and keep old variable just for struct alignment.

If plugin compatibility is not a concern, then script problem could be resolved using access remapping mechanism.

ghost commented 5 years ago

I just always been curious about those reserved properties.

They are either originally reserved for the purpose of adding actual variables there later, or were useful variables ones but deprecated. In latter case they got renamed into "reserved_..." simply to warn user to not use these.

ericoporto commented 5 years ago

When you say plug-in compatibility, you are thinking about a plug-in that access the character struct, right? From the top of my head, I think only AGS Lua plug-in did it, but it is long unmaintained.