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

A Sun and Sky in Mitsuba #1473

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

matttsss
Copy link

@matttsss matttsss commented Jan 25, 2025

Description

This PR introduces a new Sun and Sky emitter based on the Hosek-Wilkie models (sun model, sky model). With the sky's sampling based on Nick Vitsas and Konstantinos Vardis' Truncated Gaussian Mixture Model described in this paper.
The implementation details are resumed in this report, with the issue with the sun's Monte-Carlo weight mentioned in the conclusion solved since.

It makes use of datasets and tests resources that have been provided through:

On plugin instantiation, these datasets are loaded from the files, then are pre-processed (this pre-process step also happens on plugin update). On the contrary to Mitsuba 0.6, this plugin evaluates radiance "on the fly" and does not need an environment map.

The datasets within the binary resource files originate from the reference header files, and were provided as nested C arrays. Those header files are not included but the functions that process them can be found commented at the end of sunsky.h along with more detail about how to re-generate the binary files if need be.

Testing

Tests were provided within test_sunsky.py and check the following:

  • Test 01: Checks the RGB sky radiance against reference renders from Mitsuba 0.6
  • Test 02: Checks the spectral sky radiance against reference renders from Mitsuba 0.6 (however it does not vary albedo since float -> spectrum conversion is different within both versions of Mitsuba)
  • Test 03: A special check for spectral sky radiance with an arbitrary albedo spectrum.
  • Test 04: Checks the sun radiance generated for a range of points. Since the solar model differs from Mitsuba 0.6, whole comparisons of rendered images were not possible.
  • Test 05: Checks that all rays sampled when the sky is off are within the solar cone.

For the next Chi2 tests, the top of the spherical domain was cropped off since the resolution of the test cannot handle the spike in PDF.

  • Test 06: Only checks the sky's TGMM and sampling strategy
  • Test 07: Checks both the sun and sky's sampling strategies simultaneously

Also, the tests are all done with JIT variants. Although I have checked manually the functionality of the scalar variant, since the tests are vectorised, I was unsure how to approach scalar tests.

Checklist

  • My code follows the style guidelines of this project
  • My changes generate no new warnings
  • My code also compiles for cuda_* and llvm_* variants. If you can't test this, please leave below
  • I have commented my code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I cleaned the commit history and removed any "Merge" commits
  • I give permission that the Mitsuba 3 project may redistribute my contributions under the terms of its license

@matttsss matttsss marked this pull request as ready for review January 25, 2025 17:33
@matttsss matttsss marked this pull request as draft January 30, 2025 10:42
@matttsss matttsss marked this pull request as ready for review January 30, 2025 21:35
@wjakob
Copy link
Member

wjakob commented Jan 31, 2025

Dear @matttsss,

this is fantastic! Your implementation looks very well-designed, certainly much better than the one in Mitsuba 1 :).

Here are some comments from me:

Note that the CI fails the tests with the note: resources/data/sunsky/datasets/sky_rgb_params.bin": I/O error while attempting to open file: No such file or directory. Likely you forgot to reference an updated mitsuba-data repository commit.

Some parts of your implementation (sun-related precomputation in the header file) use double precision numbers in overall single precision code. This will cause many double <-> single implicit conversions to be added, which you probably did not want. Just append an f at the end, and that won't happen. There is a mixture of camelCase and underscore_case (which we use in this project) for variable names.

@wjakob wjakob changed the title A Sun and Sky in Mistuba A Sun and Sky in Mitsuba Jan 31, 2025
@wjakob
Copy link
Member

wjakob commented Jan 31, 2025

One related question: if Python is installed via PyPI/pip, those data files will need to become part of the generated package. Does the installation mechanism work, and does the installed plugin still find them? The file data/srgb.coeff (dynamically generated during the build) might be a good guide on how to get those steps working.

@wjakob
Copy link
Member

wjakob commented Jan 31, 2025

I took a quick look at the changes in the data repository. You added a number of rendered reference skies, which is great for validation. But they are really large, ~2MB for each image. Over time, such additions make the Mitsuba (and downstream) repos very costly to clone.

Could you adapt these tests so that they can work with much less data? -- I am sure that something on the order of 10x less (<200 KiB) would still satisfy the requirement of a correctness check.

@matttsss
Copy link
Author

Thank you for your comments,

Note that the CI fails the tests with the note: resources/data/sunsky/datasets/sky_rgb_params.bin": I/O error while attempting to open file: No such file or directory. Likely you forgot to reference an updated mitsuba-data repository commit.

I was unsure how to proceed with adding the data to the sub-repository, but given the rest of your comments, I will try to generate the data as indicated first before continuing.

@matttsss
Copy link
Author

One related question: if Python is installed via PyPI/pip, those data files will need to become part of the generated package. Does the installation mechanism work, and does the installed plugin still find them? The file data/srgb.coeff (dynamically generated during the build) might be a good guide on how to get those steps working.

I did not think to test the installation mechanism... Generating the data like the srgb.coeff could be a good idea but the original sampling dataset (for the TGMM) is within a .csv file that would still have to be provided separately. If I were to generate the data as suggested, where should the files go? (~ 1 .cpp file for main and 2 header files)
Also how should the sampling dataset be integrated to the installation mechanism? How could I test that?

@wjakob
Copy link
Member

wjakob commented Jan 31, 2025

No need to generate the TGMM data from CSV. What I meant to say is that the Mitsuba can still find this data after building a wheel and installing it. It would be great if that's also the case for these data files.

@matttsss
Copy link
Author

I have worked on reducing the test data size. I also updated the CMakeLists.txt to copy the datasets into the build directory. From what I understand, this should make it available for the wheel?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants