#3578 closed defect (fixed)
[PATCH] Seed random sounds
Reported by: | Thomas | Owned by: | Thomas |
---|---|---|---|
Priority: | Should Have | Milestone: | Alpha 24 |
Component: | Core engine | Keywords: | |
Cc: | Patch: | Phab:D1584 |
Description (last modified by )
Visual Actors now use the same seed for every sound.
Closes #1786.
Before using the random function it uses srand() with the actor id to seed the random function. I also made changes to rand.cpp. Apparently the first value returned by rand() is very dependent on the seed (see http://stackoverflow.com/questions/8599238/random-number-generation-in-c-first-number-not-very-random). Most actors were returning the same random value as they have similar seeds. Using modulo instead of dividing uses the least significant bits of the random value, yielding better results.
This is my first patch, so please tell me if I'm doing something wrong.
Attachments (4)
Change History (26)
by , 9 years ago
Attachment: | patch.patch added |
---|
comment:1 by , 9 years ago
Milestone: | Alpha 19 → Alpha 20 |
---|---|
Owner: | set to |
We are currently in feature freeze so this will be for next Alpha version. Please attach patches to older tickets instead of creating new ones :)
Thanks for your patch it will hopefully be reviewed soon after release (in a few weeks) Feel free to take any other tickets.
Please edit this ticket and assign it to you next time you comment :) Feel free to work on any other tickets :)
comment:2 by , 9 years ago
Owner: | removed |
---|
comment:3 by , 9 years ago
Owner: | set to |
---|
follow-up: 8 comment:7 by , 8 years ago
Patch looks fine and seems to work (or at least not to fail). I'm not sure what it changes though? Are sounds more varied? Not sure how to test this.
comment:8 by , 8 years ago
Keywords: | review removed |
---|
Replying to wraitii:
Patch looks fine and seems to work (or at least not to fail). I'm not sure what it changes though? Are sounds more varied? Not sure how to test this.
The problem was explained in the other ticket. If you selected a unit, the voice pitch would be different each time. Each unit should have their own constant pitch.
This patch doesn't look like the right solution, it's calling srand()
which reseeds the non-simulation RNG for the entire game? That is surely not desirable :) I think what you want to do is use a local rng_t
or something like we do for actor variations in CObjectBase::CalculateRandomRemainingSelections
.
by , 8 years ago
Attachment: | sound.patch added |
---|
comment:10 by , 8 years ago
Do you mean something like this? Unfortunately this code doesn't compile for me, and so far I couldn't figure out why, but the idea should work.
comment:11 by , 8 years ago
Keywords: | rfc added |
---|
comment:12 by , 8 years ago
Thanks for keeping working on it: http://pastebin.com/zCXm3LmG Here is the build log in case that helps.
comment:13 by , 8 years ago
It's been a while, but I thought I'd start again. I think I'm using Boost incorrectly, since most errors occur there.
comment:14 by , 8 years ago
I'm not sure whether you found it already, but the uni_dist
expects a reference, not a pointer. I would change rng_t* rng
to rng_t& t
. Also, maybe it would be better if we used the C++11 <random> for this? I'm not sure whether this is allowed by our coding conventions though...
by , 8 years ago
Attachment: | DeterministicSound.patch added |
---|
comment:16 by , 7 years ago
Keywords: | review added; rfc removed |
---|---|
Milestone: | Backlog → Alpha 22 |
Patch applies fine and seems to work following historicbruno's comment above.
I'm not entirely sure we need the complexity of Boost for the random number, but it's simple enough that that's nota big issue here. Bumping to review queue, if nobody sees concern with boost, I think we can commit this.
comment:18 by , 7 years ago
We are already using boost in some parts of the code for random number generation. However, as <random>
is supported by all our target compilers, it might be better to use that (which is pretty much equivalent to boost random).
Other than that, the patch looks good to me.
comment:19 by , 6 years ago
Description: | modified (diff) |
---|---|
Patch: | → D1584 |
Uploaded the patch, it was ready for committing back then so it should be pretty easy to check.
comment:20 by , 6 years ago
Keywords: | patch review removed |
---|---|
Patch: | D1584 → Phab:D1584 |
comment:22 by , 5 years ago
Milestone: | Work In Progress → Alpha 24 |
---|
Patch