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

Add eval(), sample_direction() and pdf_direction() to distant sensor #12

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

Conversation

leroyvn
Copy link
Contributor

@leroyvn leroyvn commented Jun 16, 2021

Description

This PR implements the missing eval(), sample_direction() and pdf_direction() methods for the distant sensor plugin.

Notes:

  • Not ready for merge, requires decision regarding SurfaceInteraction3f. Depending on the outcome, I might close this PR.
  • The checkerboard test segfaults when rebasing onto master, I still have to investigate that. EDIT: This appeared with 1372c60.
  • Now requires [Feature] DirectionSample upgrade #19

Testing

I added a few unit tests to check if the implementation was correct.

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 2 project may redistribute my contributions under the terms of its license

Tested with the LLVM backend only (no CUDA).

src/sensors/distant.cpp Outdated Show resolved Hide resolved
@leroyvn
Copy link
Contributor Author

leroyvn commented Jul 6, 2021

Hi @Speierers, any news regarding that? I had a look at @merlinND's ptracer branch and it doesn't look like SurfaceInteraction3f is updated over there. I'm actually in the process of implementing these methods for the non-camera sensors (which I could add to this PR btw).

src/sensors/distant.cpp Outdated Show resolved Hide resolved
Comment on lines +266 to +268
// ds.emitter = this;
// TODO: --^ must be fixed as soon as SurfaceInteraction3f is fit for
// sensor sampling
Copy link
Member

Choose a reason for hiding this comment

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

What's the issue here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, it's not SurfaceInteraction3f but DirectionSample3f. The emitter member is set to point to the sampled object e.g. upon call to DirectionalEmitter::sample_direction(), and nothing equivalent can happen here. So I was wondering if generalising this member to accept any EndPoint would make sense or if I should just ignore it.

Copy link
Member

Choose a reason for hiding this comment

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

Oh right, this makes sense. And what prevents you from doing so? (e.g. something related to the ptrace PR?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing, it's just out of the scope of mere plugin changes, so I just wanted to know if there was a reason not to do so :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually overlooked this: both SurfaceInteraction and DirectionSample would need some changes. I'll submit a separate PR for this.

@Speierers
Copy link
Member

Hi @leroyvn ,

Not sure to remember what was the issue with SurfaceInteraction3f?

@leroyvn
Copy link
Contributor Author

leroyvn commented Jul 8, 2021

Hi @Speierers, see this other comment for details.

@leroyvn
Copy link
Contributor Author

leroyvn commented Aug 30, 2021

I'm revisiting this PR. The issue we had back in July is still here. I started work on DirectionSample in #19 to generalise DirectionSample.emitter and turn it into a DirectionSample.endpoint. The update I did works but breaks many features (no problem to update all that of course): the change seems quite intrusive and breaks a symmetry which existed between DirectionSample and SurfaceInteraction.

Wrapping it up:

@wjakob
Copy link
Member

wjakob commented Aug 31, 2021

Hi @leroyvn,

I was just looking at the PRs you mention. It was actually not 100% clear what the issue with DirectionSample.endpoint was, can you clarify? There are some unrelated-seeming test failures regarding the Scene class. Is that the main problem, i.e. do you need help debugging those?

@leroyvn
Copy link
Contributor Author

leroyvn commented Aug 31, 2021

Hi @wjakob,

There are some unrelated-seeming test failures regarding the Scene class. Is that the main problem, i.e. do you need help debugging those?

I think I do (see #19 (comment)).

@wjakob wjakob force-pushed the master branch 8 times, most recently from d43e297 to 8bc5ae2 Compare September 13, 2021 13:21
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.

3 participants