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

static random distribution does not update the intervals #1681

Open
hollasch opened this issue Feb 19, 2025 · 9 comments
Open

static random distribution does not update the intervals #1681

hollasch opened this issue Feb 19, 2025 · 9 comments
Assignees
Milestone

Comments

@hollasch
Copy link
Collaborator

Discussed in #1680

Originally posted by niccolot February 18, 2025
I' ve just realized that one of the suggested way to generate random numbers, namely

inline double random_double(double min = 0, double max = 1) {
    /**
     * @brief returns a double in the range [min, max)
     */
    static std::uniform_real_distribution<double> distribution(min, max); // should not be static
    static std::mt19937 generator;

    return distribution(generator);
}

produces numbers only in the first (min,max) range it is instanciated with, e.g.

int main()
{
    for (int i=0; i<5; ++i) {
        std::cout << random_double() << std::endl;
    }
    std::cout << "\n";
    for (int i=0; i<5; ++i) {
        std::cout << random_double(1,2) << std::endl;
    }
}

prints

0.135477
0.835009
0.968868
0.221034
0.308167

0.547221
0.188382
0.992881
0.996461
0.967695

I think is kind of a coincidence that everything works regardless, the random_double() is always used with (0,1), the only problem could arise in

Vec3 HittableList::random(const Vec3& origin) const {
    auto int_size = int(objects.size());
    auto vec = objects[random_int(0, int_size-1)]->random(origin);

// rest

but i think it would not produce any particularly visible artifact.

@hollasch hollasch added this to the v4.0.2 milestone Feb 19, 2025
@hollasch hollasch self-assigned this Feb 19, 2025
@niccolot
Copy link

also i think that having static objects in inline functions means that one has a static object per function call

@hollasch
Copy link
Collaborator Author

also i think that having static objects in inline functions means that one has a static object per function call

Oif. That sounds right to me.

@hollasch
Copy link
Collaborator Author

Ah, it seems the situation is not as bad as you thought. You're correct that we should remove the inline keyword for the function definition. I'll make that change. However, your implementation above is not what the books suggested.

There are two functions involved:

  1. double random_double()
  2. double random_double(double min, double max)

The suggested use of std::uniform_real_distribution<double>() in the text has this new function replacing the first function above, but in your example you've replaced the second. Instead, the second function above should remain unchanged, as this one just takes a standard $[0,1)$ distribution and lerps it to the desired [min, max) range. I'll expand the code listing to include both functions for clarity.

@dimitry-ishenko
Copy link
Contributor

also i think that having static objects in inline functions means that one has a static object per function call

It doesn't. From inline specifier:

In an inline function,

  • Function-local static objects in all function definitions are shared across all translation units (they all refer to the same object defined in one translation unit).

@hollasch
Copy link
Collaborator Author

So @dimitry-ishenko, what's your thought on my removing the inline specifier for both functions? My suspicion is that it doesn't really matter either way (particularly given your comment above), but curious if you have any input.

@niccolot
Copy link

maybe the best thing to do would be to time inline funcs vs non inline and in case nothing changes just remove the inline to remove unnecessy keywork or just leave as it is

@hollasch
Copy link
Collaborator Author

The problem with that approach is that this is helpful for all people who have my exact setup, as of February 2024. 😄

@dimitry-ishenko
Copy link
Contributor

dimitry-ishenko commented Feb 20, 2025

So @dimitry-ishenko, what's your thought on my removing the inline specifier for both functions? My suspicion is that it doesn't really matter either way (particularly given your comment above), but curious if you have any input.

Since it's defined in the header file, it should be marked inline to avoid ODR violation if the header is used in more than one .cpp file.

maybe the best thing to do would be to time inline funcs vs non inline and in case nothing changes just remove the inline to remove unnecessy keywork or just leave as it is

There is no need. inline is just a hint to the compiler. The compiler will chose whether to inline the function or not, based on best performance. But inline keeps the compiler from complaining if it encounters more than one definition of the same function.

@dimitry-ishenko
Copy link
Contributor

Here is a super simplified example: https://godbolt.org/z/nd7sE5qc6

The rtweekend.h file is included in foo.cpp and bar.cpp. If you look closely, the compiler complains about non_inline_fn and doesn't complain about inline_fn even though both are defined in the same header file.

hollasch added a commit that referenced this issue Feb 25, 2025
hollasch added a commit that referenced this issue Feb 25, 2025
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

No branches or pull requests

3 participants