Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

fix: camera jump not working #269

Merged
merged 10 commits into from Jul 11, 2022
Merged

fix: camera jump not working #269

merged 10 commits into from Jul 11, 2022

Conversation

airslice
Copy link
Contributor

@airslice airslice commented Jul 8, 2022

Overview

The jump button on camera pop-up pane doesn't work.

After some digging i found this bug comes from another PR which was aiming at fix the view flash.

  • I didn't realize the actual usage of some code and thought them only used to fix fly animation bug when using photooverlay as a patch - BUT actually this part of code has more basic task as accept global camera data change and update the Visualizer camera.
  • I added them back to fix the jump bug. And again facing the view flash issue.

About the view flash issue I think it comes from the useEffect of global state camera (with Jotai) is somehow optimized after upgrade to React18. Here's some description in detail:

Please correct me if i have anything misunderstood.

  • We have two camera related in this issue:

    • The global state camera with Jotai , which shares between organisms.
    • The cesium camera in Visualizer. which is the actual camera used for render map.
  • The data transfers in both way in different situations:

    • When moving the earth with Visualizer, the onChange (also onMoveEnd)will be fired and the camera data will be commit to the global one. This will make reearth know the camera update.
    • When you set camera from earth editor UI or anything that update the global camera, there is a useEffect in Visualizer and it will update the camera with engineAPI flyto and update it to right place (this is where jump works)
  • Considering these two things together: moving the earth -> Visualizer camera changed -> update global camera -> useEffect fired -> update Visualizer camera. This become a loop. So there is a condition in useEffect and also a cache of camera which called emittedCamera , it will store the last emit from Visualizer so when the useEffect fired it can check if the global camera is equal to Visualizer emitted one. And if so skip the flyto .

  • This solution works fine before as the state updates as we expected. But after upgrade to React18, the global state change become some async. eg:

    • before: | emit A | useEffect got A | emit B | useEffect got B | ...
    • after: | emit A | emit B; useEffect got A | emit C; useEffect got B | ...
    • You can notice the useEffect of the last(or more earlier) commit happens in the same update with later emit and lead to the former condition can't work as we expected. As a result the flyto fires with old camera state and make it a flash.
  • What i can see for now for solution:

    • Opt.1: Add a flag when moving camera by onMoveStart onMoveEnd and skip flyto if moving. This works good but if someone moving the camera and click jump before the moving animation ends the jump will also be ignored. ✅
    • Opt.2: Add flushSync to update camera function which will force a dom re-render and make the state sync again. This can work but i think the extra dom render means performance decrease. ⚠️
    • Opt.3: use emittedCamera.current ?? camera when flyto . This can ensure flyto called with the latest emitted value and avoid flash but will result in call flyto each update. This can also be a great harm to performance. 🚫
    • Opt.4: keep the idea of compare camera data to check if it's emitted from Visualizer only make the value into an array. The array length is an experience value base on how many data versions can it be of the global state behind in one update. This is not very reliable but good at most situations. This solution can keep all the behavior as designed and the cost seems acceptable. I think is better compare to others. ⭐

Since the data got from useEffect is no longer reliable, using one single data to do the check is not enough. Using opt.4 seems better otherwise we need to add another global state as flag and passing it everywhere which i think will make the code more complex.

In general, I currently implement this with option 4.

What I've done

  • Adding back the useEffect of camera to update Visualizer camera when global camera changes.
  • Change the emittedCamera into an array to cache recent emitted values as the camera got inside useEffect can be several versions lag behind now.

What I haven't done

How I tested

  • Move the camera (drag the earth around) to check if it flashes.
  • Click jump on camera pop-up pane (even when the moving camera animation havn't finish).
  • Change the input value of camrea pop-up pane (this should also update camera).
  • Drag the FOV slider to set the fov (it was also been effected).

Screenshot

Which point I want you to review particularly

  • Please correct me if i have anything misunderstood.

Memo

@netlify
Copy link

netlify bot commented Jul 8, 2022

Deploy Preview for reearth-web ready!

Name Link
🔨 Latest commit 9d07be3
🔍 Latest deploy log https://app.netlify.com/sites/reearth-web/deploys/62cbd19859b2390008ac7c04
😎 Deploy Preview https://deploy-preview-269--reearth-web.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@codecov
Copy link

codecov bot commented Jul 8, 2022

Codecov Report

Merging #269 (9d07be3) into main (5f7c69e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #269   +/-   ##
=======================================
  Coverage   52.19%   52.19%           
=======================================
  Files          56       56           
  Lines        1205     1205           
  Branches      193      193           
=======================================
  Hits          629      629           
  Misses        503      503           
  Partials       73       73           

@airslice airslice marked this pull request as ready for review July 9, 2022 10:21
@airslice airslice added the bug Something isn't working label Jul 9, 2022
@lavalse
Copy link
Member

lavalse commented Jul 9, 2022

I have tested. It seems to work fine.

airslice and others added 4 commits July 11, 2022 15:01
@airslice airslice requested a review from rot1024 July 11, 2022 07:17
Copy link
Member

@KaWaite KaWaite left a comment

Choose a reason for hiding this comment

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

OK!

Co-authored-by: KaWaite <34051327+KaWaite@users.noreply.github.com>
@airslice airslice merged commit 48bbfe9 into main Jul 11, 2022
@airslice airslice deleted the fix/camera-jump-not-working branch July 11, 2022 07:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
4 participants