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

Fix projection for images with non-centered camera (e.g. crops) #305

Merged
merged 3 commits into from
Aug 1, 2024

Conversation

JonathonLuiten
Copy link
Contributor

The current code assumes [cx,cy] are at [w/2, h/2], if this is not the case (very common if you want to render a non-centered crop) the code gives VERY bad and incorrect results. This quick fix uses the proper [cx, cy].

image

The current code assumes [cx,cy] are at [w/2, h/2], if this is not the case (very common if you want to render a non-centered crop) the code gives VERY bad and incorrect results. This quick fix uses the proper [cx, cy].
Copy link
Collaborator

@maturk maturk left a comment

Choose a reason for hiding this comment

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

Wow, nice. LGTM!

@liruilong940607
Copy link
Collaborator

Any reason the backward (persp_proj_vjp) is not updated?

@jb-ye
Copy link
Collaborator

jb-ye commented Jul 27, 2024

This seems a major bug. Could you also fix the backward pass as well?

BTW, we also need to publish this fix to 0.1.xx version.

dendenxu added a commit to dendenxu/fast-gaussian-rasterization that referenced this pull request Jul 28, 2024
dendenxu added a commit to dendenxu/fast-gaussian-rasterization that referenced this pull request Jul 28, 2024
@hbb1
Copy link

hbb1 commented Jul 29, 2024

can we do this instead, without modifying the rasterizer? graphdeco-inria/gaussian-splatting#144 (comment)

@f-dy
Copy link

f-dy commented Jul 29, 2024

Can someone explain why the clamping?

The original 3DGS code has it too, and a comment says:

// The following models the steps outlined by equations 29
// and 31 in "EWA Splatting" (Zwicker et al., 2002).
// Additionally considers aspect / scaling of viewport.
// Transposes used to account for row-/column-major conventions.

But EWA splatting has no clamping.

What it does is that it clamps the 2D expansion (due to the projection) of Gaussians that are outside of the view frustum. My guess is that it's something that was introduced at some point, maybe to fix a visual artifact, and left there for no reason (maybe that artifact was due to something else).

I can understand the reason for clamping, when considering very large Gaussians: The projection cannot be considered to be affine anymore, and maybe the clamping was introduced to avoid very large Gaussians that are outside of the view frustum to come inside the view frustum because of the affine approximation, which is computed at the center of the Gaussian and not at the "side" of the Gaussian that is closer to the view frustum. But then why treat Gaussians that are outside and not those that are inside? The projection of Gaussians that are inside should actually be larger (that same Jacobian is higher on the sides of Gaussians that are at the principal point).

I think the correct fix is to remove the clamping.

@JonathonLuiten
Copy link
Contributor Author

@liruilong940607 yes this probably needs to be changed in the backward pass too! I don't have time to look into this myself right now though unfortunately.

@hbb1 The function you linked doesn't fix this issue, this actually needs to be changed in the cuda like suggested in this PR.

@f-dy 'Why clamping': If you do nothing, the 'approximate jacobian projection' of Gaussians out to the side of field of view make these (normally small) Gaussians very large and project into the field of view and results in complete mess (no good image at all), so we absolutely need to do something about this.

There are two solutions:
a) either completely remove Gaussians far out of field of view, e.g. Gaussians outside of 1.3*fov are completely removed / culled. This is a fine solution imo.

b) (done here) make the Gaussians outside of field of view smaller (e.g. clip their size to the size it would be if they were 1.3fov away). The advantage of this over (a) is that if you have VERY large Gaussians outside of 1.3fov that actually SHOULD contribute to the image, in this setting they are able to, whereas if you just completely removed them as in (a) they wouldn't be able to contribute.

Note that in (b) there is still culling of Gaussians to only those in the frustum, but the culling is not based on the center of gaussian location, but the projected 2D bounding box of the Gaussian (based upon the clipped projection here).

Let me know if this makes sense?

Note @f-dy if you naively try your solution to remove the clamping alone, you will get absolutely rubbish results (blury image that looks NOTHING like the scene) because all the Gaussians out to the side are basically filling the whole image with their (incorrect because of linear approximation) projection.
However an alternative (and reasonable, but maybe not as good) solution is to remove all point where their center is outside the 1.3fov (as described in a above)

@liruilong940607
Copy link
Collaborator

I just finished the backward pass fix as well as the torch impl fix. cc for review @maturk @JonathonLuiten @jb-ye

@liruilong940607 liruilong940607 requested review from maturk and jb-ye July 31, 2024 23:16
@liruilong940607
Copy link
Collaborator

I checked the tests/ locally, all tests passed

Copy link
Collaborator

@jb-ye jb-ye left a comment

Choose a reason for hiding this comment

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

lgtm

@liruilong940607 liruilong940607 merged commit 40fd2fe into nerfstudio-project:main Aug 1, 2024
2 checks passed
@AtticusZeller
Copy link

sorry to bother you guys!thanks for your incredible work of art.may i
ask a question about the fixing,does it affect the depth composing of gsplat?

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.

7 participants