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

[Do not merge] enable gaussian rendering based on filament #7186

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

zhangyang-intel
Copy link

@zhangyang-intel zhangyang-intel commented Mar 6, 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

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 6, 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.

@ssheorey ssheorey self-requested a review March 6, 2025 22:58
@ssheorey ssheorey marked this pull request as draft March 6, 2025 22:58
@@ -1369,6 +1369,16 @@ core::Tensor PointCloud::ComputeMetrics(const PointCloud &pcd2,
metrics, params);
}

bool PointCloud::IsGaussian() const {
std::vector<std::string> keys_to_check = {"f_dc", "f_rest", "normals", "opacity", "rot", "scale"};
Copy link
Member

Choose a reason for hiding this comment

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

Can you rename to IsGaussianSplat?
We should skip "f_rest" from this list. f_rest is optional.

Copy link
Author

@zhangyang-intel zhangyang-intel Mar 7, 2025

Choose a reason for hiding this comment

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

I rename this member function to IsGaussianSplat.
And i remove "f_rest", "normals" from this list.
I also modified the logic so that the geometry is determined as a Gaussian splat only if all keys in the above list are contained in point_attr_.

@@ -102,7 +102,8 @@ std::unordered_map<std::string, MaterialHandle> shader_mappings = {
ResourceManager::kDefaultUnlitPolygonOffsetShader},
{"unlitBackground", ResourceManager::kDefaultUnlitBackgroundShader},
{"infiniteGroundPlane", ResourceManager::kInfinitePlaneShader},
{"unlitLine", ResourceManager::kDefaultLineShader}};
{"unlitLine", ResourceManager::kDefaultLineShader},
{"gaussian", ResourceManager::kGaussian}};
Copy link
Member

Choose a reason for hiding this comment

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

Following the naming convention for the others, can we rename to {"gaussianSplat", ResourceManager::kGaussianSplatShader} ?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, i have renamed it.
Maybe a suggestion, the key in shader_mappings is string constant, every time you update the key, you need to remember to update the corresponding pat in member function UpdateMaterialProperties.

Copy link
Member

Choose a reason for hiding this comment

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

If possible, can we reuse some functions here from the other BufferBuilder files? We can put common functions in a new file.

Copy link
Author

Choose a reason for hiding this comment

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

Currently, Class TGaussianSplatBuffersBuilder is a subclass of Class TPointCloudBuffersBuilder so there isn't redundant code.

@billamiable billamiable force-pushed the enable-gaussian-rendering branch from d45f784 to b7c9cb5 Compare March 8, 2025 05:39
@WeiguangHan WeiguangHan mentioned this pull request Mar 8, 2025
9 tasks
@billamiable billamiable mentioned this pull request Mar 8, 2025
9 tasks
@benjaminum
Copy link
Contributor

Just a note that #7194 supersedes this PR. It is probably easier to apply potential final changes to #7194 before merge

@ssheorey ssheorey changed the title enable gaussian rendering based on filament [Do not merge] enable gaussian rendering based on filament Mar 10, 2025
@billamiable
Copy link
Contributor

Just a note that #7194 supersedes this PR. It is probably easier to apply potential final changes to #7194 before merge

Agree!

Co-authored-by: hanyin-intel [email protected]
@billamiable
Copy link
Contributor

We don't need to update this PR anymore, let's focus on updating #7194

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.

4 participants