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

fix: camera state when hidden page is rendered #1819

Merged
merged 13 commits into from
Dec 8, 2023
Merged

Conversation

w1nklr
Copy link
Collaborator

@w1nklr w1nklr commented Dec 7, 2023

Fix for #1820

Add formal size state and effect to retrieve size from deck ref for code clarity
Move global bounding box handling to React reducer instead of useEffect.
Move camera computation to useMemo instead of useEffect.
Memoize viewPortMargins and deckGLLayers

Add a new story displaying the subsurface viewer in tabs to reproduce the issue

Note that this is a first step to reduce the number of useEffect in the Map component

The story has a flag controlling the rendering of hidden tabs.

Add triggerHome control to mapLayer and SubsurfaceViewer stories.

Minor props re-ordering to group camera controls
React rule is not to read/modify ref during rendering.
It proved true: computations where randomly wrong until the size of the deck was extracted in the effect.
This avoids sequences of useEffect calling setState...
@w1nklr w1nklr changed the title Fix camera state when hidden page is rendered fix: camera state when hidden page is rendered Dec 7, 2023
@hkfb
Copy link
Collaborator

hkfb commented Dec 7, 2023

Note that this is a first step to reduce the number of useEffect in the Map component

This is just refactoring and creating an example to demonstrate the bug?

@w1nklr w1nklr requested review from nilscb and hkfb December 7, 2023 15:03
@w1nklr
Copy link
Collaborator Author

w1nklr commented Dec 7, 2023

Note that this is a first step to reduce the number of useEffect in the Map component

This is just refactoring and creating an example to demonstrate the bug?

Nope, it fixes also the bug (ie. the example was showing the bug before I fixed it in the same PR)

Copy link
Collaborator

@hkfb hkfb left a comment

Choose a reason for hiding this comment

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

I get this error in the console:
image

Copy link
Collaborator

@hkfb hkfb left a comment

Choose a reason for hiding this comment

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

The second tab seems wrongly initialized the first time it's opened:
image
The second time I open it I can see the map.

@w1nklr
Copy link
Collaborator Author

w1nklr commented Dec 7, 2023

Warning message fixed.

@w1nklr
Copy link
Collaborator Author

w1nklr commented Dec 7, 2023

The second tab seems wrongly initialized the first time it's opened: image The second time I open it I can see the map.

:/
How are you doing ?
What is your browser ??

@w1nklr w1nklr merged commit 422325e into equinor:master Dec 8, 2023
8 checks passed
hkfb pushed a commit that referenced this pull request Dec 8, 2023
@hkfb
Copy link
Collaborator

hkfb commented Dec 8, 2023

🎉 This issue has been resolved in version subsurface-viewer@0.9.2 🎉

The release is available on GitHub release

@hkfb hkfb added the released label Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants