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

Add a point cloud option for Record3D #3556

Merged
merged 4 commits into from
Dec 24, 2024

Conversation

Jameson-Crate
Copy link
Contributor

#2681

Added the option to add a point cloud to Record3D. Record3D has an export ply option which gives a dense point cloud for every camera. I just combine them all and downsample to make the point cloud sparse using open3d. I also changed the documentation under Getting Started > Using Custom Data > Record3D.

Copy link
Contributor

@akristoffersen akristoffersen 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 doing this! some minor changes requested but otherwise LGTM

@@ -101,7 +104,10 @@ def main(self) -> None:
)

metadata_path = self.data / "metadata.json"
record3d_utils.record3d_to_json(copied_image_paths, metadata_path, self.output_dir, indices=idx)
ply_path = self.ply if hasattr(self, "ply") else None
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure this is necessary? I think you can just pass in self.ply into record3d_to_json.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh you're right, sorry about that

@@ -49,6 +49,9 @@ class ProcessRecord3D(BaseConverterToNerfstudioDataset):
2. Converts Record3D poses into the nerfstudio format.
"""

ply: Optional[Path] = None
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Might want to call this ply_dir instead to emphasize that it's a directory, not a ply file itself.

Is the reason for doing a dir instead of the ply filepath because Record3D can generate multiple ply's?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea exactly, when you export Record3D gives a bunch of ply's, ill make the change rn

pcd = o3d.geometry.PointCloud()
for ply_filename in ply_dirname.iterdir():
temp_pcd = o3d.io.read_point_cloud(str(ply_filename))
pcd += temp_pcd.voxel_down_sample(voxel_size=0.8)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably make voxel_size an argument, and maybe include it in the ProcessRecord3D settings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thats a good idea

@@ -268,6 +268,35 @@ ns-process-data record3d --data {data directory} --output-dir {output directory}
ns-train nerfacto --data {output directory}
```

### Adding a Point Cloud

Adding a point cloud is useful for avoiding random initialization
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: seems like this was split into multiple lines unnecesarily?

Copy link
Contributor

@akristoffersen akristoffersen left a comment

Choose a reason for hiding this comment

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

LGTM!

@akristoffersen akristoffersen enabled auto-merge (squash) December 24, 2024 00:51
@Jameson-Crate
Copy link
Contributor Author

Thanks!

@akristoffersen akristoffersen merged commit 0ced5ce into nerfstudio-project:main Dec 24, 2024
3 checks passed
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