search

facebookresearch / synsin

View synthesis for the public.
http://www.robots.ox.ac.uk/~ow/synsin.html
Other
659 stars 93 forks source link

module 'pytorch3d.renderer.compositing' has no attribute 'CompositeParams' #18

Closed FengQiaojun closed 3 years ago

FengQiaojun commented 3 years ago

Hi,

Thanks for sharing this! I was running the Simple Demo.ipynb and got this error Screenshot from 2020-07-14 17-17-41 It seems that pytorch3d v0.2.0 removed compositing.CompositeParams. Also the fourth param of compositing function is not used anywhere in compositing.py. def alpha_composite(pointsidx, alphas, pt_clds, blend_params=None) The blend_params is not used.

Does this change matter? Then we need to change models/layers/z_buffer_layers.py according right? I tried to comment out every "params" and it worked. Is it the right solution?

gkioxari commented 3 years ago

Yes we removed CompositeParams in PyTorch3D because it was not used anywhere. The blend_params in alpha_composite or the other composite functions was actually not passed in to the actual compositing source function, as you also note above. The radius comes only in to play here: https://github.com/facebookresearch/synsin/blob/82ff948f91a779188c467922c8f5144018b40ac8/models/layers/z_buffer_layers.py#L82

This is why we decided to remove it from PyTorch3D to clean the codebase up and not cause confusion. Maybe @oawiles can verify whether our change breaks the SynSin codebase. I am almost positive we don't need CompositeParams in which case we can remove it from SynSin too! @FengQiaojun yes your solution is the right one and the one we will proceed with once @oawiles verifies the change!

oawiles commented 3 years ago

Yeah I think as Georgia said this won't actually make a difference as it wasn't used. What you did @FengQiaojun should be fine. You can always double check by running a notebook and comparing the output with your code to the preloaded values.

gkioxari commented 3 years ago

@oawiles Should we fix it in the codebase?! Or @FengQiaojun once you verify the results as @oawiles suggested do you want to submit a PR to propagate the fix to the codebase?

oawiles commented 3 years ago

If @FengQiaojun wants to submit a PR I don't see why not! I can double-check the PR to make sure that it looks ok. But I think the only thing that was passed in the params was the radius, so provided that's set correctly that should be it!

FengQiaojun commented 3 years ago

OK I submitted one PR in https://github.com/facebookresearch/synsin/pull/19. More info provided there. Thanks!