-
Notifications
You must be signed in to change notification settings - Fork 20
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): Update recalculateBoundingBox in Cognite3DViewer #4715
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 Just one question
sceneBoundingBox.union(temporaryBox); | ||
}); | ||
this._sceneHandler.customObjects.forEach(customObject => { | ||
if (!customObject.object.visible) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to have the pass === 0
-check here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It didn't have that from before. It always used the visible object in these cases.
Type of change
Description 📝
the function
fitCameraToModels
only take all the models, even invisible and doesn't use thecustomobject
at all. In order not to break the code, I made an alternative which isfitCameraToSceneBoundingBox
. This uses the getSceneBoundingBox.I also change the
recalculateBoundingBox
to use only visible object. See comment in that function. I believe that was the intension of the original code. It might break the code.... An alternative would be to make another function for doing this.I saw this bug during a demo today. The user had many models, but only one was visible. The navigation was pretty hard and the fit all button didn't work as expected.