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

Visualizing gaussian splats #7194

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

billamiable
Copy link
Contributor

@billamiable billamiable commented Mar 8, 2025

Type

  • Bug fix (non-breaking change which fixes an issue): Fixes #
  • New feature (non-breaking change which adds functionality). Resolves #
  • Breaking change (fix or feature that would cause existing functionality to not work as expected) Resolves #

Motivation and Context

This PR, along with two other PRs (#7192, #7193), will fix #7154

Checklist:

  • I have run python util/check_style.py --apply to apply Open3D code style
    to my code.
  • This PR changes Open3D behavior or adds new functionality.
    • Both C++ (Doxygen) and Python (Sphinx / Google style) documentation is
      updated accordingly.
    • I have added or updated C++ and / or Python unit tests OR included test
      results
      (e.g. screenshots or numbers) here.
  • I will follow up and update the code if CI fails.
  • For fork PRs, I have selected Allow edits from maintainers.

Description

Copy link

update-docs bot commented Mar 8, 2025

Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes.

Copy link
Contributor

@benjaminum benjaminum left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Please have a look at the comments

for (const auto& check_item : check_list) {
if (geometry_.HasPointAttr(check_item) &&
geometry_.GetPointAttr(check_item).GetDtype() != core::Float32) {
auto check_item_instance = geometry_.GetPointAttr(check_item);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to check the attribute shapes here or in Pointcloud::IsGaussianSplat(). In ConstructBuffers() assumes that the attributes have specific sizes. f_rest can have different shapes (num_verts, num_coeffs, 3) with num_coeffs = 3, 8, or 15

Copy link
Member

Choose a reason for hiding this comment

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

Agree. PR #7153 adds this checking support for IsGassianSplat(). We can skip the checks 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.

We are adopting similar logic inside PointCloudBuffers.cpp, see implementation here.

Copy link
Contributor

@benjaminum benjaminum Mar 11, 2025

Choose a reason for hiding this comment

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

Agree. PR #7153 adds this checking support for IsGassianSplat(). We can skip the checks here.

@ssheorey
We should use the same checks as for IO here but I am not sure if we can skip checks here since the user can create a GS point cloud for visualization in memory. Am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

IsGaussianSplat() is already called before this ctor is run (see line 255 in FilamentGeometryBuffersBuilder.cpp). So we don't need to repeat the check.
We should add a commment about preconditions to this function:

///IsGaussianSplat() has verified that this point cloud contains the necessary data for a Gaussian Splat.

Copy link

@zhangyang-intel zhangyang-intel Mar 13, 2025

Choose a reason for hiding this comment

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

I think we can update this function after PR #7153 is merged.

@@ -1017,6 +1019,25 @@ void FilamentResourceManager::LoadDefaults() {
lit_mat->setDefaultParameter("anisotropyMap", texture, default_sampler);
materials_[kDefaultLit] = BoxResource(lit_mat, engine_);

const auto gaussian_path = resource_root + "/gaussianSplat.filamat";
auto gaussian_mat = LoadMaterialFromFile(gaussian_path, engine_);
Copy link
Member

Choose a reason for hiding this comment

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

TODO: Verify that color space is correct, i.e. input SH coeffs encode color in sRGB color space.

Also, are the rest of the values required by filament? They will not be used in our custom shaders.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what this means. Is it supposed to be the same as #7192 (comment)?

Copy link
Member

Choose a reason for hiding this comment

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

sRGB is a compressed color space commonly used for encoding color in images. Very approximately it's linearRGB = sRGB ^ 2.2 (color values in range 0-1). See the defaultUnlit.mat file. For rendering, we need to map back from sRGB to linear RGB. Otherwise, the images may look overly bright. If the rendering looks too bright compared to the reference viewer, this is the cause.

2nd question was about the PBR properties set below (baseCoor, baseRoughness, .....). We should remove them unless filament gives a warning or error, since we do not need them.

Choose a reason for hiding this comment

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

After some tests, i think color space is linear RGB based on the following:

  1. the tpye of f_dc and f_rest is float32 in .npz file.
  2. some of data in f_dc is negative.
  3. I try to use linear_to_sRGB and sRGB_to_linear in .mat file
    raw picture:
    raw
    linear_to_sRGB:
    l_to_srgb
    sRGB_to_linear
    srgb_to_l

Copy link
Member

@ssheorey ssheorey Mar 14, 2025

Choose a reason for hiding this comment

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

About the negative f_dc values: this is due to "ringing" artifacts (wikipedia) that you see when trying to represent a high frequency signal with low frequency approximation. Spherical harmonics are 2D analog of Fourier series and exhibit the same issue. To mitigate this issue, we should clamp the final output color values to the [0, 1] range. Apply any color processing (e.g. linear_to_sRGB) after the clamping.

for (const auto& check_item : check_list) {
if (geometry_.HasPointAttr(check_item) &&
geometry_.GetPointAttr(check_item).GetDtype() != core::Float32) {
auto check_item_instance = geometry_.GetPointAttr(check_item);
Copy link
Member

Choose a reason for hiding this comment

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

Agree. PR #7153 adds this checking support for IsGassianSplat(). We can skip the checks here.

@billamiable billamiable changed the title Doc for visualizing gaussian splats Visualizing gaussian splats Mar 12, 2025
@@ -1413,9 +1413,10 @@ int PointCloud::GaussianSplatGetSHOrder() const {
"SH degree higher than 2 is not supported. The SH degree "
"of in input data is {}, we set it as 2.",
deg);
deg = 2;
Copy link
Member

Choose a reason for hiding this comment

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

Hi @billamiable we don't support sh degree > 2 for rendering, but we do support it for other operations (reading and writing 3DGS files, and in the future editing 3DGS data). I would skip this warning here, and instead only add it if the user tries to display a 3DGS point cloud with 3 or higher degree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Visualize 3D Gaussian splats
4 participants