Skip to content

Conversation

@RongLiu-Leo
Copy link
Contributor

Video.Project.3.mp4

Besides to rendering full SH as RGB, this PR renders SH=0 component as the view independent diffuse map and SH>0 components as the view dependent specular map.

Copy link
Collaborator

@liruilong940607 liruilong940607 left a comment

Choose a reason for hiding this comment

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

Left some comments! I like the idea of supporting diffuse / specular in viewer!

pass
else:
# Colors are SH coefficients, with shape [..., N, K, 3] or [..., C, N, K, 3]
colors = colors.clone()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This clone() breaks the auto diff graph for training. (Also it is not ideal from the efficiency perspective)

I think in this case it's better to not touch this rendering.py file. We could just update the viewer file to support diffuse/specular.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it is a good idea

Comment on lines +1098 to +1099
"diffuse": "Diffuse",
"specular": "Specular",
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we dont touch rendering.py we we remove these from here..

make colors = colors.clone() only reachable in no_grad view function
Copy link
Collaborator

@liruilong940607 liruilong940607 left a comment

Choose a reason for hiding this comment

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

Only request is to move the diffuse/specular out of the rasterization function.

Comment on lines +498 to +502
# SH = 0 is the view-independent diffuse component, SH >= 1 are view-dependent specular components.
if render_mode in ("Diffuse", "Specular"):
colors = colors.clone()
sel = slice(1, None) if render_mode == "Diffuse" else slice(0, 1)
colors[..., sel, :] = 0.0 # Zero out the unwanted SH components.
Copy link
Collaborator

@liruilong940607 liruilong940607 May 29, 2025

Choose a reason for hiding this comment

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

I still think this logic should live outside of the rasterization function (into viewer_render_fn). For two reasons:

  • What if I want to rendering diffuse color + depth map? there is no "Diffuse+D" mode here. User would end up have to write this logic outside of rasterization function and use "RGB+D" mode anyway. (The major reason)
  • I'm still not quite happy with the inplace operation colors[..., sel, :] = 0.0 and colors.clone(). It's not very good to have inplace operations in the torch differentiable graph. It might break the auto diff graph or lead to unused parameters.

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.

2 participants