-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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 (web/server) 360 degrees Web panoramas #3098
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Probably I need to add some tests, but not sure. Need some advise. |
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.
You can try running make api
to regenerate the api. You can also run the npm run check:all
scripts in the web/server folders to make sure those all pass.
Hi! |
@dmitry-brazhenko hello, I haven't gotten a chance to review the PR yet. Once I do and if there is nothing else to address, it will be merged. Thank you for the contribution |
server/src/microservices/processors/metadata-extraction.processor.ts
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.
Is it possible to include this css in the asset-viewer.svelte component directly via a <style> ... </style>
tag at the end of the file instead?
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 tried to do that, but it did not work. Here is a lib example: https://github.com/naver/egjs-view360/blob/master/packages/svelte-view360/demo/App.svelte
I overrided original css file. I asked a question here, but the suggestion did not work.
I am not very good in frontend technologies. If there are any ways to enable, please let me know, I will fix it :)
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 think it can work, but you might need to add something special to make the style not scoped to the component.
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.
Need to double check the API generated code, but other than that LGTM.
localVarFormParams.append('{{baseName}}', {{paramName}} as any);{{/isPrimitiveType}}{{^isPrimitiveType}}{{#isEnum}} | ||
localVarFormParams.append('{{baseName}}', {{paramName}} as any);{{/isEnum}}{{^isEnum}} | ||
localVarFormParams.append('{{baseName}}', new Blob([JSON.stringify({{paramName}})], { type: "application/json", }));{{/isEnum}}{{/isPrimitiveType}}{{/multipartFormData}} | ||
localVarFormParams.append('{{baseName}}', {{paramName}} as any);{{/isPrimitiveType}}{{^isPrimitiveType}} | ||
localVarFormParams.append('{{baseName}}', new Blob([JSON.stringify({{paramName}})], { type: "application/json", }));{{/isPrimitiveType}}{{/multipartFormData}} |
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.
Need to double check where this change is coming from
I think there is some issue with the recent rebase/merge, can you only include the changes to the panorama view? |
After I ran "make api" - it resulted to multiple file changes |
These changes somehow reverting some of the changes that we made. How about you only include the changes in the |
Yes, I will do that |
2654895
to
c6d23e8
Compare
@alextran1502 I reverted my attempts to merge main branch to my branch. |
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.
Please remove this file from your PR
docker/docker-compose.dev.yml
Outdated
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.
Please remove this file from your PR
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.
Why not to keep it?
Once I started running Immich, it was failing as containers just stopped. Adding this restart;always helped me to get started.
btw, I removed it for now, but I can create a separate PR and merge it there
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.
Please remove this file from your PR
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.
Why remove it? I fixed the path?
https://github.com/immich-app/immich/tree/main/server/src/infra/entities
btw, I reverted it, but I can create a separate PR and merge it there
@alextran1502 would you mind if merge docker compose improvement and MD fix in a separate PR? |
I think 360 panoramas should be detected more reliably in this PR. Currently, the code assumes that all images with an aspect ratio >= 2 are 360 panoramas. But, for example:
I believe we should use the EXIF tag |
I agree. My google pixel photosphere images have the following projection type:
I also have photoshere images that are not complete 360 degree images so the aspect ratio is not 2:1. |
@CodeWithMa @brighteyed you're right. I fixed panorama detection. Now it consumes Exif and marks images by "panorama" only if there is a necessary exif tag. @alextran1502 Could you help me with merging this PR? Thanks |
Can you create another PR to consolidate the changes on the web and the server? Please put migrations into a single file. Once you've done that please ping me and I can help generating the API |
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 we transform isPanorama
fields into projectionType
it will be easier in the future to add additional types of projections (most likely cylindrical
)
@@ -32,6 +32,7 @@ export class AssetResponseDto { | |||
people?: PersonResponseDto[]; | |||
/**base64 encoded sha1 hash */ | |||
checksum!: string; | |||
isPanorama!: boolean; |
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'd prefer projectionType
field with the enum
type
@@ -58,6 +59,7 @@ export function mapAsset(entity: AssetEntity): AssetResponseDto { | |||
tags: entity.tags?.map(mapTag), | |||
people: entity.faces?.map(mapFace), | |||
checksum: entity.checksum.toString('base64'), | |||
isPanorama: entity.isPanorama, |
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'd prefer projectionType
field with the enum
type
@@ -43,6 +43,9 @@ export class ExifEntity { | |||
@Column({ type: 'float', nullable: true }) | |||
longitude!: number | null; | |||
|
|||
@Column({ type: 'varchar', nullable: true }) | |||
projectionType!: string | null; |
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.
Can we use enum
here?
Will go on in this PR: #3412. Implemented Projectiontype as enum |
In this PR I aded
How it was implemented
Minor technical changes and improvements:
Potential further improvements:
Here is thumb that shows a panorama:
Panorama after is opened: