feat(reveal): Add support for CDM 360 images in reveal #4894
Conversation
…mh/add-cdm-support-for-360-images
...providers/descriptor-providers/datamodels/system-space/Cdf360DataModelsDescriptorProvider.ts
Outdated
Show resolved
Hide resolved
.../image-360-data-providers/descriptor-providers/datamodels/cdm/Cdf360CdmDescriptorProvider.ts
Outdated
Show resolved
Hide resolved
.../image-360-data-providers/descriptor-providers/datamodels/cdm/Cdf360CdmDescriptorProvider.ts
Outdated
Show resolved
Hide resolved
| imageProp.bottom as DirectRelationReference | ||
| ]); | ||
|
|
||
| const externalIdBatches = chunk(chunk(cubeMapFileIds, 1000), 15); |
There was a problem hiding this comment.
What is 15 and can add it into Constant variable
There was a problem hiding this comment.
I'm not sure about why it's 15, but I would guess that it has something to do about the File API's concurrency limits. I asked Christopher who wrote the datamodeling provider I based this CDM implementation on, and he did not remember why 15 was chosen
| import chunk from 'lodash/chunk'; | ||
| import zip from 'lodash/zip'; | ||
| import groupBy from 'lodash/groupBy'; | ||
| import partition from 'lodash/partition'; |
There was a problem hiding this comment.
| import chunk from 'lodash/chunk'; | |
| import zip from 'lodash/zip'; | |
| import groupBy from 'lodash/groupBy'; | |
| import partition from 'lodash/partition'; | |
| import { chunk, zip, groupBy, partition } from 'lodash'; |
There was a problem hiding this comment.
Eslint asks me to import the modules individually, so I'll thus keep it as is
...viders/src/image-360-data-providers/descriptor-providers/Cdf360CombinedDescriptorProvider.ts
Outdated
Show resolved
Hide resolved
|
React components is currently not building as I have not bumped reveal there. I'll remove the changes done in react components after the person(s) reviewing has tested this as it's easier to test it through react-components. I'll then create a new PR with the changes to react-components when this PR is merged |
| } | ||
| } | ||
|
|
||
| type CoreDmFileResponse = { |
There was a problem hiding this comment.
Move the type at the top of the file
| imageProp.front as DirectRelationReference, | ||
| imageProp.back as DirectRelationReference, |
There was a problem hiding this comment.
Do you have to do this casting? Isn't the result object properly typed?
There was a problem hiding this comment.
Ah, this is the viewer, I guess we don't have proper typing on DMS-queries here?
| const [x, y, z] = [revision.eulerRotationX, revision.eulerRotationY, revision.eulerRotationZ]; | ||
| const eulerRotation = new Euler(x, z, -y, 'XYZ'); | ||
| return new Matrix4().makeRotationFromEuler(eulerRotation); | ||
| } | ||
|
|
||
| function getTranslation(): Matrix4 { | ||
| const [x, y, z] = [revision.translationX, revision.translationY, revision.translationZ]; | ||
| return new Matrix4().makeTranslation(x, z, -y); |
There was a problem hiding this comment.
Multiply by CDF_TO_VIEWER_TRANSFORM instead here
| return [ | ||
| { | ||
| fileId: fileInfos[0].id, | ||
| face: 'front', | ||
| mimeType: fileInfos[0].mimeType! | ||
| }, | ||
| { | ||
| fileId: fileInfos[1].id, | ||
| face: 'back', | ||
| mimeType: fileInfos[1].mimeType! | ||
| }, | ||
| { | ||
| fileId: fileInfos[2].id, | ||
| face: 'left', | ||
| mimeType: fileInfos[2].mimeType! | ||
| }, | ||
| { | ||
| fileId: fileInfos[3].id, | ||
| face: 'right', | ||
| mimeType: fileInfos[3].mimeType! | ||
| }, | ||
| { | ||
| fileId: fileInfos[4].id, | ||
| face: 'top', | ||
| mimeType: fileInfos[5].mimeType! | ||
| }, | ||
| { | ||
| fileId: fileInfos[5].id, | ||
| face: 'bottom', | ||
| mimeType: fileInfos[5].mimeType! | ||
| } | ||
| ] as Image360FileDescriptor[]; |
There was a problem hiding this comment.
This can be a bit nicer if you create a list with all the face names ('front', 'back' etc..) and then iterate through it to create all these objects.
| const ret = Object.values(groups) | ||
| .concat(imagesWithoutStation.map(p => [p])) | ||
| .map(imageWithFileDescriptors => { | ||
| return this.getHistorical360ImageSet(collectionId, collectionLabel, imageWithFileDescriptors); | ||
| }); | ||
|
|
||
| return ret; | ||
| } |
There was a problem hiding this comment.
Can we return right away here?
| this.getImageRevision(mainImagePropsArray[index], p.fileDescriptors) | ||
| ), | ||
| label: '', | ||
| transform: this.getRevisionTransform(mainImagePropsArray[0] as any) |
There was a problem hiding this comment.
What's up with the any? 🧐
| private getImageRevision(imageProps: ImageResultProperties, fileInfos: FileInfo[]): Image360Descriptor { | ||
| return { | ||
| faceDescriptors: getFaceDescriptors(), | ||
| timestamp: imageProps.takenAt as string |
There was a problem hiding this comment.
Isn't takenAt properly typed in imageProps?
There was a problem hiding this comment.
Looks alright - but I'd like some code deduplication between CoreDM and the system model implementations when we have the time @anders-hopland
@anders-hopland I would also like to have the |
I'll create a Jira ticket for it 👌 |
Type of change
Jira ticket 📘
BND3D-4787
Description 📝
Add support for CDM 360 images in reveal
Checklist ☑️