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

Implementation of the pathreparam integrator with OptiX 7 #157

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Speierers
Copy link
Member

Continuation of PR #44.

@abhinavvs
Copy link

Does this mean that "pathreparam" plug-in still is not fully released yet (not even on OptiX 6)?

Any rough idea on when this plug-in will be available (and fully integrated with Mitsuba2)? I am really looking forward to this plug-in as I need this feature for my research project.

Thanks!

@Speierers
Copy link
Member Author

The previous implementation based on "Optix 6" wasn't release, although it is publicly available here:
https://github.com/loubetg/mitsuba2

@realWDC
Copy link

realWDC commented Jun 21, 2020

Great works!

Quick question:
How to auto-diff w.r.t. shape parameters, for example m_radius in src/shapes/sphere.cpp?

I have seen following comments in this PR; Any hints on how to proceed?

 // TODO: make si differentiable w.r.t. shape parameters if necessary

Thanks!

@Speierers
Copy link
Member Author

Great works!

Quick question:
How to auto-diff w.r.t. shape parameters, for example m_radius in src/shapes/sphere.cpp?

I have seen following comments in this PR; Any hints on how to proceed?

 // TODO: make si differentiable w.r.t. shape parameters if necessary

Thanks!

This is not supported at the moment. For this to work, Shape::m_to_world would need to become a differentiable type (e.g. Transform<Point<CUDAArray<float>, 4>> which involves a lot of changes accross the different shape plugins.
This will definitively be part of a future PR, but I won't be able to give any ETA at the moment.

si_out[active] = si;
std::pair<Point3f, Normal3f> differentiable_position(const SurfaceInteraction3f &si,
Mask /*active*/) const override {
auto [c, s] = sincos(si.uv.y() * math::TwoPi<Float>);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be [s, c] instead of [c, s]?

auto [c, s] = sincos(si.uv.y() * math::TwoPi<Float>); // wrong?
auto [s, c] = sincos(si.uv.y() * math::TwoPi<Float>); // correct?

MTS_MASK_ARGUMENT(active);

// Check whether the SurfaceInteraction need to be differentiable w.t.r. m_vertex_positions_buf
bool differentiable_pos = is_diff_array_v<Float> && requires_gradient(m_vertex_positions_buf);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To enable scalar_* variants compilation, how about changing from

bool differentiable_pos = is_diff_array_v<Float> && requires_gradient(m_vertex_positions_buf);

to

bool differentiable_pos = false;
if constexpr (is_diff_array_v<Float>) {
    differentiable_pos = requires_gradient(m_vertex_positions_buf);
}

Copy link
Member

@wjakob wjakob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a a big PR, which makes this very difficult to review. Can we please split it up into smaller parts with the key parts?

Specifically:

  • differentiable intersections
  • other things needed by this plugin (smootharea, etc.)
  • the plugin itself

Since these depend on each other, we could do them one after the other instead of having 3 parallel PRs that would be difficult to keep in sync.

For the differentiable intersections, it will be essential to have testcases to make sure that the differentiation is working as expected. (Imagine how difficult it would be to investigate a poorly converging gradient descent, only to find that derivatives are not correctly propagated between mesh positions and derived shading normals.)

For the first part, it would also be good to discuss some of the design decisions that were made. For instance, how fill_intersection gets an intersection record as input and fills it with further information? I understand why this was done due to difficulties in modifying parameters via references in the GPU backends. But there are also downsides to this approach: the masking that this is now needed in all implementations of this method (a source of errors when a mask is forgotten somewhere?), along with the general costs of passing around this data structure (it’s the largest one we have in Mitsuba2). What I would suggest is that we develop an interface where the input SurfaceInteraction3f is not needed.


First, we define two helper functions that we will use to transform the mesh
parameter buffers (flatten arrays) into ``VectorXf`` type (and the other way around).
Note that those functions will be natively supported by ``enoki`` in a futur release.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

@@ -554,9 +554,9 @@ MTS_INLINE Value square_to_beckmann_pdf(const Vector<Value, 3> &m,
// =======================================================================

/// Warp a uniformly distributed square sample to a von Mises Fisher distribution
template <typename Value>
template <typename Value, typename KappaValue = scalar_t<Value>>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important: when you declare a template argument that is used as a function parameter, it is meaningless to specify a default type because it will always be overwritten by what the function is called with. In this change and the next few, can we just change KappaValue to Value directly?

@@ -244,6 +244,37 @@ struct SurfaceInteraction : Interaction<Float_, Spectrum_> {
fmsub(a00, b1y, a01 * b0y) * inv_det);
}

/// Fill with detailed information describing the intersection
void fill_surface_interaction(const Ray3f &ray,
const void *cache, // TODO should be Float* but can't because of ENOKI_STRUCT_SUPPORT (pointer to reference issue)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue mentioned here doesn't ring a bell, can we discuss it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can of course ignore if it is this something that won't play any role in the upcoming Enoki that doesn't allow Array<Float&, ... anymore.

@@ -244,6 +244,37 @@ struct SurfaceInteraction : Interaction<Float_, Spectrum_> {
fmsub(a00, b1y, a01 * b0y) * inv_det);
}

/// Fill with detailed information describing the intersection
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not immediately clear to me why this interface is needed, so some more documentation would be good.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to just be copying a SurfaceInteraction, why not do an assignment at the caller?

* \brief Specifies the surface interaction computation mode when tracing rays
*/
enum class HitComputeMode : uint32_t {
/// Let the tracer engine (Embree, Optix) compute the surface interaction
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tracer engine -> ray tracing engine

Default = 0,

/// Compute a differential surface interaction if shape parameters require gradients. This
/// mode will disable the computation by the tracer engine.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tracer engine -> ray tracing engine. Is this comment accurate? IIRC we are still doing most of the ray tracing work with OptiX/Embree, it's just the final intersection record that is computed differently.

Differentiable = 1,

/// Only compute si.t, si.p, si.uv (barycentric), si.shape, and si.prim_index
Least = 2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this also differentiable? Perhaps Minimal instead of Least (which would be an appropriate naming scheme if there was also Most or something like that.)?

at the borders of the area light, in uv space. (Default: 0.1)

This plugin implements an area light with a smooth transition from full emission
to zero (black) at its borders. This type of light is usefull for differentiable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usefull->useful


Float smooth_profile(Float x) const {
Float res(0);
res = select(x >= m_blur_size && x <= Float(1) - m_blur_size, Float(1), res);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be expressed with 1-2 max()/min calls?

@priyamvad
Copy link

Hello, any movement on this PR? I'd love to test out the ability to derive a displacement map to match the reference render, something it sounds like this feature should help with?

@Speierers
Copy link
Member Author

Hello, any movement on this PR? I'd love to test out the ability to derive a displacement map to match the reference render, something it sounds like this feature should help with?

Hi @priyamvad,
We are currently busy working on a major refactoring of the Mitsuba 2 + Enoki codebase.

You should be able to use this branch as it is for you experiments. Have a look at the heightfield optimization tutorial to get started.

@RiverIntheSky
Copy link

Great works!
Quick question:
How to auto-diff w.r.t. shape parameters, for example m_radius in src/shapes/sphere.cpp?
I have seen following comments in this PR; Any hints on how to proceed?

 // TODO: make si differentiable w.r.t. shape parameters if necessary

Thanks!

This is not supported at the moment. For this to work, Shape::m_to_world would need to become a differentiable type (e.g. Transform<Point<CUDAArray<float>, 4>> which involves a lot of changes accross the different shape plugins.
This will definitively be part of a future PR, but I won't be able to give any ETA at the moment.

Excuse me, is there any update on differentiable to_world? Is this pathreparam branch necessary for this goal compared to master branch? If I'm going to proceed in this direction myself, where should I make changes? Currently I can't pass Transform4f to optix::Transform4f

@Speierers
Copy link
Member Author

Great works!
Quick question:
How to auto-diff w.r.t. shape parameters, for example m_radius in src/shapes/sphere.cpp?
I have seen following comments in this PR; Any hints on how to proceed?

 // TODO: make si differentiable w.r.t. shape parameters if necessary

Thanks!

This is not supported at the moment. For this to work, Shape::m_to_world would need to become a differentiable type (e.g. Transform<Point<CUDAArray<float>, 4>> which involves a lot of changes accross the different shape plugins.
This will definitively be part of a future PR, but I won't be able to give any ETA at the moment.

Excuse me, is there any update on differentiable to_world? Is this pathreparam branch necessary for this goal compared to master branch? If I'm going to proceed in this direction myself, where should I make changes? Currently I can't pass Transform4f to optix::Transform4f

We are busy working on a major refactoring and haven't made any progress on this unfortunately.

Differentiable to_world will for sure introduce discontinuities in the rendering equation, therefore using pathreparam will be necessary to account for those.

You will need to make changes to the shape class and the shape plugins so that there traverse() and parameters_changed() method expose and handle changes in m_to_world.

To pass the transform to optix, you will need to evaluate it and reduce it to a scalar value (e.g. using enoki::hsum which won't do much as every entry of that matrix is a single value).

Good luck!

@RiverIntheSky
Copy link

Great works!
Quick question:
How to auto-diff w.r.t. shape parameters, for example m_radius in src/shapes/sphere.cpp?
I have seen following comments in this PR; Any hints on how to proceed?

 // TODO: make si differentiable w.r.t. shape parameters if necessary

Thanks!

This is not supported at the moment. For this to work, Shape::m_to_world would need to become a differentiable type (e.g. Transform<Point<CUDAArray<float>, 4>> which involves a lot of changes accross the different shape plugins.
This will definitively be part of a future PR, but I won't be able to give any ETA at the moment.

Excuse me, is there any update on differentiable to_world? Is this pathreparam branch necessary for this goal compared to master branch? If I'm going to proceed in this direction myself, where should I make changes? Currently I can't pass Transform4f to optix::Transform4f

We are busy working on a major refactoring and haven't made any progress on this unfortunately.

Differentiable to_world will for sure introduce discontinuities in the rendering equation, therefore using pathreparam will be necessary to account for those.

You will need to make changes to the shape class and the shape plugins so that there traverse() and parameters_changed() method expose and handle changes in m_to_world.

To pass the transform to optix, you will need to evaluate it and reduce it to a scalar value (e.g. using enoki::hsum which won't do much as every entry of that matrix is a single value).

Good luck!

I am not sure if I understand enoki::hsum correctly. As I read in Enoki Reference, value_t<Array> hsum(Array value), so hsum(Float m_radius) should return value_t<Float>, which I tested is float (my compiler says 'value_t<enoki::DiffArray<CUDAArray<float>>>' (aka 'float'). But as I tried to assign it to float it failed:

error: no viable conversion from 'enoki::DiffArray<CUDAArray<float>>' to 'float'
        float radius = hsum(m_radius);
              ^        ~~~~~~~~~~~~~~

Why is this? It looks like hsum(Float m_radius) still returns Float.

@Speierers
Copy link
Member Author

Hi @RiverIntheSky ,

Could you please open a seperate issue for this? Could you try something like hsum(detach(m_radius)) maybe?

@lo-zed
Copy link

lo-zed commented Feb 3, 2021

Dear developers,
First of all thank you for your great tool and all the work done.
If I understand correctly differentiation of weights for blended BSDFs as well as mesh positions depend on this PR? Or was it already included as part of another refactoring mentioned above?
Do you have an idea if work on this PR will start again in the near future?
Thank you very much for your time.

@Speierers
Copy link
Member Author

Hi @Idiot-z ,

This PR will stay as it is for now. The refactored codebase will be very different, requiring a complete re-write of this plugin to fully leverage the various new features of the system. I can't say for now whether it will included or not "natively" in the upcoming release.

@lo-zed
Copy link

lo-zed commented Feb 3, 2021

Hi @Speierers
Thank you for your prompt response. Just to be sure, is differentiation of weights for blended bsdf supposed to be functional in the current release (fe2a1bd)? Because there is a star next to the corresponding parameter, however I get very strange results.

@Speierers
Copy link
Member Author

Hi @Idiot-z ,
Please open a seperate issue for this as it is completely unrelated to this PR ;)

@mehrab2603
Copy link
Contributor

Is there any plan of merging this with master? I need some features from master (projector emitter) that were implemented after this. Tried merging myself but got so many conflicts that I don't know how to fix.

@Speierers
Copy link
Member Author

@mehrab2603 No we are not going to merge this with master as we are about to release our refactored/redesigned version of the codebase which will render this PR completely out of sync.

In the future, we are planning on implementing integrators that can deal with discontinuities. But this won't be part of the upcoming release unfortunately.

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

Successfully merging this pull request may close these issues.

9 participants