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.

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

less deterministic random #36

Merged
merged 1 commit into from
Jun 10, 2019
Merged

Conversation

npisanti
Copy link
Collaborator

this commit initialize a random seed on start and uses it in the random hashing function, without that the number generated by R would be the same each time the .orca file is restarted, try this before the commit

.........................
...R.....................
1gL......................
.........................

save the file, and start it, then close and restart it, you can see that the generated numbers are the same. That is a bad behavior for making generative music, so this commit fixes that.

As this is the first time i go outside the sim.c file, i'd like to @cancel to review this commit when he has time.

@npisanti npisanti requested a review from cancel May 21, 2019 07:12
@cancel
Copy link
Collaborator

cancel commented May 21, 2019

This is the wrong way to do this.

  1. Global variables are very slow.
  2. You declared an external global variable without an orca_ prefix or something like that, so it will clash with any other project that attempts to link or build with orca-c and uses the ranseed symbol name.
  3. orca-c now requires an explicit "initialization" phase to work before it can be used at all. This also adds threading issues because now any program that wants to use the orca-c vm has to have a non-threaded synchronized startup during the init phase before it can call the orca-c vm stepping function. orca-c didn't require this previous and adding it as a requirement to use it makes it hugely more complicated to use from a scripting language, another runtime, etc. This sounds small but it's actually a huge problem and should be avoided at all costs.
  4. unsigned long and long int are non-portable for byte size and should never be used in orca-c. There is a reason I never used them and only used Usz, Isz, I32, etc. This code would not be accepted into any project that has portability in mind.

Instead:

Add another argument to orca_run for the random seed, which should be Usz. Add a random_seed field to the Oper_extra_params struct. Assign the random_seed argument to the field on that struct around this line https://github.com/hundredrabbits/Orca-c/blob/master/sim.c#L783 Then you can access the random_seed value from the R operator definition like extra_params->random_seed

@npisanti
Copy link
Collaborator Author

understood! i thought that adding another argument to orca_run would have been a problem bigger than those, but all those things are actually doable so i'll work on fix it =)

all the 1-2-4 point are clear, just for 3, i make all those things, then i declare and initialize the random seed to pass locally into the tui_main.c instead than doing it globally, right?

@cancel
Copy link
Collaborator

cancel commented May 21, 2019

Yeah. Leave the generation of the random seed up to whatever is calling orca_run. You can just store it as a variable in the Ged struct in tui_main.c

@npisanti
Copy link
Collaborator Author

sounds good, i will do it as soon as i have some mental stamina for this

@npisanti
Copy link
Collaborator Author

i was trying all the suggestion, but as the random seed i'm generating is just created once when i pass it to the orca_run by value is always the same, generating the same output depending just on how many R i use, instead before it was always fed back between frames.

I was thinking about using the generated random seed (that is always the same for each execution in your frame and position dependant hashing, what do you think would be the best way to do that? for example something like this, to avoid multiplying the seed by 0 on the first frame:

  Usz key = y * width + x;
  key = hash32_shift_mult((y * width + x) ^ (((Tick_number+16)*extra_params->random_seed) << UINT32_C(16)));
  Usz val = key % (max - min) + min;

@npisanti
Copy link
Collaborator Author

npisanti commented May 24, 2019

also, using the seed this way open up of keeping the possibility of a --use-seed CLI argument to force a seed for getting deterministic output, good for making demo videos for performances.

@npisanti
Copy link
Collaborator Author

( i pushed those changes so you can give a look )

@npisanti
Copy link
Collaborator Author

npisanti commented May 24, 2019

maybe another way could be :

  Usz key = y * width + x;
  key = hash32_shift_mult((extra_params->random_seed + y * width + x) ^ (Tick_number << UINT32_C(16)));
  Usz val = key % (max - min) + min;

it would be the equivalent of shifting the whole random coordinates using the seed, i like it more than the committed one

@npisanti npisanti merged commit 18b164f into hundredrabbits:master Jun 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants