-
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
feat(react-components): FDM scene loader component #3971
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #3971 +/- ##
=======================================
Coverage 73.54% 73.54%
=======================================
Files 357 357
Lines 35855 35855
Branches 2735 2736 +1
=======================================
+ Hits 26369 26371 +2
+ Misses 9381 9379 -2
Partials 105 105 |
…ndersmh/fdm-scene-loader-component
onLoadError={onModelLoadedError} | ||
/> | ||
); | ||
if ('siteId' in addModelOption) { |
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.
That could be a nice helper function to distinguish between FDM/events based 360 images.
sceneExternalId, | ||
sceneSpaceId, | ||
sdk |
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.
The API for this component should be an extension of Reveal3DResources
component, i.e. support styling of instances and callback for when models are loaded.
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.
The reason for that is FDX and other apps relies on styling capabilities a lot.
react-components/src/components/SceneContainer/SceneContainer.tsx
Outdated
Show resolved
Hide resolved
react-components/src/components/SceneContainer/SceneContainer.tsx
Outdated
Show resolved
Hide resolved
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.
Looks good but a few questions related to queries and coordinates for FDM stored data.
// There is a bug with setCameraState rotation, so must first set transform | ||
// and then lookAt the correct direction | ||
viewer.cameraManager.setCameraState({ position }); | ||
viewer.cameraManager.getCamera().lookAt(position.clone().add(vec)); |
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's very wrong to do that. Any camera manipulation should be done through setCameraState
.
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.
If I set the position and rotation like follows:
viewer.cameraManager.setCameraState({ position, rotation: quaternion });
I get a problem where I can not rotate my camera, only pane it. I have tried with several different rotations, so the problem should not be a one off where I have used a specific rotation.
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.
We should then investigate the issue within reveal. The reason why we always should use setCameraState is because changes in camera transformation affects when and how to render sectors of the model and we track "changes" through the method 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.
I also just realised that it is probably better to set camera parameteres with position and target instead of rotation. What do you think about that?
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 will be also much easier to convert it from/to CDF coordinates
…nContainer signature
…ndersmh/fdm-scene-loader-component
…com/cognitedata/reveal into andersmh/fdm-scene-loader-component
…com/cognitedata/reveal into andersmh/fdm-scene-loader-component
Type of change
Jira ticket 📘
BND3D-2319
Adds component in reveal react components for loading scenes stored in FDM. A scene is used to define everything that goes into a 3D scene. This include:
How has this been tested? 🔍
Test instructions ℹ️
Checklist ☑️