Skip to content

Project4: Linda Zhu#11

Open
LinDadaism wants to merge 6 commits into
CIS565-Fall-2023:base-codefrom
LinDadaism:base-code
Open

Project4: Linda Zhu#11
LinDadaism wants to merge 6 commits into
CIS565-Fall-2023:base-codefrom
LinDadaism:base-code

Conversation

@LinDadaism

@LinDadaism LinDadaism commented Oct 21, 2023

Copy link
Copy Markdown

Repo link

Features:

  • G-Buffers for normals and positions
  • Edge-avoiding A-Trous filtering denoiser

Feedback:
I made all the changed in this very basic version of path tracer code base, instead of merging the denoiser to my project 3. Even just with this simple code base, there seems to be a few mistakes that led to confusion. Another issue with the base code is it is not self-explanatory, no meaningful comments, which can be hard to follow if we want to build something upon it.

  • First of all, the G-buffer visualizations for positions and normals in the INSTRUCTIONS are incorrect. The surface normals for the left and right walls are apparently the opposite. How come they have the same color? I assume the normal map was calculated by flipping the negatives in the [-1, 1] range to [0,1], instead of the correct way like uv conversion, which is ([-1, 1]+ 1) / 2.
  • In gbufferToPBO(), whatever the intersection data they had was multiplied to 256 while the color RGB can only reside in the range of [0, 255].
  • Ignoring the fact that the base code is just for demonstration so we shouldn't reply on it too much, the paper based off which we are trying to implement the denoiser is outdated and ambiguous in explaining the parameters we used in the math equations like where do they get the kernel for pure blur. I had to revisit the paper for a few times to figure out how they calculate the number of denoising passes applied to the image, which is in the annotation of a table/chart, not even mentioned in the main texts.

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.

1 participant