-
Notifications
You must be signed in to change notification settings - Fork 12
Framework - Hover service #891
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
base: main
Are you sure you want to change the base?
Conversation
…ed to layer wrapper. Readout moved to Subsurface-wrapper be defined in layers-wrapper
…, intersection, and well-log
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.
Pull Request Overview
This pull request introduces a comprehensive hover synchronization system across the application through a new HoverService, refactoring hover interactions from the previous WorkbenchServices approach. The changes enable different modules (3D Viewer, 2D Viewer, Intersection, WellLogViewer) to publish and subscribe to hover events in a unified manner.
Key changes:
- Introduced
HoverServicewith throttled hover data updates and topic-based publish/subscribe pattern - Migrated from global
WorkbenchServiceshover topics to dedicatedHoverTopicenum - Refactored "SettingsGroup" to "ContextBoundary" terminology throughout the codebase
- Added wellbore trajectory interpolation utilities to support precise hover visualization at MD (Measured Depth) locations
Reviewed Changes
Copilot reviewed 39 out of 40 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/framework/HoverService.ts | New service implementing hover data management with throttling and topic-based subscriptions |
| frontend/src/framework/Workbench.ts | Integrated HoverService into the Workbench |
| frontend/src/modules/_shared/utils/wellbore.ts | Added trajectory interpolation functions and z-axis inversion support |
| frontend/src/modules/_shared/utils/subsurfaceViewerLayers.ts | New utility for extracting hover data from DeckGL picking information |
| frontend/src/modules/_shared/DataProviderFramework/visualization/VisualizationAssembler.ts | Updated to use new HoverTopic type and improved hover visualization function merging |
| frontend/src/modules/_shared/DataProviderFramework/framework/ContextBoundary/* | Renamed from SettingsGroup to ContextBoundary |
| frontend/src/modules/3DViewer/view/components/HoverVisualizationWrapper.tsx | Implements hover visualization handling for 3D viewer |
| frontend/src/modules/2DViewer/view/components/HoverVisualizationWrapper.tsx | Implements hover visualization handling and crosshair layer for 2D viewer |
| frontend/src/modules/Intersection/view/components/ReadoutWrapper.tsx | Migrated to use HoverService instead of WorkbenchServices |
| frontend/src/modules/WellLogViewer/view/components/SubsurfaceLogViewerWrapper.tsx | Migrated to use HoverService for MD synchronization |
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
frontend/src/modules/_shared/DataProviderFramework/visualization/VisualizationAssembler.ts
Show resolved
Hide resolved
...shared/DataProviderFramework/visualization/hooks/useSubscribedProviderHoverVisualizations.ts
Show resolved
Hide resolved
frontend/src/modules/3DViewer/view/components/DataProvidersWrapper.tsx
Outdated
Show resolved
Hide resolved
frontend/src/modules/2DViewer/view/components/SubsurfaceViewerWrapper.tsx
Outdated
Show resolved
Hide resolved
frontend/src/modules/2DViewer/view/components/HoverVisualizationWrapper.tsx
Outdated
Show resolved
Hide resolved
frontend/src/modules/WellLogViewer/view/components/SubsurfaceLogViewerWrapper.tsx
Show resolved
Hide resolved
jorgenherje
left a comment
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.
Really nice work! Well structured and easy to read code! Descriptive and easy to understand.
Some minor comments from my side.
| ZONE = "hover.zone", | ||
| REGION = "hover.region", | ||
| FACIES = "hover.facies", | ||
| WORLD_POS = "hover.world_pos", |
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 would be good to clarify which type off coordinates we expect - e.g. utm-coordinates (i assume). Either do it in the naming of hover topic, or in the data below, i.e. x renamed to xUtm, and same for y. z is usually without suffix utm i believe.
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.
Went with WORLD_POS_UTM, I want to keep the {x, y, z} structure so it's easier to use in Vec2/Vec3 utils
frontend/src/modules/_shared/DataProviderFramework/visualization/VisualizationAssembler.ts
Show resolved
Hide resolved
frontend/src/modules/_shared/DataProviderFramework/visualization/VisualizationAssembler.ts
Show resolved
Hide resolved
frontend/src/modules/_shared/DataProviderFramework/visualization/VisualizationAssembler.ts
Outdated
Show resolved
Hide resolved
jorgenherje
left a comment
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.
Just two small comments, then all good :)
Replaces the old synced hover with a dedicated hover manager service, as well as some state-like hooks for using it. The general idea is that all hover effects should go through this service in the future.
For this initial release, the hover-service has been implemented for the following modules: