fix: Component for displaying performance metrics#2535
fix: Component for displaying performance metrics#2535nilscb wants to merge 8 commits intoequinor:masterfrom
Conversation
nilscb
added
enhancement
w1nklr
left a comment
w1nklr
left a comment
There was a problem hiding this comment.
I (with my current react knowledge) am wondering about this usage of ref/imperative handle.
I don't know all the benefits/drawbacks, but I feel it's a bit overkill for the need.
Curious, I checked and currently found only 1 (resp. 2) usage of forwardRef (resp. useImperativeHandle) in Deck.gl repo, 0 (resp. 1) in webviz and about 15 (I did not count duplicated code) (resp. 0) in the whole NGR code.
|
The reason for using forward ref etc is that I want only the metrics component to redraw when a new metrics update occurs. Alternatively it could be a state in the story component and the metrics info could be sent to the metrics component trough a property. However that would cause also the deck component to redraw which is undesirable. But this rest on my understanding that if a react components state is changed then all its children will also be redrawn, I could be wrong here (Håvard disagrees I think) I don't have any strong feelings about it but redrawing deck may alter its statistics which is the very thing we want to measure. |
|
Here a snapshot: |
|
|
||
| /** | ||
| * Callback called from deck.gl whith metrics data. | ||
| */ | ||
| onMetrics?: ((m: DeckMetrics) => void) | null; |
There was a problem hiding this comment.
Not needed, since MapProps is already included in the type definition.
| lights, | ||
| children, | ||
| verticalScale, | ||
| onMetrics, |
There was a problem hiding this comment.
Probably not needed, should be included in ...args
| triggerResetMultipleWells={triggerResetMultipleWells} | ||
| lights={lights} | ||
| verticalScale={verticalScale} | ||
| onMetrics={onMetrics ?? undefined} |
There was a problem hiding this comment.
Probably not needed, should be included in ...args
| /** | ||
| * Callback called from deck.gl whith metrics data. | ||
| */ | ||
| onMetrics?: ((m: DeckMetrics) => void) | null; |
There was a problem hiding this comment.
Suggest using something like DeckGL["_onMetrics"], so that we don't need to duplicate the type
| const metricsComponentRef = React.useRef<{ | ||
| updateMetrics?: (m: DeckMetrics) => void; | ||
| } | null>(null); | ||
| const updateMetrics = (m: DeckMetrics) => { | ||
| if (!metricsComponentRef.current) { | ||
| return; | ||
| } | ||
| if ("updateMetrics" in metricsComponentRef.current) { | ||
| metricsComponentRef.current.updateMetrics?.(m); | ||
| } | ||
| }; | ||
|
|
||
| const onMetrics = (m: DeckMetrics) => { | ||
| updateMetrics(m); | ||
| }; |
There was a problem hiding this comment.
Suggest creating an exported custom React hook that returns
- a state that contains the metrics
- a callback that updates the state, used as the
onMetricsargument
I think it will be a more versatile and easier to use approach. I agree with @w1nklr that the ref shouldn't be necessary. I think it's at least worth a try.
3 participants
Added a component to show metrics from deck.gl (like frames pr sec etc)
Demonstrated in story Map/BigMap3D