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

Texel values at upper and bottom poles of InfiniteAreaLight leak to other directions #63

Closed
mmp opened this issue Mar 31, 2016 · 3 comments

Comments

@mmp
Copy link
Owner

mmp commented Mar 31, 2016

If the InfiniteAreaLight constructor is modified to create a low-res environment map with only the top scanline emitting like this:

          resolution.x = resolution.y = 10;
          for (int y = 0; y < resolution.y; ++y) {
            for (int x = 0; x < resolution.x; ++x) {
              if (y == 0)
                texels[y * resolution.x + x] = 1000000.;
              else
                texels[y * resolution.x + x] = 0.;
            }
          }

Then if a small test like this is performed:

    for (Float y = 0; y < 1.f; y += .0249f) {
      for (Float x = 0; x < 1.f; x += .125f) {
        fprintf(stderr, "(%f, %f) = %f\n", x, y,
                Lmap->Lookup(Point2f(x, y)).y());
      }
    }

Then for lots of y values close to 1, the lookup returns a non-zero result:

(0.625000, 0.996000) = 494165.437500

For high-res environment maps, this issue is hidden by the fact that the sinTheta value that the environment map luminance is multiplied by when creating the sampling PDF is very small. But this is still bad.

The root issue is that we want the MIPMap to use REPEAT (as it does now) for out-of-bounds s coordinates, but to do a mirror and flip combination for out of bounds t. Unfortunately, MIPMap only supports one wrap mode. The right fix is probably to rewrite InfiniteAreaLight to not use MIPMap at all, since we don't need most of its functionality anyway.

Also, we should add a unit test for this when it's fixed.

@mmp
Copy link
Owner Author

mmp commented Mar 31, 2016

(Figured this out when thinking about issue #62)

@johguenther
Copy link

To emulate CLAMP_TO_EDGE for v for the texture lookup after sampling in InfiniteAreaLight::Sample_Li and InfiniteAreaLight::Sample_Le you can just clamp v to [0.5/height, 1.0-0.5/height].

@mmp
Copy link
Owner Author

mmp commented Mar 29, 2018

That's a great idea. It turns out to be a little messy in practice, since we use a MIPMap to store the image and it upsamples to a power of 2 resolution. Thus, we need to use the original image height, not the MIP map image height. (Maybe this is what you were suggesting, but it's not the first thing I tried. :-) ). The shift also needs to be by 1/height just so none of the texels used for the bilinear interpolation have a chance to wrap around.

I'm going to go ahead and close this and leave this unfixed; it feels like it's getting a bit messy for a rare issue. We'll fix it properly in pbrt-v4.

@mmp mmp closed this as completed Mar 29, 2018
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

2 participants