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 parsing of non ascii image names in colmap's images.bin #3256

Merged

Conversation

TamirCohen
Copy link
Contributor

@TamirCohen TamirCohen commented Jun 25, 2024

If colmap image file name is non ascii an error will be thrown in the line current_char.decode("utf-8") while trying to parse the file images.bin
It is thrown because it tries to decode a single byte as utf-8, And non ascii chars are represented using more bytes than 1.

My solution proposal is to read all the bytes, and than decode it.
I tested it and it worked OK

Copy link
Collaborator

@kerrj kerrj left a comment

Choose a reason for hiding this comment

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

looks good, also tested on a few datasets and it works thanks!

@TamirCohen
Copy link
Contributor Author

Thanks @kerrj.
Due to an error in the sphynx build I can not merge it (I do not think it is related to my commit).
Should I wait after #3265 is fixed and then merge it?

@kerrj kerrj enabled auto-merge (squash) June 27, 2024 23:08
@kerrj
Copy link
Collaborator

kerrj commented Jun 28, 2024

Could you re-run lint please? the linter was updated along with the np 2.0 patch

@TamirCohen
Copy link
Contributor Author

TamirCohen commented Jun 28, 2024

I reran it and it did not work because there are empy comments in:

https://github.com/nerfstudio-project/nerfstudio/blame/d6df39b70d2e0b444178d23b694989e39d624781/tests/pipelines/test_vanilla_pipeline.py#L45

Should we remove these empty comment in another PR or in this PR?

@jb-ye
Copy link
Collaborator

jb-ye commented Jun 28, 2024

Just rebase to the top main should resolve the issue.

@kerrj kerrj merged commit 1af71f2 into nerfstudio-project:main Jun 28, 2024
2 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.

3 participants