Skip to content
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

Merged
merged 13 commits into from
Jan 8, 2024

Conversation

etowahadams
Copy link
Contributor

@etowahadams etowahadams commented Dec 20, 2023

Fix #1006
Toward #

Change List

  • Stretches tiles instead of redrawing them if they they have stretchable marks (`'bar', 'line', 'rect', 'area') and is not a circular or 2D track.
  • Adds an experimental stretchTiles option. If true, all tiles will be stretched. If false, no tiles will be stretched. The threshold the tile can be stretched to can be tuned with stretchTilesThreshold option.

Checklist

  • Ensure the PR works with all demos on the online editor
  • Unit tests added or updated
  • Examples added or updated
  • Documentation updated (e.g., added API functions)
  • Screenshots for visual changes (e.g., new encoding support or UI change on Editor)

@etowahadams
Copy link
Contributor Author

Stretching individual shapes within a graphics object isn't allowed; you can only stretch the whole graphics object apparently. Perhaps another experiment to try would be to have all of the drawCircles etc be drawn to a Graphics object that is a child of tile.graphics, not to tile.graphics directly.

Copy link
Member

@sehilyi sehilyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, I think this is a great improvement! I left several comments, but otherwise the changes look good to me. Feel free to merge this to main after addressing them. I created an example on Editor to compare the original/new rendering options – please merge it to this PR if it looks good to you.

I will also test this with tracks in Chromoscope since I think this approach of stretching graphics works well with several tracks in Chromoscope.

src/tracks/gosling-track/gosling-track.ts Outdated Show resolved Hide resolved
import type * as PIXI from 'pixi.js';
import * as PIXI from 'pixi.js';
Copy link
Member

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?

Copy link
Contributor Author

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!

* 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remind me what it means that graphicsXScale is equal to 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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!

Comment on lines 1514 to 1516
(!Is2DTrack(this.getResolvedTracks()[0]) &&
this.options.spec.layout !== 'circular' &&
stretchableMarks.includes(this.options.spec.mark || ''))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we still enable testing the original rendering with stretchGraphics: false?

Suggested change
(!Is2DTrack(this.getResolvedTracks()[0]) &&
this.options.spec.layout !== 'circular' &&
stretchableMarks.includes(this.options.spec.mark || ''))
(typeof this.options.spec.experimental?.stretchGraphics == 'undefined' &&
!Is2DTrack(this.getResolvedTracks()[0]) &&
this.options.spec.layout !== 'circular' &&
stretchableMarks.includes(this.options.spec.mark || ''))

this.options.spec.experimental?.stretchGraphics ||
(!Is2DTrack(this.getResolvedTracks()[0]) &&
this.options.spec.layout !== 'circular' &&
stretchableMarks.includes(this.options.spec.mark || ''))
Copy link
Member

Choose a reason for hiding this comment

The 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', ...}
]

Example Editor

Screenshot 2023-12-23 at 2 57 38 PM

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))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that makes sense! I'll add that to hasStretchableGraphics

@etowahadams
Copy link
Contributor Author

These should resolve all the comments, thanks for the review. Will test the changes once the SSL cert is renewed to make sure everything looks good, and then will merge

@etowahadams
Copy link
Contributor Author

Caught an additional thing---we don't want stretching to occur by default then mouse interactions are enabled.

@etowahadams etowahadams merged commit 0b405ba into main Jan 8, 2024
5 checks passed
@etowahadams etowahadams deleted the etowahadams/stretch-tile branch January 8, 2024 16:23
@sehilyi
Copy link
Member

sehilyi commented Jan 8, 2024

Thanks for merging. I will bump the minor version of gosling.js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Performance: Stretch and translate instead of redraw
2 participants