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

Switching the gaussian-splatting dataparser from ColmapDataParser to NerfStudioDataparser and updating ns-process-data #2791

Merged
merged 20 commits into from
Jan 19, 2024

Conversation

CardiacMangoes
Copy link
Contributor

Hey guys!

The goal of this PR is to switch the ColmapDataParserConfig that's currently default in the gaussian-splatting method to NerfstudioDataParserConfig

In order to do so, I update ns-process-data so that it outputs a .ply file in its outputs along with a path to the file named "ply_file_path" in transforms.json. The default name of this file is "sparse_pc.ply"

I also added some backwards compatibility so that this commit does work with older datasets. If the dataset includes colmap outputs but a .ply point cloud, we ask the user if we can create one for them.

@@ -460,6 +462,11 @@ def colmap_to_json(
applied_transform[2, :] *= -1
out["applied_transform"] = applied_transform.tolist()

# create ply from colmap
assert ply_filename.endswith(".ply"), f"ply_filename: {ply_filename} does not end with '.ply'"
create_ply_from_colmap(ply_filename, recon_dir, output_dir)
Copy link
Collaborator

@jb-ye jb-ye Jan 19, 2024

Choose a reason for hiding this comment

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

There is a bug here, when creating ply from colmap, you need to apply applied_transform to the point cloud similar like here: https://github.com/nerfstudio-project/nerfstudio/blob/main/nerfstudio/data/dataparsers/colmap_dataparser.py#L354-L366

Otherwise, the camera poses exported to nerfstudio convention will be in different world coordinate as point clouds. And the quality of training GS will be severely affected. See more discussions here:

#2793 (not directly related, but good to know in general)

@jb-ye
Copy link
Collaborator

jb-ye commented Jan 19, 2024

I am actually against changing the default parser to nerfstudio because majority of community users are using colmap directly now. If this is decided by the nerfstudio team, I would prefer I participate in validating this PR, since converting colmap project to nerfstudio format and ensuring all things are well arranged requires a bit more careful checks. @tancik

@brentyi
Copy link
Collaborator

brentyi commented Jan 19, 2024

I am actually against changing the default parser to nerfstudio because majority of community users are using colmap directly now. If this is decided by the nerfstudio team, I would prefer I participate in validating this PR, since converting colmap project to nerfstudio format and ensuring all things are well arranged requires a bit more careful checks. @tancik

I think we should prioritize consistency here; since all of the other methods default to nerfstudio-data, it's unideal if the splatting model is the only one that defaults to colmap.

Maybe there's some middle ground, like having the nerfstudio dataparser support loading raw colmap datasets?

@jb-ye
Copy link
Collaborator

jb-ye commented Jan 19, 2024

I am actually against changing the default parser to nerfstudio because majority of community users are using colmap directly now. If this is decided by the nerfstudio team, I would prefer I participate in validating this PR, since converting colmap project to nerfstudio format and ensuring all things are well arranged requires a bit more careful checks. @tancik

I think we should prioritize consistency here; since all of the other methods default to nerfstudio-data, it's unideal if the splatting model is the only one that defaults to colmap.

Maybe there's some middle ground, like having the nerfstudio dataparser support loading raw colmap datasets?

I am fine with the consistency rationale. Some comments:
(1) I could help validate the PR to ensure the change is compatible with #2793
(2) Add some docs to describe how to use GS from a colmap project either from a colmap parser or converting colmap to nerfstudio and use nerfstudio parser.
(3) Ideally both path (colmap->gs and colmap->nerfstudio->gs) should create the same artifacts post training (and this requires a bit more careful checks)

Copy link
Collaborator

@brentyi brentyi left a comment

Choose a reason for hiding this comment

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

I adjusted the coordinate frame logic + left some comments with my understanding of each of them -- things look good when I tested but if you could double-check @CardiacMangoes @jb-ye that would be appreciated!

@brentyi brentyi enabled auto-merge (squash) January 19, 2024 07:57
@brentyi brentyi merged commit 288e149 into main Jan 19, 2024
4 checks passed
@brentyi brentyi deleted the gsplat_colmap_to_nerfstudio_dataparser branch January 19, 2024 08:00
@jb-ye
Copy link
Collaborator

jb-ye commented Jan 19, 2024

lgtm, I will test out later this week. I left one comment for my own understanding.

ArpegorPSGH pushed a commit to ArpegorPSGH/nerfstudio that referenced this pull request Jun 22, 2024
…NerfStudioDataparser and updating ns-process-data (nerfstudio-project#2791)

* ns-process-data updated to output a .ply of sparse points from colmap

* docs for loading 3D points

* format fixing

* ruff formatting

* added read_points3D_text

* formatting

* added 3D points to test

* prompted user bool fix

* small fixes to pass test cases

* fix colors

* fixed coord system

* Tweak frames

* Comment wording

* ruff

* Comment intentional default change for load_3D_points

* Wording

* More wording

* Fix double/float issue

---------

Co-authored-by: Ethan Weber <[email protected]>
Co-authored-by: Brent Yi <[email protected]>
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.

4 participants