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

Flexible spectral sampling for sensors #142

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

leroyvn
Copy link
Contributor

@leroyvn leroyvn commented May 19, 2020

This PR adds flexible spectral sampling for sensors as discussed in #127. Changes are as follows. I only added it to the perspective sensor plugin so far, I'll add this to the other sensor plugins depending on feedback.

Perspective camera plugin

I added a new member m_srf which, if set, defines a spectrum from which sensor wavelengths will be sampled. As requested by @wjakob, I tried to minimize the performance overhead in default cases. This means that:

  • if unspecified, m_srf will be set to nullptr;
  • if m_srf is set when using a non-spectral variant, it will be ignored, set to nullptr and a warning will be issued;
  • if m_srf is set, it will be used to sample wavelengths when calling sample_ray or sample_ray_differential;
  • otherwise, the default RGB sampling method will be used without a virtual function call.

Tests have been updated accordingly.

Uniform spectrum plugin

I added the possiblity to specify a spectral interval for which the uniform value is applied; otherwise, the plugin returns 0. This led to adding two plugin parameters lambda_min and lambda_max, as well as the corresponding data members. All parameters are optional, including value, which now defaults to 1 (not sure if it's desirable behaviour, I just thought it would be convenient sometimes). I'm however unsure about the performance cost brought by the multiple parameter checks I introduced: is it okay?

Example renders

Default Cornell box:

cbox_00

Constant SSR (value=1) in [400, 500 nm] using modified uniform spectrum plugin:

cbox_01

Sentinel-2A/MSI Band 03 SSR using a loaded irregular plugin:

cbox

Todo

  • Add support for all sensor plugins
  • Update spectrum documentation
  • Update sensor documentation
  • Add render tests?
  • Add extended spectral range tutorial?

@leroyvn leroyvn force-pushed the flexible_spectral_sampling branch from cb346ee to 81fb6f0 Compare May 25, 2020 16:09
@leroyvn
Copy link
Contributor Author

leroyvn commented May 25, 2020

Hi @Speierers, I pushed an update to the uniform plugin tests. I tried refactoring those for perspective, but I get (for some tests, not all of them), errors like:

RuntimeError: ​[xml_v.cpp:259] Unkown value type: <class 'mitsuba.core.Transform4f'>

This is the refactored sensor creation function:

def create_camera(o, d, fov=34, fov_axis="x", s_open=1.5, s_close=5, dict_srf=None):
    from mitsuba.core.xml import load_dict
    from mitsuba.core import Transform4f, Vector3f

    dict_camera = {
        "type": "perspective",
        "near_clip": 1.0,
        "far_clip": 35.0,
        "focus_distance": 15.0,
        "to_world": Transform4f.look_at(
            origin=Vector3f(o),
            target=Vector3f(o) + Vector3f(d),
            up=[0, 1, 0]
        ),
        "fov": fov,
        "fov_axis": fov_axis,
        "shutter_open": s_open,
        "shutter_close": s_close,
        "film": {
            "type": "hdrfilm",
            "width": 512,
            "height": 256,
        }
    }

    if dict_srf:
        dict_camera["srf"] = dict_srf

    return load_dict(dict_camera)

Any idea of what could be wrong?

@Speierers
Copy link
Member

Speierers commented May 26, 2020

Any idea of what could be wrong?

Maybe you should try using ScalarTransform4f and ScalarVectort3f?

@leroyvn
Copy link
Contributor Author

leroyvn commented May 26, 2020

Still no luck using ScalarTransform4f. I however noticed that this error occurs with several variants, I'll create an issue for that.

Edit: I got to the bottom of it, I had missed your point about ScalarVector3f. It works! I'm pushing a fix.

@leroyvn leroyvn force-pushed the flexible_spectral_sampling branch 2 times, most recently from 15a7271 to 4e6d703 Compare May 29, 2020 15:40
@wjakob wjakob mentioned this pull request Jun 3, 2020
@leroyvn leroyvn force-pushed the flexible_spectral_sampling branch from 4e6d703 to 223f853 Compare June 11, 2020 12:31
@leroyvn leroyvn marked this pull request as ready for review June 12, 2020 08:51
@leroyvn leroyvn marked this pull request as draft June 12, 2020 09:15
@leroyvn
Copy link
Contributor Author

leroyvn commented Jun 12, 2020

Hi @Speierers, I'm reviving this PR a little: would you mind telling me if this implementation suits you? I could then proceed with propagating this to the other sensor plugins.

I was actually wondering if it wouldn't be better (for maintenance) to have this implemented as part of Sensor, but given what @wjakob said about minimising virtual function calls, I'm afraid this would impact performance. Are my doubts justified?

@leroyvn leroyvn force-pushed the flexible_spectral_sampling branch 2 times, most recently from 878b46d to 65fe4a8 Compare July 15, 2020 10:47
@leroyvn
Copy link
Contributor Author

leroyvn commented Jul 15, 2020

Hi @wjakob, I just finished propagating the sensor response function implementation to all the currently available sensor plugins (including relevant docs updates). The currently open questions are:

  • is this code suitable for merge?
  • render tests: should I add a couple of render tests based on the examples I showed in the PR text?
  • extended spectral range tutorial: should I add a docs page (I guess in the "Advanced topics" section) explaining how to modify the spectral range?

@leroyvn leroyvn marked this pull request as ready for review July 15, 2020 13:24
@leroyvn leroyvn changed the title Flexible spectral sampling ✨ [New Feature] Flexible spectral sampling for sensors Jul 15, 2020
@Speierers Speierers changed the title ✨ [New Feature] Flexible spectral sampling for sensors Flexible spectral sampling for sensors Jul 15, 2020
@Speierers Speierers added the ✨ new feature New feature or request label Jul 15, 2020
@leroyvn leroyvn force-pushed the flexible_spectral_sampling branch 3 times, most recently from a0b43ff to 17bbfcd Compare July 22, 2020 15:20
Ported test_irradiancemeter.py

Ported test_radiancemeter.py

Ported test_thinlens.py

Partially revert "Updated test_perspective.py"

This reverts formatting changes in commit caf2198.

Autopep8 and isort pass

Updated perspective plugin and tests with SRF support

Added flexible spectral range and default value to uniform spectrum plugin

Fixes after rebase and ported tests to dict API

Fixed perspective camera

Added SRF to radiancemeter

Added SRF to irradiancemeter plugin

Updated plugin docs

Docs update and minor fixes to uniform spectrum plugin

Minor fix to uniform spectrum
@Speierers
Copy link
Member

Hi @leroyvn and @wjakob ,

Should we revive this PR and continue working on this in prospect to merging this into next?

@leroyvn
Copy link
Contributor Author

leroyvn commented Apr 28, 2021

Hi @Speierers, I think this is still relevant to atmospheric radiative transfer use cases (although we're still not using it). I however wonder if changes could not be made more concise with some Sensor (or maybe Endpoint?) interface updates.

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

Successfully merging this pull request may close these issues.

2 participants