-
Notifications
You must be signed in to change notification settings - Fork 74
Use static blocks to calculate dynamic blocks #5237
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
base: main
Are you sure you want to change the base?
Conversation
| InterRegionPaddingBlock { | ||
| "assemblyName": undefined, | ||
| "end": undefined, | ||
| "key": "{test}ctgA:1..200-0-beforeFirstRegion", | ||
| "offsetPx": 0, | ||
| "refName": undefined, | ||
| "start": undefined, | ||
| "type": "InterRegionPaddingBlock", | ||
| "variant": "boundary", | ||
| "widthPx": -0, | ||
| }, |
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.
The new dynamic blocks calculation does not produce this (and one more below) 0-width block, which I don't think should change anything.
| "end": 72.4, | ||
| "isLeftEndOfDisplayedRegion": false, | ||
| "isRightEndOfDisplayedRegion": false, | ||
| "key": "{test}ctgA:27.05..72.4-0", |
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.
The keys can change a bit in the new dynamic blocks calculation, but they're still unique.
| const view = { ...baseView, offsetPx: 550, bpPerPx: 5 } | ||
| const dynamicBlocks = calculateVisibleRegions(view).getBlocks() | ||
| const staticBlocks = calculateStaticBlocks(view).getBlocks() | ||
| expect(dynamicBlocks.at(1)?.offsetPx).toEqual(staticBlocks.at(6)?.offsetPx) |
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.
These tests failed before the new dynamic blocks calculations, for example the dynamic block's offsetPx was 600 and the static block's offsetPx was 604, even though they're both for the same region that is fully contained on the screen.
|
this seems pretty reasonable. I am sometimes a little bit unsure even about including offsetPx in the static blocks because this makes them rapidly change, but that may be a separate issue |
|
Potentially weird thing with 'show all regions' on hg38: This branch
Main
Note that both of them are off when it is showing all regions, which I'm not sure if there is an issue for (#1771 is sort of similar long standing one but a bit different) this branch
main
|




I noticed when displaying multiple regions that sometimes the sequence track in Apollo seemed to be drawing the block boundaries in the wrong places:
After digging into it, I found that the
offsetPxvalues for the dynamic blocks could be wrong if there were multiple regions offscreen to the left. The first commit in this PR adds some tests that demonstrate this by showing that blocks that should have been shown on the same offset on the screen in both static blocks and dynamic blocks had differentoffsetPxs.I tried fixing the existing dynamic blocks calculation, but it was difficult because to line up the calculation of static and dynamic blocks, the dynamic blocks had to know how many static blocks were currently offscreen to the left. So I decided to try a completely different approach to calculating dynamic blocks: calculate the static blocks, then modify the calculated static blocks into dynamic blocks.
Here's the same track with the new dynamic blocks calculation:
This approach also has the advantage of not duplicating some complicated logic between static and dynamic block calculations.