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

Add floating labels to SVG features #2781

Merged
merged 47 commits into from
Apr 7, 2022
Merged

Conversation

cmdcolin
Copy link
Collaborator

@cmdcolin cmdcolin commented Mar 1, 2022

Possible method for adding floating labels to svg features

Tries to address (now closed) issue #175

This tries to implement floating labels in the renderer, and detects the view's offsetpx and such in the feature label code. An alternative approach could be adding them at the display level (similar to the y-scalebar is added to the linear wiggle display) but that is not considered here

Uses some methods from the #2671 canvas renderer, but canvas features is a complex and adding this to svg would benefit existing svg use cases

Could also be used to have floating arrowheads, reducing need for chevron display indicators on exons

Base branch: #2775

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Mar 1, 2022
@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Mar 1, 2022

currently, the labels start float into position after a scroll, so might need to pass the view state to the backend renderer to make it work before scroll

also not tuned for horizontally flipped yet, but that is also tricky because horizontally flipped actually renders to the feature labels on the right hand side of the feature

@cmdcolin cmdcolin added enhancement New feature or request and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Mar 1, 2022
Base automatically changed from new_svg_gene_gylph to main March 18, 2022 17:24
@cmdcolin cmdcolin force-pushed the new_svg_gene_gylph_floating_label branch from 454f8aa to cbdac1b Compare March 24, 2022 16:31
@cmdcolin
Copy link
Collaborator Author

This PR is shaping up to be closer to completion. It is not pixel perfect, sometimes some labels can be cut off, but it is actually working pretty good. It could be that we could consider merging before pixel perfect

Some key changes that have been made:

  • The FeatureLabel component is an observer for the view's offsetPx. Previously, a higher level component FeatureGlyph did this, but making the FeatureLabel reduces re-rendering of the entire featureglyph.
  • A "getViewParams" function helps instantiate this in 3 locations: 1) the serverSideRenderedBlock RPC call for first instantiation 2) inside the FeatureLabel as an observer 3) in BaseLinearDisplayModel for SVG export which is also an RPC call

some todos:
block boundaries can cut off the label, may need maxFeatureGlyphExpansion type thing

demo
https://jbrowse.org/code/jb2/new_svg_gene_gylph_floating_label/?config=test_data%2Fconfig_demo.json&session=share-LgNbcXVmxC&password=ZsvP1

note that this PR sort of further "leans into" SVG feature rendering...the canvas is somewhat difficult to deliver right, and we could probably do ok for ourselves if we keep doing SVG

@codecov
Copy link

codecov bot commented Mar 28, 2022

Codecov Report

Merging #2781 (bfce3e2) into main (f365543) will increase coverage by 0.02%.
The diff coverage is 91.83%.

❗ Current head bfce3e2 differs from pull request most recent head e7c2d78. Consider uploading reports for the commit e7c2d78 to get more accurate results

@@            Coverage Diff             @@
##             main    #2781      +/-   ##
==========================================
+ Coverage   59.69%   59.72%   +0.02%     
==========================================
  Files         591      591              
  Lines       26746    26757      +11     
  Branches     6510     6513       +3     
==========================================
+ Hits        15967    15981      +14     
+ Misses      10475    10472       -3     
  Partials      304      304              
Impacted Files Coverage Δ
...aseLinearDisplay/models/BaseLinearDisplayModel.tsx 86.45% <ø> (ø)
...aseLinearDisplay/models/serverSideRenderedBlock.ts 95.90% <ø> (ø)
...LinearGenomeView/components/OverviewRubberBand.tsx 25.51% <ø> (ø)
.../src/SvgFeatureRenderer/components/Subfeatures.tsx 85.71% <ø> (ø)
...FeatureRenderer/components/SvgFeatureRendering.tsx 72.63% <0.00%> (+0.75%) ⬆️
...gins/svg/src/SvgFeatureRenderer/components/util.ts 91.83% <ø> (ø)
...src/SvgFeatureRenderer/components/FeatureLabel.tsx 79.31% <88.46%> (+4.31%) ⬆️
packages/core/util/index.ts 85.47% <100.00%> (+0.28%) ⬆️
...linear-genome-view/src/LinearBasicDisplay/model.ts 65.21% <100.00%> (+1.58%) ⬆️
...gins/svg/src/SvgFeatureRenderer/components/Box.tsx 100.00% <100.00%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f365543...e7c2d78. Read the comment docs.

@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Apr 5, 2022

This is somewhat smooth now. It might be that this could be passable. Can try out here

Example link https://jbrowse.org/code/jb2/new_svg_gene_gylph_floating_label/?config=test_data%2Fconfig_demo.json&session=share-xTrvawByDA&password=rZ784

Continued challenges include horizontally flipped mode, the layout of the features and calculations are a bit different in horizontally flipped, but this PR pretends that it's the same and it works mostly ok

Reminder: this approach in the PR is based on the renderer, specifically making FeatureLabel an observable, and it has to deal with a mix of regionBp/featureBp/offsetPx/bpPerPx math. An alternative approach could be creating a new "layer" that is rendered using the display, similar to how the Y-scale bar is rendered on wiggle tracks, to do all this logic, and then it might be more in screen coordinates instead of offsetPx's, but it would move the "responsibility" away from the renderer and therefore might require a new display type

@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Apr 6, 2022

seemed like this would be ok to go in, despite some possible label collisions in the horizontally flipped mode with the labels

@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Apr 6, 2022

(just noting from what we said in meeting:))

@cmdcolin cmdcolin marked this pull request as ready for review April 6, 2022 23:10
@cmdcolin cmdcolin merged commit 9d38654 into main Apr 7, 2022
@cmdcolin cmdcolin deleted the new_svg_gene_gylph_floating_label branch April 7, 2022 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant