-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: attach CPU timing metadata to render event #7768
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -186,8 +186,11 @@ export type MapEventType = { | |
| * - a change to the map's style | ||
| * - a change to a GeoJSON source | ||
| * - the loading of a vector tile, GeoJSON file, glyph, or sprite | ||
| * | ||
| * The event carries a `timing` object with high-resolution CPU-side timing | ||
| * data; see {@link MapLibreRenderEvent}. | ||
| */ | ||
| render: MapLibreEvent; | ||
| render: MapLibreRenderEvent; | ||
| /** | ||
| * Fired immediately after the map has been resized. | ||
| */ | ||
|
|
@@ -440,6 +443,40 @@ export type MapLibreEvent<TOrig = unknown> = { | |
| originalEvent: TOrig; | ||
| }; | ||
|
|
||
| /** | ||
| * Timing metadata attached to the `render` event. All timestamps are in | ||
| * `performance.now()`-space (DOMHighResTimeStamp, milliseconds since the | ||
| * page navigation timeline origin). | ||
| * | ||
| * Note: these are CPU-side timestamps. WebGL exposes no portable signal for | ||
| * GPU buffer-swap completion, so callers needing true frame readiness for | ||
| * capture/export should additionally listen for `viewporttilesloaded` (when | ||
| * available) or sample `canvas.captureStream(0)`. | ||
| * | ||
| * @group Event Related | ||
| */ | ||
| export type MapLibreRenderTiming = { | ||
| /** When `painter.render()` was entered. */ | ||
| start: number; | ||
| /** | ||
| * When `painter.render()` returned and all WebGL commands for the frame | ||
| * were submitted to the driver. Equivalent to `start + duration`. | ||
| */ | ||
| end: number; | ||
| /** Wall-clock time spent inside `painter.render()`, in milliseconds. */ | ||
| duration: number; | ||
| }; | ||
|
|
||
| /** | ||
| * The render event. Carries CPU-side timing information for the just-completed | ||
| * frame (see {@link MapLibreRenderTiming}). | ||
| * | ||
| * @group Event Related | ||
| */ | ||
| export type MapLibreRenderEvent = MapLibreEvent & { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Man.. this is a mess, I now see that some events are classes and some are types, I'm not sure I even know why...
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the benefit of a class over a nested object?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can |
||
| timing: MapLibreRenderTiming; | ||
| }; | ||
|
|
||
| /** | ||
| * The style data event | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -89,3 +89,18 @@ test('redraw', async () => { | |
| map.redraw(); | ||
| await renderPromise; | ||
| }); | ||
|
|
||
| test('render event carries timing metadata', async () => { | ||
| const map = createMap(); | ||
| await map.once('idle'); | ||
|
|
||
| const renderPromise = map.once('render'); | ||
| map.redraw(); | ||
| const event = await renderPromise; | ||
|
|
||
| expect(event.timing).toBeDefined(); | ||
| expect(event.timing.end).toBeGreaterThanOrEqual(event.timing.start); | ||
| expect(event.timing.duration).toBeCloseTo(event.timing.end - event.timing.start, 5); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this check survive CI flackiness? |
||
| expect(event.timing.duration).toBeGreaterThanOrEqual(0); | ||
| expect(event.timing.duration).toBeLessThan(1000); | ||
| }); | ||
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.
As you said, I think start/end/duration is better