-
Notifications
You must be signed in to change notification settings - Fork 24
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
perf(core): stretch certain marks instead of redraw #1018
Changes from 2 commits
8137e5a
bffddc9
c5ebfa5
1e7622a
a3236dd
3b9aeb2
cf3fdd9
8e46cc5
15bcb31
33156c5
eabbb16
4a167a6
965f8cc
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 | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,4 +1,4 @@ | ||||||||||||||||
import type * as PIXI from 'pixi.js'; | ||||||||||||||||
import * as PIXI from 'pixi.js'; | ||||||||||||||||
import { isEqual, sampleSize, uniqBy } from 'lodash-es'; | ||||||||||||||||
import type { ScaleLinear } from 'd3-scale'; | ||||||||||||||||
import type { | ||||||||||||||||
|
@@ -333,14 +333,43 @@ const factory: PluginTrackFactory<Tile, GoslingTrackOptions> = (HGC, context, op | |||||||||||||||
override drawTile(tile: Tile) { | ||||||||||||||||
if (PRINT_RENDERING_CYCLE) console.warn('drawTile(tile)'); | ||||||||||||||||
|
||||||||||||||||
tile.drawnAtScale = this._xScale.copy(); // being used in `super.draw()` | ||||||||||||||||
|
||||||||||||||||
/** | ||||||||||||||||
* If we don't have info about the tile, we can't draw anything. | ||||||||||||||||
*/ | ||||||||||||||||
const tileInfo = this.#processedTileInfo[tile.tileId]; | ||||||||||||||||
if (!tileInfo) { | ||||||||||||||||
// We do not have a track model prepared to visualize | ||||||||||||||||
return; | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
/** | ||||||||||||||||
* Add a copy of the track scale to the tile. The tile needs its own scale because we will use it to | ||||||||||||||||
* determine how much the tile has been stretched (if we are stretching the graphics) | ||||||||||||||||
*/ | ||||||||||||||||
if (!tile.drawnAtScale) { | ||||||||||||||||
// This is the first time this tile is being drawn | ||||||||||||||||
tile.drawnAtScale = this._xScale.copy(); | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
/** | ||||||||||||||||
* For certain types of marks and layouts (linear), we can stretch the graphics to avoid redrawing | ||||||||||||||||
* This is much more performant than redrawing everything at every frame | ||||||||||||||||
*/ | ||||||||||||||||
const [graphicsXScale, graphicsXPos] = this.getXScaleAndOffset(tile.drawnAtScale); | ||||||||||||||||
if (this.#hasStretchableGraphics() && !this.#isTooStretched(graphicsXScale) && graphicsXScale !== 1) { | ||||||||||||||||
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. Could you remind me what it means that 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. On first render, the graphicsXScale is equal to 1. We don't to stretch because nothing has been drawn yet. I just added a variable to better reflect this. Hopefully it is more clear now! |
||||||||||||||||
// Stretch the graphics | ||||||||||||||||
tile.graphics.scale.x = graphicsXScale; | ||||||||||||||||
tile.graphics.position.x = graphicsXPos; | ||||||||||||||||
return; | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
/** | ||||||||||||||||
* If we can't stretch the graphics, we need to redraw everything! | ||||||||||||||||
*/ | ||||||||||||||||
|
||||||||||||||||
// We need the tile scale to match the scale of the track | ||||||||||||||||
tile.drawnAtScale = this._xScale.copy(); | ||||||||||||||||
// Clear the graphics and redraw everything | ||||||||||||||||
tile.graphics?.clear(); | ||||||||||||||||
tile.graphics?.removeChildren(); | ||||||||||||||||
|
||||||||||||||||
|
@@ -1472,6 +1501,31 @@ const factory: PluginTrackFactory<Tile, GoslingTrackOptions> = (HGC, context, op | |||||||||||||||
} | ||||||||||||||||
}); | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
/** | ||||||||||||||||
* Used in drawTile() | ||||||||||||||||
* Checks if the track has marks which are stretchable. Stretching | ||||||||||||||||
* is not supported for circular layouts or 2D tracks | ||||||||||||||||
*/ | ||||||||||||||||
#hasStretchableGraphics() { | ||||||||||||||||
const stretchableMarks = ['bar', 'line', 'rect', 'area']; | ||||||||||||||||
return ( | ||||||||||||||||
this.options.spec.experimental?.stretchGraphics || | ||||||||||||||||
(!Is2DTrack(this.getResolvedTracks()[0]) && | ||||||||||||||||
this.options.spec.layout !== 'circular' && | ||||||||||||||||
stretchableMarks.includes(this.options.spec.mark || '')) | ||||||||||||||||
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. Could we still enable testing the original rendering with
Suggested change
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. It is possible that a track that superposes multiple sub-tracks can have different mark types defined by the sub-tracks (e.g., rect, text, and triangles in ideograms). So, in some cases, it may end up using the new rendering method even though there are un-stretchable marks being used by sub-tracks. e.g., alignment: 'overlay',
mark: 'rect',
tracks: [
{ ... },
{ mark: 'text', ...}
] So, we will need to check all the marks used in the sub-tracks as well, e.g., this.getResolvedTracks().reduce((acc, spec) => acc && stretchableMarks.includes(spec.mark), true)) 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. I see that makes sense! I'll add that to |
||||||||||||||||
); | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
/** | ||||||||||||||||
* Used in drawTile() | ||||||||||||||||
* Checks if the tile Graphic is too stretched. If so, it returns true. | ||||||||||||||||
* @param stretchFactor The factor by which the tile is stretched | ||||||||||||||||
* @returns True if the tile is too stretched, false otherwise | ||||||||||||||||
*/ | ||||||||||||||||
#isTooStretched(stretchFactor: number) { | ||||||||||||||||
return stretchFactor > 2 || stretchFactor < 0.5; | ||||||||||||||||
etowahadams marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
return new GoslingTrackClass(); | ||||||||||||||||
}; | ||||||||||||||||
|
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.
Is the removal of
type
intended here?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.
Ah this is because I was experimenting with some other Pixi stuff before. I'll add it back. Good catch!