Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix: [Script] ScriptBase::Rand() return value was between -MIN(int32) and MAX(int32) #10443

Merged
merged 1 commit into from
Feb 10, 2023
Merged

Fix: [Script] ScriptBase::Rand() return value was between -MIN(int32) and MAX(int32) #10443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
150 changes: 75 additions & 75 deletions regression/regression/result.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,9 @@
abs( 21): 21

--AIBase--
Rand(): -2062310602
Rand(): -1780331126
Rand(): -397928569
Rand(): 2232656694
Rand(): 2514636170
Rand(): 3897038727
RandRange(0): 0
RandRange(0): 0
RandRange(0): 0
Expand Down Expand Up @@ -421,36 +421,36 @@
1099 => 46158
Randomize ListDump:
1 => 688298322
2 => -1709546982
2 => 2585420314
1000 => 1701392078
1001 => -1630848421
1002 => -886500935
1003 => -196324972
1004 => -436037402
1005 => -520341784
1006 => -1485224804
1007 => -311036236
1008 => -1503442439
1009 => -110945695
1010 => -82825175
1001 => 2664118875
1002 => 3408466361
1003 => 4098642324
1004 => 3858929894
1005 => 3774625512
1006 => 2809742492
1007 => 3983931060
1008 => 2791524857
1009 => 4184021601
1010 => 4212142121
1011 => 46859773
1012 => -1199223018
1013 => -1190555925
1012 => 3095744278
1013 => 3104411371
1014 => 326384434
1015 => 1486817960
1016 => -1411425597
1017 => -508426854
1016 => 2883541699
1017 => 3786540442
1018 => 820019294
1019 => 710762995
1020 => -760867032
1021 => -709611146
1020 => 3534100264
1021 => 3585356150
1022 => 732190215
1023 => 236336673
1024 => 740596257
1025 => 1135321785
1026 => 2067474156
1027 => -1395683607
1028 => -240528699
1027 => 2899283689
1028 => 4054438597
1029 => 928616892
1030 => 1712486685
1031 => 1994118287
Expand All @@ -462,46 +462,46 @@
1037 => 1359697121
1038 => 1888433963
1039 => 941623020
1040 => -1925663292
1041 => -771540264
1042 => -1058341359
1040 => 2369304004
1041 => 3523427032
1042 => 3236625937
1043 => 182127597
1044 => 646955927
1045 => -1424621714
1045 => 2870345582
1046 => 623062612
1047 => -1986955586
1048 => -1268826980
1049 => -456776220
1051 => -1112555329
1052 => -1532134052
1047 => 2308011710
1048 => 3026140316
1049 => 3838191076
1051 => 3182411967
1052 => 2762833244
1053 => 1960404034
1054 => 1573325453
1055 => -316619303
1055 => 3978347993
1056 => 699712177
1057 => 863274966
1058 => 1728276475
1059 => -246695889
1059 => 4048271407
1060 => 1919485436
1061 => 111273464
1062 => 125435213
1063 => 155132602
1064 => -171674076
1064 => 4123293220
1065 => 655046914
1066 => 1577399562
1067 => 1028818150
1068 => 447058239
1069 => -1057920269
1070 => -1326215323
1071 => -198688588
1069 => 3237047027
1070 => 2968751973
1071 => 4096278708
1072 => 1523643051
1073 => 231373233
1074 => 1121759962
1075 => 1449439846
1076 => -1615270753
1077 => -1509293864
1076 => 2679696543
1077 => 2785673432
1078 => 2116903943
1079 => 672822173
1080 => -969573911
1080 => 3325393385
1081 => 1589904755
1082 => 1148782015
1083 => 663503316
Expand All @@ -510,54 +510,54 @@
1086 => 402172048
1087 => 1812250453
1088 => 667300501
1089 => -1838825777
1090 => -856474776
1089 => 2456141519
1090 => 3438492520
1091 => 420696035
1092 => 2131427774
1093 => -435303548
1094 => -160883878
1093 => 3859663748
1094 => 4134083418
1095 => 1969629634
1096 => -555794155
1097 => -835119691
1098 => -1460907909
1099 => -1146924084
1096 => 3739173141
1097 => 3459847605
1098 => 2834059387
1099 => 3148043212
KeepTop(10):
1 => 688298322
2 => -1709546982
2 => 2585420314
1000 => 1701392078
1001 => -1630848421
1002 => -886500935
1003 => -196324972
1004 => -436037402
1005 => -520341784
1006 => -1485224804
1007 => -311036236
1001 => 2664118875
1002 => 3408466361
1003 => 4098642324
1004 => 3858929894
1005 => 3774625512
1006 => 2809742492
1007 => 3983931060
KeepBottom(8):
1000 => 1701392078
1001 => -1630848421
1002 => -886500935
1003 => -196324972
1004 => -436037402
1005 => -520341784
1006 => -1485224804
1007 => -311036236
1001 => 2664118875
1002 => 3408466361
1003 => 4098642324
1004 => 3858929894
1005 => 3774625512
1006 => 2809742492
1007 => 3983931060
RemoveBottom(2):
1000 => 1701392078
1001 => -1630848421
1002 => -886500935
1003 => -196324972
1004 => -436037402
1005 => -520341784
1001 => 2664118875
1002 => 3408466361
1003 => 4098642324
1004 => 3858929894
1005 => 3774625512
RemoveTop(2):
1002 => -886500935
1003 => -196324972
1004 => -436037402
1005 => -520341784
1002 => 3408466361
1003 => 4098642324
1004 => 3858929894
1005 => 3774625512
RemoveList({1003, 1004}):
1002 => -886500935
1005 => -520341784
1002 => 3408466361
1005 => 3774625512
KeepList({1003, 1004, 1005}):
1005 => -520341784
1005 => 3774625512
AddList({1005, 4000, 4001, 4002}):
1005 => 1005
4000 => 8000
Expand Down
15 changes: 9 additions & 6 deletions src/script/api/script_base.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,33 +13,36 @@

#include "../../safeguards.h"

/* static */ uint32 ScriptBase::Rand()
/* static */ SQInteger ScriptBase::Rand()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it better to use int64 than SQInteger? I always thought SQInteger was specifically used outside the API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SQInteger is the type used in every squirrel internal functions (especially sq_pushinteger() to pass a number to the squirrel VM and sq_getinteger() to get a number from the VM). It is indeed an int64, but I think using SQInteger is better as it hides implementation details from the API.

{
return ScriptObject::GetRandomizer().Next();
}

/* static */ uint32 ScriptBase::RandItem(int unused_param)
/* static */ SQInteger ScriptBase::RandItem(SQInteger unused_param)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused_param doesn't really need to change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but my idea would be to use SQInteger everywhere in the API for generic numbers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then, as a reminder for a later (cleanup) story: remove the casts from squirrel_helper_type.hpp:49-51 as well as 57.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah you mean squirrel_helper.hpp

{
return ScriptBase::Rand();
}

/* static */ uint ScriptBase::RandRange(uint max)
/* static */ SQInteger ScriptBase::RandRange(SQInteger max)
{
max = Clamp<SQInteger>(max, 0, UINT32_MAX);
return ScriptObject::GetRandomizer().Next(max);
}

/* static */ uint32 ScriptBase::RandRangeItem(int unused_param, uint max)
/* static */ SQInteger ScriptBase::RandRangeItem(SQInteger unused_param, SQInteger max)
{
return ScriptBase::RandRange(max);
}

/* static */ bool ScriptBase::Chance(uint out, uint max)
/* static */ bool ScriptBase::Chance(SQInteger out, SQInteger max)
{
out = Clamp<SQInteger>(out, 0, UINT32_MAX);
max = Clamp<SQInteger>(max, 0, UINT32_MAX);
EnforcePrecondition(false, out <= max);
return ScriptBase::RandRange(max) < out;
}

/* static */ bool ScriptBase::ChanceItem(int unused_param, uint out, uint max)
/* static */ bool ScriptBase::ChanceItem(SQInteger unused_param, SQInteger out, SQInteger max)
{
return ScriptBase::Chance(out, max);
}
18 changes: 12 additions & 6 deletions src/script/api/script_base.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,50 +25,56 @@ class ScriptBase : public ScriptObject {
* Get a random value.
* @return A random value between 0 and MAX(uint32).
*/
static uint32 Rand();
static SQInteger Rand();

/**
* Get a random value.
* @param unused_param This parameter is not used, but is needed to work with lists.
* @return A random value between 0 and MAX(uint32).
*/
static uint32 RandItem(int unused_param);
static SQInteger RandItem(SQInteger unused_param);

/**
* Get a random value in a range.
* @param max The first number this function will never return (the maximum it returns is max - 1).
* The value will be clamped to 0 .. MAX(uint32).
* @return A random value between 0 .. max - 1.
*/
static uint RandRange(uint max);
static SQInteger RandRange(SQInteger max);

/**
* Get a random value in a range.
* @param unused_param This parameter is not used, but is needed to work with lists.
* @param max The first number this function will never return (the maximum it returns is max - 1).
* The value will be clamped to 0 .. MAX(uint32).
* @return A random value between 0 .. max - 1.
*/
static uint RandRangeItem(int unused_param, uint max);
static SQInteger RandRangeItem(SQInteger unused_param, SQInteger max);

/**
* Returns approximately 'out' times true when called 'max' times.
* After all, it is a random function.
* @param out How many times it should return true.
* The value will be clamped to 0 .. MAX(uint32).
* @param max Out of this many times.
* The value will be clamped to 0 .. MAX(uint32).
* @pre \a out is at most equal to \a max.
* @return True if the chance worked out.
*/
static bool Chance(uint out, uint max);
static bool Chance(SQInteger out, SQInteger max);

/**
* Returns approximately 'out' times true when called 'max' times.
* After all, it is a random function.
* @param unused_param This parameter is not used, but is needed to work with lists.
* @param out How many times it should return true.
* The value will be clamped to 0 .. MAX(uint32).
* @param max Out of this many times.
* The value will be clamped to 0 .. MAX(uint32).
* @pre \a out is at most equal to \a max.
* @return True if the chance worked out.
*/
static bool ChanceItem(int unused_param, uint out, uint max);
static bool ChanceItem(SQInteger unused_param, SQInteger out, SQInteger max);
};

#endif /* SCRIPT_BASE_HPP */