search

GMOD / jbrowse-components

Source code for JBrowse 2, a modern React-based genome browser
https://jbrowse.org/jb2
Apache License 2.0
209 stars 62 forks source link

Reduce re-rendering on quantitative and snpcoverage track height adjustments (pt. 2) #4654

Closed cmdcolin closed 1 week ago

cmdcolin commented 1 week ago

Part of https://github.com/GMOD/jbrowse-components/pull/4652 had to be reverted due to hanging the track

This restores the functionality

Previously, what we would see a lot with quantitative tracks when you e.g. zoom in is

a) you have track rendered b) you hit zoom in button c) it would render the track using the stats that are preexisting (e.g. from the zoomed out state) d) the stats would update for the new zoom in level e) then it would re-render again

for the user, this would appear sort of like weird flash where the track would render, blank, and then rerender with the new stats

this PR makes it so that it gets the stats for the visible region before rendering

one risk is that the user gets a slightly 'slower' response because instead of rendering with the old stats 'instantly' it has to 'wait for the new stats before rendering'

loading states do make you lose focus a little

however, by removing the render_with_old_stats->blank_screen->rerender_with_new_stats effect, I think users can focus better on the result, and this is especially true for wiggle renderings which take a substantial time (e.g. large BAM/CRAM SNPCov) since it only renders once instead of twice

In order to accomplish this PR, I make a getter called "quantitativeStatsUpToDate" that decides whether the stats are up to date if current view.dynamicBlocks exactly matches a stringified version of the dynamicBlocks stored on the stats. Note that this string could get long if there are many regions being displayed,

cmdcolin commented 1 week ago

note also: I think that there could be optimizations to the getQuantitativeStats procedure. for example, currently it counts all e.g. bam/cram mismatches because it just runs getFeatures on the SNPCov adapter which does the normal coverage bin calculation, but there could be an optimized pipeline which does not include mismatches in the calculation, and/or the SNPCoverageAdapter could cache results perhaps

cmdcolin commented 1 week ago

there is an weird typescript bug (??) where the .d.ts files that are generated are badly formatted and sensitive to small changes to our source code, particularly the #property/#getter/etc docstrings. these can trigger the .d.ts files to suddenly not work. I noticed this once before but it seems to have happened again

https://github.com/GMOD/jbrowse-components/actions/runs/11842042037/job/32999727258?pr=4654#step:5:823

CI log looks like

Error: ../../plugins/wiggle/esm/shared/SharedWiggleMixin.d.ts(196,9): error TS1005: '>' expected.
Error: ../../plugins/wiggle/esm/shared/SharedWiggleMixin.d.ts(201,11): error TS1109: Expression expected.
Error: ../../plugins/wiggle/esm/shared/SharedWiggleMixin.d.ts(202,37): error TS1005: ',' expected.
Error: ../../plugins/wiggle/esm/shared/SharedWiggleMixin.d.ts(203,27): error TS1005: ',' expected.
Error: ../../plugins/wiggle/esm/shared/SharedWiggleMixin.d.ts(204,40): error TS1005: ',' expected.
Error: ../../plugins/wiggle/esm/shared/SharedWiggleMixin.d.ts(214,6): error TS1109: Expression expected.
Error: ../../plugins/wiggle/esm/shared/SharedWiggleMixin.d.ts(214,8): error TS1109: Expression expected.
Error: ../../plugins/wiggle/esm/shared/SharedWiggleMixin.d.ts(215,5): error TS1128: Declaration or statement expected.
Error: ../../plugins/wiggle/esm/shared/SharedWiggleMixin.d.ts(216,5): error TS1128: Declaration or statement expected.
Error: ../../plugins/wiggle/esm/shared/SharedWiggleMixin.d.ts(217,18): error TS1005: ';' expected.
Error: ../../plugins/wiggle/esm/shared/SharedWiggleMixin.d.ts(218,5): error TS1128: Declaration or statement expected.
Error: ../../plugins/wiggle/esm/shared/SharedWiggleMixin.d.ts(219,5): error TS1128: Declaration or statement expected.
Error: ../../plugins/wiggle/esm/shared/SharedWiggleMixin.d.ts(220,21): error TS1005: ';' expected.
Error: ../../plugins/wiggle/esm/shared/SharedWiggleMixin.d.ts(220,59): error TS1011: An element access expression should take an argument.
Error: ../../plugins/wiggle/esm/shared/SharedWiggleMixin.d.ts(221,5): error TS1128: Declaration or statement expected.
Error: ../../plugins/wiggle/esm/shared/SharedWiggleMixin.d.ts(221,67): error TS1011: An element access expression should take an argument.
Error: ../../plugins/wiggle/esm/shared/SharedWiggleMixin.d.ts(222,29): error TS1005: ';' expected.
Error: ../../plugins/wiggle/esm/shared/SharedWiggleMixin.d.ts(223,1): error TS1128: Declaration or statement expected.
Error: ../../plugins/wiggle/esm/shared/SharedWiggleMixin.d.ts(223,3): error TS1109: Expression expected.
Error: ../../plugins/wiggle/esm/shared/SharedWiggleMixin.d.ts(225,5): error TS1005: ',' expected.

inspecting the .d.ts around the lines indicated shows the chaos, where a #property comment is badly formatted. if I get the nerve up, could submit to typescript repo but can code around it by adding more contents to a docstring for now

cmdcolin commented 1 week ago

example of the erroneous generated .d.ts (screenshot)

image

cmdcolin commented 1 week ago

note that DisplayBlurb is not a "#property" so those docstrings are getting pulled in from somewhere crazy

cmdcolin commented 1 week ago

actually this needed reverting as well. even though it seems like a good idea, and the stuttering is bad and duplicates work, it's almost worse with this change because there is a delay due to loading time which is aggravated by the quantitative stats being on a 1s delay, so you get an added 1s delay where only "Loading..." is displayed on any zoom in or out. the static blocks tracks are only renderDelay:50ms by default so if the rendering is fast, it happens more or less instantly and then only double renders a second later when the quantitative stats autorun is run.

if there was a way to remove the 1s delay, and precede the rendering, it might be able to get in a later release