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

Improve FastRNG implementation #2056

Merged
merged 5 commits into from
Jul 27, 2024
Merged

Improve FastRNG implementation #2056

merged 5 commits into from
Jul 27, 2024

Conversation

DelinWorks
Copy link
Contributor

@DelinWorks DelinWorks commented Jul 26, 2024

Describe your changes

improved FastRNG struct to have better performance, clearer inclusive and exclusiveness, and more uniform distribution of random numbers generated.
also improved particle system and cpp-tests

Checklist before requesting a review

For each PR

  • Add Copyright if it missed:
    - "Copyright (c) 2019-present Axmol Engine contributors (see AUTHORS.md)."

  • I have performed a self-review of my code.

    Optional:

    • I have checked readme and add important infos to this PR.
    • I have added/adapted some tests too.

For core/new feature PR

  • I have checked readme and add important infos to this PR.
  • I have added thorough tests.

@halx99 halx99 linked an issue Jul 27, 2024 that may be closed by this pull request
@halx99 halx99 added this to the 2.1.5 milestone Jul 27, 2024
@halx99 halx99 added the enhancement New feature or request label Jul 27, 2024
@halx99
Copy link
Collaborator

halx99 commented Jul 27, 2024

image
image

int main()
{
#if defined(_DEBUG)
    std::print(std::cout, "test on debug build\n");
#else
    std::print(std::cout, "test on release build\n");
#endif
    constexpr int test_rounds = 10;
    constexpr int test_count = 10000000;
    FastRNG rgn;
    std::minstd_rand stdrgn;

    double total1 = 0;
    double total2 = 0;

    rgn.seed(time(NULL));
    stdrgn.seed(time(NULL));

    for (int k = 0; k < test_rounds; ++k) {

        auto start = axmol::highp_clock();
        for (int i = 0; i < test_count; ++i) {
            total1 += rgn.rangef(0, 1);
        }
        auto diff = axmol::highp_clock() - start;

        std::print(std::cout, "FastRGN cost: {} ms\n", diff / 1000.0);

        start = axmol::highp_clock();
        for (int i = 0; i < test_count; ++i) {
            total2 += stdrgn() / (double)stdrgn.max();
        }
        diff = axmol::highp_clock() - start;

        std::print(std::cout, "minstd_rand cost: {} ms\n", diff / 1000.0);
    }

    std::print(std::cout, "total1={}, total2={}", total1, total2);
    return 0;
}

release build, fast rng is faster than std::minstd_rand, good job, merged

@halx99 halx99 merged commit 4c80b80 into axmolengine:dev Jul 27, 2024
15 checks passed
@halx99
Copy link
Collaborator

halx99 commented Jul 27, 2024

BTW: do we need move class FastRNG into namespace ax?

@DelinWorks
Copy link
Contributor Author

thank @smilediver he's the one that wrote and suggested these awesome optimizations

@DelinWorks
Copy link
Contributor Author

BTW: do we need move class FastRNG into namespace ax?

come to think of it yeah it should be in the ax namespace

@DelinWorks
Copy link
Contributor Author

also some functions need to be static too, I'll do PR

Copy link
Contributor

@smilediver smilediver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've also noticed that this never was in ax namespace. Maybe it should be added?

core/math/FastRNG.h Show resolved Hide resolved
core/math/FastRNG.h Show resolved Hide resolved
core/math/FastRNG.h Show resolved Hide resolved
core/math/FastRNG.h Show resolved Hide resolved
core/math/FastRNG.h Show resolved Hide resolved
core/math/FastRNG.h Show resolved Hide resolved
core/math/FastRNG.h Show resolved Hide resolved
core/math/FastRNG.h Show resolved Hide resolved
core/math/FastRNG.h Show resolved Hide resolved
@halx99
Copy link
Collaborator

halx99 commented Jul 27, 2024

miss include math/MathBase.h

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FastRNG seems to have problematic implementation
3 participants