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

Adding camera bounds cause incorrect bounding box estimation #47

Closed
ox opened this issue Aug 10, 2018 · 6 comments · Fixed by #48
Closed

Adding camera bounds cause incorrect bounding box estimation #47

ox opened this issue Aug 10, 2018 · 6 comments · Fixed by #48

Comments

@ox
Copy link
Contributor

ox commented Aug 10, 2018

else if self.children.is_empty() {
// Cameras (others?) have neither mesh nor children. Their position is the origin
// TODO!: are there other cases? Do bounds matter for cameras?
self.bounds = Aabb3::zero();
self.bounds = self.bounds.transform(&self.final_transform);

I stumbled onto this TODO after trying to take a screenshot of a model with a camera, but using bounding box estimation. It seems that the camera bounds are being taken into account when trying to set a nice camera position, causing the bounding box to be incorrectly estimated.

Here is the screenshot produced by gltf-viewer:
heart-render-1080

And here is the scene view in Houdini, notice the small camera dot in the bottom left:
screenshot from 2018-08-10 13-40-41

I'm attempting to get the camera node's bounds to not count in the scene bounds, but my Rust knowledge is very limited. I hacked together this diff that seems to resolve the problem:

diff --git a/src/render/node.rs b/src/render/node.rs
index a1dc13c..80a487f 100644
--- a/src/render/node.rs
+++ b/src/render/node.rs
@@ -106,6 +106,10 @@ impl Node {
             self.bounds = mesh.bounds
                 .transform(&self.final_transform);
         }
+        else if let Some(ref _camera) = self.camera {
+            // Skip updating bounds for Cameras, since they don't have bounds.
+        }
         else if self.children.is_empty() {
             // Cameras (others?) have neither mesh nor children. Their position is the origin
             // TODO!: are there other cases? Do bounds matter for cameras?

Is this the correct solution? Do you have any other ideas about how to fix this?

@bwasty
Copy link
Owner

bwasty commented Aug 11, 2018

Looks good - a slight simplification would be else if self.camera.is_some().

@ox
Copy link
Contributor Author

ox commented Aug 13, 2018

awesome. should I make a PR or...?

@bwasty
Copy link
Owner

bwasty commented Aug 14, 2018

Yes, that would be great :)

@ox
Copy link
Contributor Author

ox commented Aug 22, 2018

when could I expect this change to make it onto crates.io? I'd love to be able to just cargo install with a version rather than with a git url and sha.

@bwasty
Copy link
Owner

bwasty commented Aug 22, 2018

Here it is: https://crates.io/crates/gltf-viewer :)

@ox
Copy link
Contributor Author

ox commented Aug 22, 2018

wow, thank you for the quick reply and publish! I really appreciate it!

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 a pull request may close this issue.

2 participants