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 fov xy mismatch #99

Merged
merged 1 commit into from
Jun 6, 2024
Merged

Fix fov xy mismatch #99

merged 1 commit into from
Jun 6, 2024

Conversation

Willyzw
Copy link
Contributor

@Willyzw Willyzw commented Jun 5, 2024

Hi, I found another inconsistency in the CPU pipeline, which is the mismatched x and y in the fov calculation.

@pierotofy
Copy link
Owner

Hey @Willyzw 👋 thanks the PR. Have you tested these changes with the simple_trainer app?

In particular, try to run it with --width 100 --height 50 --render ./render --cpu and compare the output of the render files with the GPU run (toggling --cpu).

@pierotofy
Copy link
Owner

Now that I think about it, this should also be tested via a normal run (e.g. ./opensplat /path/to/banana --render-val ./render --cpu -n 300) and compare outputs to the GPU, since fovx/fovy values are equal in the simple_trainer.

@Willyzw
Copy link
Contributor Author

Willyzw commented Jun 6, 2024

Now that I think about it, this should also be tested via a normal run (e.g. ./opensplat /path/to/banana --render-val ./render --cpu -n 300) and compare outputs to the GPU, since fovx/fovy values are equal in the simple_trainer.

During my testing with the simple_trainer and the banana dataset, I got same results with and without the changes I made. However, based on my experience, training the model using the CUDA pipeline and validating the rendered image with CPU, with my changes it can lead to improved validation. Also intuitively, we use “x” to refer to width and “y” to refer to height, right?

Regarding the failed check, I have no clue why it failed. Could you help check it?

@pierotofy
Copy link
Owner

pierotofy commented Jun 6, 2024

Did some more analysis and it does indeed look like these are swapped. It wasn't apparent during testing because the banana dataset has the same focal values for X/Y.

The truck dataset has different values and is a better one to test against.

Thanks for the fix!

The Windows build issue is unrelated to this PR.

@pierotofy pierotofy merged commit 39f0e04 into pierotofy:main Jun 6, 2024
26 of 27 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