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

feat (web/server) 360 degrees Web panoramas [attempt 2] #3412

Merged
merged 45 commits into from
Jul 28, 2023

Conversation

dmitry-brazhenko
Copy link
Contributor

@dmitry-brazhenko dmitry-brazhenko commented Jul 24, 2023

Initial PR: #3098

In this PR I aded

  1. Support for 360 Panoramas as it was discussed here: [Feature]: Panoramas: Use PhotoSphere Viewer to display photos with equirectangular projection #1630

How it was implemented

  1. Once an images is uploaded, in metadata-extraction.processor.ts it checks aspectRatio. We check exif (ProjextionType) property to determine if it is a panorama.
  2. If it is a panorama, we add a property projectionType as Enum (none/EQUIRECTANGULAR/...)
  3. We use library @egjs/svelte-view360 to show the panorama
  4. In the library we use a special symbol to display if a thumb is a panorama.

Potential further improvements:

  1. Probably we can switch library and use smth else. But this liibrary works OK both on desktop and mobile-web
  2. Support panoramas in Flutter mobile app
  3. There 360 videos. Later we can support them as well

Here is thumb that shows a panorama:
2023-07-03_17-48-25

Panorama after is opened:
2023-07-03_17-50-11

@vercel
Copy link

vercel bot commented Jul 24, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
immich ⬜️ Ignored (Inspect) Jul 27, 2023 6:16pm

@dmitry-brazhenko dmitry-brazhenko changed the title Web panoramas 2 feat (web/server) 360 degrees Web panoramas [attempt 2] Jul 24, 2023
@dmitry-brazhenko dmitry-brazhenko marked this pull request as ready for review July 24, 2023 09:42
@dmitry-brazhenko
Copy link
Contributor Author

@alextran1502 here is a new PR as you asked here #3098

Need your help to merge it.

I added fix: changed "isPanorama" property to ProjectionType enum property.

server/src/infra/entities/exif.entity.ts Show resolved Hide resolved
server/src/infra/entities/asset.entity.ts Outdated Show resolved Hide resolved
server/src/infra/entities/asset.entity.ts Outdated Show resolved Hide resolved
server/src/domain/asset/response-dto/asset-response.dto.ts Outdated Show resolved Hide resolved
server/src/infra/migrations/1690140002097-Panoramas.ts Outdated Show resolved Hide resolved
@dmitry-brazhenko
Copy link
Contributor Author

dmitry-brazhenko commented Jul 24, 2023

@brighteyed Hope I fixed all suggestions :)

@dmitry-brazhenko
Copy link
Contributor Author

Need help with creating documentation and merging the change

@dmitry-brazhenko
Copy link
Contributor Author

@jrasm91 I changed column to varchar

Hope it is ready to be merged now :)
@alextran1502 could you help to regenerate docs properly and merge the change?

Thanks

@dmitry-brazhenko
Copy link
Contributor Author

There is a problem with code prettiffier locally. Once I save file in visual studio, it applies some format that is not accepted by checker :(

@dmitry-brazhenko
Copy link
Contributor Author

Only one migration file is necessary
2023-07-27_17-47-42

server/package.json Show resolved Hide resolved
server/src/infra/entities/exif.entity.ts Outdated Show resolved Hide resolved
Comment on lines 340 to 351
const exifProjectionType = getExifProperty('ProjectionType');
let projectionType: ProjectionType = ProjectionType.NONE;

if (exifProjectionType) {
const exifPropertyUpper = exifProjectionType.toUpperCase();

if (exifPropertyUpper in ProjectionType) {
projectionType = exifPropertyUpper as ProjectionType;
}
}

newExif.projectionType = projectionType;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const exifProjectionType = getExifProperty('ProjectionType');
let projectionType: ProjectionType = ProjectionType.NONE;
if (exifProjectionType) {
const exifPropertyUpper = exifProjectionType.toUpperCase();
if (exifPropertyUpper in ProjectionType) {
projectionType = exifPropertyUpper as ProjectionType;
}
}
newExif.projectionType = projectionType;
const projectionType = getExifProperty('ProjectionType');
if (projectionType) {
newExif.projectionType = String(projectionType).toUpperCase();
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just store the value always, regardless if it matches the enum. We can update the enum in the future if we discover a new valid value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newExif.projectionType is a enum. What will happen in case String(projectionType).toUpperCase(); does not exist in this enum?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The database type is a varchar, right? The entity type should be ProjectionType | string.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we only accept valid values we should use pg enum. If we accept other values we should use a varchar and store the raw value (maybe transformed to uppercase). IMO the projection type is just a hint of what valid types are available to be used, but the value is just a string. I don't know if it is possible/easy to use enum in the web and flutter without getting an error for other values.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think everything would be quite a bit easier if you just stored it as a string everywhere in the server codebase, and had the property in the response dto be a string.

You can define the enum in the web to use for your if statements. Happy to discuss more in discord if you want to chat there instead of back and forth here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actualy i liked enum . I would suggested to keep enum | str. I changed it here: a06ee84

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to keep enum | str.

As far as I understood, there are just 2 most popular projection types:equirectangular and cylinder

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was some back and forth when it came to using a projection type enum server-side. In short, the server doesn't even use the value anywhere. The only place it is used currently is in the UI. Everything is a lot simpler if this is stored and treated as a string and the enum exists in the web for this specific panorama case. Javascript isn't typed and can load arbitrary values, but flutter is.

Copy link
Contributor

@brighteyed brighteyed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR doesn't check the aspect ratio anymore. @dmitry-brazhenko, please update the PR description

@alextran1502 alextran1502 merged commit e071b82 into immich-app:main Jul 28, 2023
@dmitry-brazhenko dmitry-brazhenko deleted the web_panoramas_2 branch July 28, 2023 06:12
@dmitry-brazhenko
Copy link
Contributor Author

🎉

@MingchenZhang
Copy link

Is there a way to add the property projectionType to post upload? I think there might be some issue with the metadata of my original panorama image that it is not showing up as panorama in immich. It would be nice to (force) show them as panorama in immich.

@dmitry-brazhenko
Copy link
Contributor Author

I would say this could make sense.

Most likely we could add it here:
2024-04-25_20-14-15

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 this pull request may close these issues.

7 participants