MantisBT - Zandronum
View Issue Details
IDProjectCategoryView StatusDate SubmittedLast Update
0001698Zandronum[All Projects] Bugpublic2014-02-09 02:062018-09-30 21:35
ReporterDusk 
Assigned ToDusk 
PriorityhighSeveritymajorReproducibilityalways
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version 
Target Version1.3Fixed in Version1.3 
Summary0001698: botscript rand() is just plain wrong
Descriptionbotcommands.cpp:867:

    g_iReturnInt = g_RandomBotCmdSeed.Random( ) % lMax;
    while ( g_iReturnInt < lMin )
        g_iReturnInt += ( lMax - lMin );


first of all, this causes lMax not to be a possible return value. this is in direct clash with ACS's random() which is confusing.

secondly, this causes bias. if suppose rand is called with, say args 5 and 9, this would first get seed.Random() % 9, which gets a random value between 0 and 8 inclusive.

the while loop then offsets the values. possible outcomes:
0 (less than 5): -> 4 -> 8
1 (less than 5): -> 5
2 (less than 5): -> 6
3 (less than 5): -> 7
4 (less than 5): -> 8
5
6
7
8

overall probability:
- 5: 2 / 9 = 22.2%
- 6: 2 / 9 = 22.2%
- 7: 2 / 9 = 22.2%
- 8: 3 / 9 = 33.3%
- 9: 0 / 9 = 0%

notice how it favors the value 8. suggested change:

    g_iReturnInt = g_RandomBotCmdSeed.Random( ) % ( lMax - lMin + 1 ) + lMin;
Steps To Reproduce
Additional Informationif we absolutely have to keep the original behavior for backwards compatibility, I suggest we introduce a new random() for this behavior. I can make botc use this one for random() and not use the current, broken rand().
TagsNo tags attached.
Relationships
related to 0001657closed  botc 
Attached Files
Issue History
Date ModifiedUsernameFieldChange
2014-02-09 02:06DuskNew Issue
2014-02-09 02:07DuskRelationship addedrelated to 0001657
2014-02-09 02:07DuskDescription Updatedbug_revision_view_page.php?rev_id=4478#r4478
2014-02-09 02:10DuskAdditional Information Updatedbug_revision_view_page.php?rev_id=4480#r4480
2014-02-09 02:11DuskAdditional Information Updatedbug_revision_view_page.php?rev_id=4481#r4481
2014-02-09 15:22Torr SamahoNote Added: 0008179
2014-03-02 22:19DuskNote Added: 0008300
2014-03-02 22:20DuskNote Edited: 0008300bug_revision_view_page.php?bugnote_id=8300#r4523
2014-03-03 19:23Blzut3Note Added: 0008311
2014-03-03 22:39DuskNote Added: 0008313
2014-03-04 03:53DuskNote Edited: 0008313bug_revision_view_page.php?bugnote_id=8313#r4533
2014-03-04 04:00Blzut3Note Added: 0008315
2014-05-18 12:47DuskAssigned To => Dusk
2014-05-18 12:47DuskStatusnew => needs review
2014-06-01 15:15Torr SamahoNote Added: 0008827
2014-06-01 15:15Torr SamahoStatusneeds review => needs testing
2014-10-05 22:20DuskFixed in Version => 1.3
2014-10-05 22:22DuskNote Added: 0010345
2014-10-05 22:22DuskStatusneeds testing => resolved
2014-10-05 22:22DuskResolutionopen => fixed
2018-09-30 21:35Blzut3Statusresolved => closed

Notes
(0008179)
Torr Samaho   
2014-02-09 15:22   
I don't think that we have to keep the old, broken behavior.
(0008300)
Dusk   
2014-03-02 22:19   
(edited on: 2014-03-02 22:20)
I'm doing some last needed rework to botc and I think I could push the beta build soon, like next week perhaps? However, this still stands in the way. If botc is released before 1.3/2.0 is then both functions will have to be kept, which IMO is becoming quite unavoidable.

So here's what I'm proposing:
- on Zandronum's side add the new random() as a separate command
- botc will default to the old rand(), unless told (with the using directive) to use 1.3 functionality, in which case it uses the new random() instead.
- to make the transition go over smoother, whenever the old rand() is used, the second parameter is added 1 to. botc recognizes constant expressions so if consts are used it'll result in no bytecode overhead.
- when 1.3/2.0 is released, botc will start defaulting to the new random(). I guess the old one could be still be used with some __old_broken_rand() command.

Sound any good?
(0008311)
Blzut3   
2014-03-03 19:23   
Given that custom bot scripts were not really possible until now, I would personally go the route of documenting the quirk and just not supporting custom scripts for 1.2. (That is, no backwards compatibility.)
(0008313)
Dusk   
2014-03-03 22:39   
(edited on: 2014-03-04 03:53)
Yeah sure, I'll just go release botc now and say "OH RIGHT ANY SCRIPTS USING RAND() WILL BREAK IN 2.0 BECAUSE BOTSCRIPT IS ACTUALLY NOT SUPPORTED IN 1.2, GET IT?"

Might as well not fix this at all.
(0008315)
Blzut3   
2014-03-04 04:00   
It would still be possible to write a script that works in 1.2 and 2.0, but you'd have to be aware of the quirk. Plus, by leaving 1.x bot script unsupported, it gives an opportunity to iron out any other issues that Carn may have left in.
(0008827)
Torr Samaho   
2014-06-01 15:15   
I transplanted your latest patch for this.
(0010345)
Dusk   
2014-10-05 22:22   
Marking as resolved as 1.3 is now released.