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

Difference in sphere projection #1406

Closed
PanierAvide opened this issue Aug 7, 2024 · 18 comments
Closed

Difference in sphere projection #1406

PanierAvide opened this issue Aug 7, 2024 · 18 comments
Labels
Milestone

Comments

@PanierAvide
Copy link

PanierAvide commented Aug 7, 2024

Describe the bug

Hello,

I'm working with some images with strong XMP correction, it seems that the way they are projected on PSV sphere doesn't fully compensate shifts (comparison with Pannellum, same image) :

image

This happens with either XMP metadata read from image, as well as setting manually sphereCorrection values.

Online demo URL

https://files.pavie.info/clients/panoramax/psv_xmp/

Photo Sphere Viewer version

5.9.0

Plugins loaded

None

OS & browser

Linux, latest Firefox & Chromium

Additional context

Started working on this topic due to an issue started here : https://gitlab.com/panoramax/clients/web-viewer/-/issues/148

Part of it is my fault (inverted sphereCorrection values), but even with this fix, I wasn't able to have something satisfying. As suggested by the person who reported the issue, I tried with Pannellum and was surprised by the rendering difference.

If that's of any help, examples images I used.

@PanierAvide PanierAvide added the bug label Aug 7, 2024
@mistic100
Copy link
Owner

I don't know what I can do.

The angles are applied to the mesh here https://github.com/mistic100/Photo-Sphere-Viewer/blob/main/packages/core/src/services/Renderer.ts#L269-L275

I tried every permutation of the last parameter and only ZXY displays something relatively correct.

@PanierAvide
Copy link
Author

I have no clue how all of this works, could it be related to sphere shape simplification ? Its position in 3D space or its scale ?

@mistic100
Copy link
Owner

I don't think so, the sphere and the camera are both positionned at 0,0,0 so only the rotations should matter.

@mistic100
Copy link
Owner

Try this instead, I get the same result as pannelum (excluding the heading, which pannelum seems to ignore ?)

mesh.rotation.set(cleanCorrection.tilt, cleanCorrection.pan, cleanCorrection.roll, 'YXZ');

it seems my mistake was to try to apply the inverse transformation

before:

mesh.rotation.set(-cleanCorrection.tilt, -cleanCorrection.pan, -cleanCorrection.roll, 'ZXY');

@PanierAvide
Copy link
Author

Tried it locally and this is far better 😁 This may also apply to setSphereCorrection for manual correction. Thanks for finding out the issue 👏

@mistic100
Copy link
Owner

I don't think changing setSphereCorrection is good idea, it would be a big breaking change for people having fine tuned their angles (as Google doc says, Euler angles are not permutable).

If setPanoramaPose is inherently wrong on the other hand, it is ok to change it.

@PanierAvide
Copy link
Author

I understand the need for retro-compatibility. I was asking this for setSphereCorrection because we have on our API XMP data that is stored outside the picture itself (in case of CSV correlation, manual correction afterwards, or on other EXIF/XMP tags not read usually). Could we imagine a way to have a similar angle correction as setPanoramaPose passed through Virtual Tour nodes ? Or maybe an init parameter saying if we prefer setSphereCorrection old behaviour or new one ?

@mistic100
Copy link
Owner

It should not matter isn't it ? If your configuration page is made for the current angles order used in sphereCorrection, every correction is still possible, this is independant of panoramaPose.

@PanierAvide
Copy link
Author

This would mean those corrected angles only work with PSV, not other third-party software that rely on XMP correction angles. The straightforward solution would be to put XMP angles in-file directly, but we don't do it to avoid too much disk RW operations and lose performance. That's why we'd like to stick with a kinda-like sphereCorrection on-the-fly way to define these angles, but in XMP values range.

If that's not possible, that's okay, I already have some patches on our side over PSV, and as you find out the correct code to overcome this projection issue, I can easily patch setSphereCorrection in our viewer (the magic of functional JS) 😁

@mistic100
Copy link
Owner

I don't understand how this is a problem as sphereCorrection is specific to PSV, and as you said you don't put the data back in the file itself. But of course I don't know all your integration details.

If your use case is "embedded XMP" or manual settings, you can set panoData in the virtual tour nodes configuration (with only the pose parameters), this way it will be handled by setPanoramaPose instead.

@PanierAvide
Copy link
Author

We don't integrate back in file yet, but could be something we do on-demand later. That will be handy when we will actually have a web UI for editing those angles.

Are you talking about poseRoll/poseHeading/posePitch config ? I tried that at some point of my investigations, it didn't work and reading docs made me understand that they are not used with equirectangular tiles adapter. Maybe I missed something, I can look again to try that 😅

@mistic100
Copy link
Owner

They are, for the base image only. I removed it from the doc because there was a big misunderstanding between that and sphereCorrection.

@mistic100
Copy link
Owner

Having two differents behaviour is bad though, so let's go for an option to switch between old and new. But I have no idea how to handle the migration. Obviously most people won't read the deprecation warning in the release note and it will break at once.

@PanierAvide
Copy link
Author

If you keep old fashion as default, this will not cause migration issues. And a good bit of docs will help make it clear that you may actually want to switch to new fashion 😁

@mistic100
Copy link
Owner

That means I will never be able to remove the old behaviour, and I don't want that.

@PanierAvide
Copy link
Author

So a minor release + warning in console if function is used ? I mean that's why semantic versioning exist, if users update without testing before release, that's a shame but not your fault 😅

@mistic100 mistic100 added this to the 5.9.1 milestone Aug 8, 2024
@mistic100
Copy link
Owner

Migration plan, inspired by ThreeJS :

  • 5.9.1 uses previous behaviour
    A warning is displayed in the console if at least two angles are provided to XMP/panoData or sphereCorrection (with only one angle, the change has no effect)
    The global flag Viewer.useNewAnglesOrder allows to switch to the new behaviour
  • 5.11.0 sets useNewAnglesOrder to true by default and the new behaviour is enabled by default
    A warning is displayed if someone sets it back to false
  • 5.13.0 the flag is removed and only the new behaviour is available

Copy link

github-actions bot commented Sep 7, 2024

This feature/bug fix has been released in version 5.10.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants