Zandronum Chat on our Discord Server Get the latest version: 3.1
Source Code
My View | View Issues | Change Log | Roadmap | Zandronum Issue Support Ranking | Rules | My Account
  

View Issue Details [ Jump to Notes ] [ Issue History ] [ Print ]
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;
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().
Attached Files

- Relationships
related to 0001657closed botc 

-  Notes
User avatar (0008179)
Torr Samaho (administrator)
2014-02-09 15:22

 I don't think that we have to keep the old, broken behavior.
User avatar (0008300)
Dusk (developer)
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?
User avatar (0008311)
Blzut3 (administrator)
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.)
User avatar (0008313)
Dusk (developer)
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.
User avatar (0008315)
Blzut3 (administrator)
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.
User avatar (0008827)
Torr Samaho (administrator)
2014-06-01 15:15

 I transplanted your latest patch for this.
User avatar (0010345)
Dusk (developer)
2014-10-05 22:22

 Marking as resolved as 1.3 is now released.

Issue Community Support
This issue is already marked as resolved.
If you feel that is not the case, please reopen it and explain why.
Supporters: No one explicitly supports this issue yet.
Opponents: No one explicitly opposes this issue yet.

- Issue History
Date Modified Username Field Change
2014-02-09 02:06 Dusk New Issue
2014-02-09 02:07 Dusk Relationship added related to 0001657
2014-02-09 02:07 Dusk Description Updated View Revisions
2014-02-09 02:10 Dusk Additional Information Updated View Revisions
2014-02-09 02:11 Dusk Additional Information Updated View Revisions
2014-02-09 15:22 Torr Samaho Note Added: 0008179
2014-03-02 22:19 Dusk Note Added: 0008300
2014-03-02 22:20 Dusk Note Edited: 0008300 View Revisions
2014-03-03 19:23 Blzut3 Note Added: 0008311
2014-03-03 22:39 Dusk Note Added: 0008313
2014-03-04 03:53 Dusk Note Edited: 0008313 View Revisions
2014-03-04 04:00 Blzut3 Note Added: 0008315
2014-05-18 12:47 Dusk Assigned To => Dusk
2014-05-18 12:47 Dusk Status new => needs review
2014-06-01 15:15 Torr Samaho Note Added: 0008827
2014-06-01 15:15 Torr Samaho Status needs review => needs testing
2014-10-05 22:20 Dusk Fixed in Version => 1.3
2014-10-05 22:22 Dusk Note Added: 0010345
2014-10-05 22:22 Dusk Status needs testing => resolved
2014-10-05 22:22 Dusk Resolution open => fixed
2018-09-30 21:35 Blzut3 Status resolved => closed




Questions or other issues? Contact Us.

Links

Copyright © 2000 - 2024 MantisBT Team
Powered by Mantis Bugtracker