ID:142263
 
Code:
  verb/Spawn_Monkeys()
set category = "Admin"
var/N = 0
while(N<20)
var/L = list()
for(var/area/A in world)
for(var/turf/T in A)
if(!T.density)
L+=T
new /mob/monkey(pick(L))
N++


Problem description:
It only spawns one monkey.
First, your code should be fixed and has terrible logic. You're looping through all turfs* in the world 20 times when only 1 time is needed, and you're also doing the creations (new() call) before the list of locations is even complete, which will lead to unintended, messy behavior. Fix this and it will likely fix your specific problem as well.

*: Additionally, looping through all areas in the world then looping through all the turfs in them is silly; simply loop through all the turfs in the world, no need for a nested loop.
In response to Kaioken
You don't even really have to loop through the turfs once - picking a turf at random using locate() and random co-ordinates, and then checking the resulting turf (and re-picking if it is dense) should produce the desired effect.
In response to Hazman
This is in fact nice thinking, but in actuality that would only be right if he wanted any turf, not only turfs that fulfill condition X. This is because if he has, for example, a lot of dense wall objects in his game, his code could end up doing unnecessary work, choosing wall objects and re-looping an undefined amount of times, which proves generally less efficient.
In response to Kaioken
Testing shows that it's still more efficient to pick turfs rather than looping and then picking from a list. The code I used is as follows:
client
verb

RunTestSuite(densepercent as num,monkeys as num)
set desc = "Run each test 100 times. First argument is density percentage, second is monkey count"

for(var/a in 1 to 1000)
SeedMap(densepercent)
LoopTest(monkeys)

for(var/a in 1 to 1000)
SeedMap(densepercent)
PickTest(monkeys)

world << "Done"

LoopTest(n as num)
set desc = "Run the loop test, placing n monkeys, once"
_LoopTest(n)
PickTest(n as num)
set desc = "Run the pick test, placing n monkeys, once"
_PickTest(n)
SeedMap(n as num)
set desc = "Set n% of the map as dense"
var/list/turfs = list()
for(var/turf/t in world)
t.density = 0
t.text = "0"
turfs += t
var/totalturfs = world.maxx*world.maxy*world.maxz
var/changeturfs = round(totalturfs * n/100)
while(changeturfs--)
var/turf/changeturf = pick(turfs)
changeturf.density = 1
turfs -= changeturf
changeturf.text = "1"

proc
_LoopTest(n)

. = list()
var/list/turfs = list()
for(var/turf/t in world)
if(!t.density)turfs += t
while(length(.) < n). += pick(turfs)
return .

_PickTest(n)
. = list()
while(n--)
var/turf/t
do
t = locate(rand(1,world.maxx),rand(1,world.maxy),rand(1,world.maxz))
while(t.density)
. += t
return .


Obviously, as more dense turfs appear the pick system becomes less efficient, but running with 50% density and 20 monkeys gives the loop test with 1.236 CPU time and the pick test with 0.045. As high as 80% the pick test is still ahead at 1.032 vs. 0.125.
The fewer times you loop with the loop test, the better, of course, so if you were to keep a list of all non-dense turfs from the start of the game you'd be able pick out from it very quickly, but ensuring that the list is accurate may be a problem in some games.

[EDIT] I've fixed a minor bug in the loop test - the proc was building a turf list but not actually picking turfs from it.
In response to Hazman
Hazman wrote:
Testing shows [...]

Short-time, debug kind of testing this is very, very futile. Trust me when I say this; I used to think like you, but that logic is wrong and fails. Moreover, 'efficiency' is not 'speed'; your suggested approach is definitely less efficient, because it varies greatly, and can potentially (randomly) give better results, some of the time (randomly), but its variability also renders it unreliable as it can, some of the time, cause unexpected behavior, like being potentially much slower. Therefore, the other method is preferred, because it provides more invariable, and consistent results. Similarly, "always quite fast" is more efficient than "sometimes very fast, sometimes very slow".
In response to Kaioken
That test ran 1000 times for each proc. The playfield was re-seeded with dense turfs for every test. The playfield was 50x50x1 in size, relatively small by all accounts, and a loop test will get proportionately slower as the playfield gets larger, whereas a pick test will only get slower as the playfield gets more dense.

Re-running each test 5000 times on the same playfield size, with 50% density and 20 monkeys, gives the loop test 7.17 ticks of CPU time with the pick test using just 0.374. The pick test is only going to do worse than the loop test in extremely rare cases or extremely high densities, where it has to test density more times than there are turfs on the map - increasing it's effectiveness versus a loop system even more on a larger map.
In response to Hazman
Since you continue to pretty much simply disregard my posts and my points, there is not much I can do, however I will attempt to get my point across one more time.

Hazman wrote:
and a loop test will get proportionately slower as the playfield gets larger, whereas a pick test will only get slower as the playfield gets more dense.

You are trying to prove a method out of 2 methods (with completely different results) better judging by its (incidentally, potential-)execution speed alone. This is wrong, as well as testing this is quite unreliable, since the code's function varies greatly randomly. Code that variably, randomly produces bad behavior is bad and inefficient code. You do not want your code to do random acts (such as loop again and again randomly), you want it to only do what its intended function is.

Re-running each test 5000 times on the same playfield size, with 50% density and 20 monkeys, gives the loop test 7.17 ticks of CPU time with the pick test using just 0.374.

Again, your implementation is not consistent and all and will vary every execution, therefore this kind of testing it (especially in an unrealistic test environment like that) isn't completely accurate.

Lastly, I shall indicate my main point, why your approach is bad: Your code's behavior will change randomly, and it can potentially loop much, much more times than it needs to <small>(depending on random factors which is not something you can properly test in practice so easily as you attempt)</small>, whereas my more correct approach will always loop the exact amount of times it is required to - no more, no less, and is therefore more efficient in that it will always produce appropriate, consistent, robust behavior, unlike your own.
More than that, I can't say much to change your mind, so I won't go into deeper argument of this; it is unneeded. The OP and other readers are free to choose whomever's method strike them best, for the good and for the worse.
In response to Kaioken
I'm not disregarding your posts, but the fact is that the my method is going to be slower than yours once every n executions, where n is the number of turfs being tested. So on my 50x50 test map, it is slower once every 2500 tests. On a more realistically sized (but still fairly small for an RPG) 200x200, we're talking about a 1 in 40,000 chance of equal performance. A 99.9975% chance of outperforming your method. That means that if the proc was run once every minute solidly for a month it would statistically be slower only approximately once.

I'd take a 39,999 in 40,000 chance of being faster than your method against being that slow every time any day.

Which random factors, exactly, can't I reproduce? The brief is that a set of turfs with a certain property need to be picked, which is exactly what is being tested. And how is the test environment unrealistic? I could use any possible map and still have an accurate simulation simply because the turf selection is always random. The test map makes no difference to the overall results except in size and density percentage - both of which are accounted for in the simulation.

The fastest solution would use your method, but would cache the turf list so as not to have to reproduce it every time.
In response to Hazman
Hazman wrote:
I'm not disregarding your posts,

This is wrong, because you're disregarding my point that code's speed is not more important than its general efficiency.

Which random factors, exactly, can't I reproduce?

The fact that your code chooses turfs completely randomly, and can potentially keep choosing dense turfs, or even the same turfs, over and over, which you are not accounting for in your rather poor equation to represent the matter.

The fastest solution would use your method, but would cache the turf list so as not to have to reproduce it every time.

However, this is bringing yet more, different factors into the matter of whether it is a better method: memory and object use (whether the list is worthwhile enough to be kept depending on map size and how often it is used) and the necessary need to update the list whenever turfs are changed (added or deleted). If you wish, I wouldn't mind discussing those with you at a different time, and in a more appropriate topic/elsewhere.
In response to Kaioken
    verb/Spawn_Monkeys()
set category = "Admin"
var/N = 0
var/L = list()
for(var/turf/T in world)
if(!T.density)
L+=T
while(N<20)
new /mob/monkey(pick(L))
N++

This didnt work >.>
Just to let you all know...I have this for the monkey...
/mob
monkey
New()
world << "Monkey was created."
src.MR() // Monkey Brain
In response to Lcooper
This should normally work, but I think I identify the problem. Next time, also show the MR() proc, because it is relevant: my guess is that it's a proc that handles AI, more importantly, a proc that loops infinitely, correct? This means that proc never returns (finishes), and so New() doesn't return either. This means your Spawn_Monkeys() verb doesn't ever continue, because after calling new(), it waits for the New() proc to finish, but it never does because it waits for MR() to finish, but it never does because it has an infinite loop.

To solve this, you'd use the spawn() instruction (please look it up). For example, in New(), use it to 'spawn-off' the call to MR(), so the New() procedure has a chance to finish/return (and the MR() is called after that). You don't need to put any extra delay in spawn()'s argument for this purpose, so you can leave the parentheses empty.