feat(core): Enhance onAfterRender with pass parameter and add hasActiveTransitions#10361
feat(core): Enhance onAfterRender with pass parameter and add hasActiveTransitions#10361akre54 wants to merge 1 commit into
Conversation
chrisgervang
left a comment
There was a problem hiding this comment.
Let's see if we can get the same end result without adding another render callback to the core API. I think there's additive way to get the same end result by enhancing onAfterRender with pass and adding deck.hasActiveTransitions
| gpuTime: number | null; | ||
| gpuTimingSupported: boolean; | ||
| timestamp: number; | ||
| layersRendered: number; |
There was a problem hiding this comment.
Wouldn't a user have this trivially available in the callback? deck.props.layers.length
|
|
||
| // Fire onFrameComplete only for actual on-screen draws so picking/texture | ||
| // passes don't pollute the timing stream consumed by capture pipelines. | ||
| if (opts.pass === 'screen' && this.props.onFrameComplete !== noop) { |
There was a problem hiding this comment.
What if instead of adding a new API we passed opts.pass through onAfterRender?
| const animationsInProgress = | ||
| Boolean(this.layerManager?.hasActiveTransitions()) || | ||
| Boolean(this.viewManager?.hasActiveTransitions()); |
There was a problem hiding this comment.
We could add a top level deck.hasActiveTransitions(), add pass to onAfterRender, and then an application can implement this on their own.
This pattern could be shared in documentation in tips and tricks
| * Useful for performance instrumentation, frame pacing, and headless capture | ||
| * pipelines that need to know when the GPU is finished with a frame and | ||
| * whether any animations would change the next frame. |
There was a problem hiding this comment.
Can an application monitor GPU metrics timings on their own?
See getStats
|
@chrisgervang - Great point about API proliferation. Your suggestion to enhance existing APIs is cleaner than adding a new callback. Revised approach:
For GPU timing, I'll check whether Re: If this can all be achieved by enhancing existing APIs + documentation rather than adding a new callback, that's definitely better. Let me revise the approach and come back with an enhancement proposal instead. |
Enhances existing APIs instead of adding new callbacks, reducing API
proliferation while providing the same functionality.
Changes:
--------
1. onAfterRender callback now includes 'pass' parameter
- Distinguishes 'screen' from 'picking' and other render passes
- Enables efficient frame capture (skip non-screen renders)
2. New deck.hasActiveTransitions() method
- Checks for active viewport transitions
- Checks for active layer uniform transitions
- Returns true if any animations are in progress
Use Cases:
----------
- Video export: Wait for transitions before capturing frames
- Screenshots: Ensure scene is settled before capture
- Testing: Verify animations complete
Example:
--------
deck.setProps({
onAfterRender: ({pass}) => {
if (pass === 'screen' && !deck.hasActiveTransitions()) {
const allLoaded = deck.props.layers.every(l => l.isLoaded);
if (allLoaded) {
captureFrame();
}
}
}
});
Benefits over new callback:
---------------------------
- No new API surface (enhances existing onAfterRender)
- Composable with existing patterns
- Clear separation of concerns (pass detection vs transition detection)
- Backward compatible (pass parameter is new but optional)
Testing:
--------
- Unit tests for onAfterRender pass parameter
- Unit tests for hasActiveTransitions() with layer transitions
- Documentation with complete examples
0b51ee8 to
3d7d186
Compare
Summary
Enhances existing APIs instead of adding a new callback, reducing API proliferation while providing the same functionality.
Changes
1. onAfterRender Enhancement
Added
passparameter to distinguish between render pass types:2. New hasActiveTransitions() Method
Checks for active viewport and layer uniform transitions:
Why This Approach
Per @chrisgervang's feedback, enhancing existing APIs is better than adding new callbacks:
Use Cases
Video Export
Screenshots
Comparison to Original PR
Original (new callback):
onFrameCompletecallbackdeck.props.layers.length)New Approach (enhance existing):
onAfterRenderwithpassparameterhasActiveTransitions()methodTesting
onAfterRenderpass parameterhasActiveTransitions()with layer transitionshasActiveTransitions()with viewport transitionsRelated
This addresses the feedback from the original PR discussion about reducing API proliferation. Instead of a new callback, we enhance what exists.