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

ofRandom #7598

Merged
merged 55 commits into from Aug 23, 2023
Merged

ofRandom #7598

merged 55 commits into from Aug 23, 2023

Conversation

artificiel
Copy link
Contributor

as discussed in #7579 i've separated the engine from the distribution, and applied the minimum changes to the core to correctly integrate the engine, and fix the shuffle problem.

about deprecation warnings, i'd like to suggest now is the correct time to apply them to shuffle and seed (the implementation is at of::random::shuffle, but also allows ofShuffle for the classic ofPattern). note that nowhere in the OF codebase (including examples) ofRandomize() is being used, so it's not like warnings are going to pop everywhere (and the actual seeding process/naming is known to be confusing). also, the seeding has been parallelized so both the old and new get seeded (or reset) by the same seed calls.

i created a new example, randomExample, that showcases the engine and the shuffling (and can be later expanded once the std:: distributions are "revealed").

to that effect, as of now the branch contains everything, but ofRandomDistributions.h isn't included/used anywhere by default. if you include ofRandomDistributions.h in a project (like the random explorer devapp) it will compile and run. so it's ready to go — the next step is to change the old srand-based stuff to the new std::uniform stuff.

if you'd rather not include the distributions yet, simply drop utils/ofRandomDistributions.h and the random explorer devapp.

NOTE: the armv6 build still fails, and i think it cancels all the concurrent runs? i manually re-ran osx and it passes testing: https://github.com/artificiel/openFrameworks/actions/runs/5875297725

@ofTheo
Copy link
Member

ofTheo commented Aug 16, 2023

Thanks @artificiel - this looks epic!

I added a comment above.

The main reason I wanted to preserve ofRandomize, was it might be more intuitive for someone searching for the function as they might start typing ofRand.... and see it via auto complete.

I'll let others chime in @ofZach @dimitre etc

But overall excited to merge this.
Will see if there is anything we can do for the armv6 checks.

Thanks!

@dimitre
Copy link
Member

dimitre commented Aug 16, 2023

Great!
One point:I've seen your new example uses space in name, so one suggestion is keep medial capitals / camelCase to keep examples uniformization, and to avoid make known issues with spaces in path.

@artificiel
Copy link
Contributor Author

I've seen your new example uses space in name, so one suggestion is keep medial capitals / camelCase to keep examples uniformization, and to avoid make known issues with spaces in path.

well, there the "real" example which is called randomExample, and a more complicated "random explorer" which i've placed in apps/devapps as i don't think that code makes a good "example". there is already a spaces-containing devapp called ofPolyline resample tester... and i'm in defense of spaces in paths (even on linux; no opinion on windows) as they tend to reveal problems to be fixed... (that would arise with unicode or other "special" characters). i also enjoy having my executable show up as "Random Explorer" in the dock, taskbar, etc.

i've implemented/tested on macOS and it works fine but i will not put up a fight — if there is a concensus i will rename it RandomExplorer -- but that devapps directory should be cleaned up as there are all kinds of flavours there (mostly camel, but also Pascal, snake, spaces, and even a kebab!)

@artificiel
Copy link
Contributor Author

@ofTheo wrote

The main reason I wanted to preserve ofRandomize, was it might be more intuitive for someone searching for the function as they might start typing ofRand.... and see it via auto complete.

ah good point, i tested here (macOS 13.4 / Xcode 14.3.1) and the autocomplete provided this:

image

@dimitre
Copy link
Member

dimitre commented Aug 16, 2023

just clarifying I'm totally ok to keep as it is also.

@artificiel
Copy link
Contributor Author

@ofTheo if you plan on including the distributions, for them to be readily available it should be included somewhere. the distributions depend on the engine and as i kept the things header-only, it must be included after ofRandomEngine. i guess the simplest way is to add a line in ofUtils.h, right after the inclusion of ofRandomEngine.h.

or we keep it under the radar until the expurgation of srand/rand?

@ofTheo
Copy link
Member

ofTheo commented Aug 18, 2023 via email

@artificiel
Copy link
Contributor Author

OK, the #include now works for Engine and Distributions (but the distributions themselves are not used anywhere yet, except in the Random Explorer devapp). and deprecation removed on ofRandomize()!

@ofTheo
Copy link
Member

ofTheo commented Aug 19, 2023

Thanks @artificiel - will merge this as soon as I get armv6 working

@ofTheo
Copy link
Member

ofTheo commented Aug 23, 2023

@artificiel can you pull into your branch from master and see if the checks pass now?

@artificiel
Copy link
Contributor Author

image image image image image image image image image image image image image image image image image image image image image image image image image image image image image image image image image image image image image image image image image image image image image image image image image

@artificiel
Copy link
Contributor Author

awesome!

@ofTheo
Copy link
Member

ofTheo commented Aug 23, 2023

woohoo!! 🎉
Thanks @artificiel !!

@ofTheo ofTheo merged commit 16d7da1 into openframeworks:master Aug 23, 2023
8 checks passed
@artificiel artificiel deleted the ofRandom branch August 24, 2023 00:07
@dimitre
Copy link
Member

dimitre commented Aug 26, 2023

suggestion: is it possible to remove the deprecation for the release?
It is just so we don't have a deprecation warning built in the core itself
Screen Shot 2023-08-26 at 11 00 46

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.

None yet

4 participants