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

[Book2] Incorrect marble pattern at the final Perlin noise section #1286

Closed
MaximusPrimeForever opened this issue Oct 2, 2023 · 6 comments
Closed
Assignees

Comments

@MaximusPrimeForever
Copy link

Hi, I'm implementing this code in Rust instead of CPP so I very much hope that this isn't due to language differences.

The issue

Anyways, in the Perlin Noise chapter at the marble pattern section the book proposes this modification to get the marble texture:

color value(double u, double v, const point3& p) const override {
    auto s = scale * p;
    return color(1,1,1) * 0.5 * (1 + sin(s.z() + 10*noise.turb(s)));  // << this line
}

Here is my equivalent Rust code:

fn value(&self, _: f64, _: f64, point: &Point3) -> Color {
    let scaled_point = self.scale * (*point);
    Color::new(1.0, 1.0, 1.0) * 0.5 * (1.0 + (scaled_point.z() + 10.0 * self.noise.turbulence(scaled_point, 7)).sin())
}

And this is the image produced:
marble_sphere_bugged

This is doesn't really look like marble, and does not resemble the following example from the book:
from-the-book

Potential solution

I've played around with the code, and noticed that if I pass the raw point (unscaled) to the turbulence function like so:

fn value(&self, _: f64, _: f64, point: &Point3) -> Color {
    let scaled_point = self.scale * (*point);
    Color::new(1.0, 1.0, 1.0) * 0.5 * (1.0 + (scaled_point.z() + 10.0 * self.noise.turbulence(*point, 7)).sin()) // << *point instead of scaled_point
}

I get the following image:
marble_sphere_correct

Which appears much closer to the intended result.
I cannot say I understand the math behind this chapter very well, so I don't know if this is the correct solution, but wanted to share anyway.

Hope this is helpful!

@MaximusPrimeForever MaximusPrimeForever changed the title Incorrect marble pattern at the final Perlin noise section [Book2] Incorrect marble pattern at the final Perlin noise section Oct 2, 2023
@hollasch hollasch added this to the v4.0.0-alpha.2 milestone Oct 2, 2023
@hollasch hollasch self-assigned this Nov 14, 2023
@gau-nernst
Copy link

It seems like your solution matches the code in v3: https://raytracing.github.io/v3/books/RayTracingTheNextWeek.html#perlinnoise/adjustingthephase

Scaling is only applied to the phase p.z()

@MaximusPrimeForever
Copy link
Author

It seems like your solution matches the code in v3: https://raytracing.github.io/v3/books/RayTracingTheNextWeek.html#perlinnoise/adjustingthephase

Scaling is only applied to the phase p.z()

Looks like it's fixed for future releases then :)

@hollasch
Copy link
Collaborator

No, @gau-nernst is saying that the old version matches your solution, and the new version in development has the defect you point out.

At this point, your comment and solution look correct to me.

In order to properly test this, I'll need to be in the middle of the book 2 progression work that we have scheduled when the book 2 alpha is getting ready for release. I'll revisit then and likely apply your fix.

@hollasch
Copy link
Collaborator

Wait, no — I was looking at perlin.h when I should have been looking at texture.h. Testing now.

@hollasch
Copy link
Collaborator

Yup, looks good. Nice catch; thank you!

hollasch added a commit that referenced this issue Feb 20, 2024
At some point we passed the scaled point to the noise.turb() function,
when we should have continued to pass the unscaled point.

Resolves #1286
hollasch added a commit that referenced this issue Feb 20, 2024
At some point we passed the scaled point to the noise.turb() function,
when we should have continued to pass the unscaled point.

Resolves #1286
hollasch added a commit that referenced this issue Feb 21, 2024
At some point we passed the scaled point to the noise.turb() function,
when we should have continued to pass the unscaled point.

Resolves #1286
@hollasch
Copy link
Collaborator

Fixed.

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