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

fix(web): center markers in sequence views #709

Merged
merged 1 commit into from
Jan 31, 2022

Conversation

ivan-aksamentov
Copy link
Member

@ivan-aksamentov ivan-aksamentov commented Jan 27, 2022

Resolves #631 #708
Followup of #638 #644

Continues on the adventure of centering various markers in sequence views, started in #638 and #644.

The goal is to make the different markers to be aligned to their positions in the sequence properly, as well as to be aligned between different other types of markers. The challenge is that there's not enough space to display them and that some of the markers are single positions (substitutions), while others are ranges (Ns), and they can overlap, hide each other, and cause all kinds of other visual anomalies.

This time I adjusted markers:

  • for all ranges (peptide fs, nuc fs, aa Xs, nuc Ns, nuc dels, unsequenced nucs)
  • for nucleotide substitutions (they are non-range, 1-nuc markers)

by subtracting half of a width of the nuc/aa from their left positions, so that they are shifted left by a half a nuc/aa, ensuring they are anchored at the center of the first nuc/aa in the range. It is important to note that the width of a nuc/aa is variable, so the max of pixel-per-item and min-item-width need to be considered as a width.

We need to check how it plays with the AA mutation groups (which are the single-aa markers aggregated into groups), which has a more complex formula for positions (see #638, #644).

I believe that the proper fix would be to add pan and zoom (#295).

Resolves #708 #631
Followup of #638, #644

Continues on the adventure of centering various markers in sequence views, started in #644. 

The goal is to make the different markers to be aligned to their positions in the sequence properly, as well as to be aligned between each other. The challenge is that there's enough space to display them and that some of the markers are single positions (substitutions), while others are ranges (Ns), and then can overlap, hide each other and cause all kinds of visual anomalies.

This time I adjusted markers:
 - for all ranges (peptide fs, nuc fs, aa Xs, nuc Ns, nuc dels, unsequenced nucs)
 - for nucleotide substitutions (they are non-range, 1-nuc markers)
by subtracting  half of a width of the nuc/aa from their left positions, so that they are shifted left by a half a nuc/aa, ensuring they are anchored at the center of the first nuc/aa in the range. It is important to note that the width of a nuc/aa is variable, so the max of pixel-per-item and min-item-width need to be considered as a width.

We need to check how it plays with the AA mutation groups (which are the single-aa markers aggregated into groups), which has a more complex formula for positions (see #638, #644).
@vercel
Copy link

vercel bot commented Jan 27, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/nextstrain/nextclade/GaJZkUXED8UR3xtS9ddBdU3i5QCm
✅ Preview: https://nextclade-git-fix-center-markers-nextstrain.vercel.app

Comment on lines -27 to +30
const x = begin * pixelsPerBase - pixelsPerBase / 2
let width = (end - begin) * pixelsPerBase
width = Math.max(width, BASE_MIN_WIDTH_PX)
const halfNuc = Math.max(pixelsPerBase, BASE_MIN_WIDTH_PX) / 2 // Anchor on the center of the first nuc
const x = begin * pixelsPerBase - halfNuc
Copy link
Member Author

Choose a reason for hiding this comment

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

Let's take the missing range (N nucs) as an example.

I take a max of "pixels per nuc" and "min nuc width", which gives the width of a single nuc for this particular marker in pixels.

Then I take half of this number, which gives a half of a nuc.

I subtract this half-nuc-width from the x position of the rectangle representing the range.

Elements in SVG are anchored on their left edges by default. So subtracting half a nuc shifts the rectangle by half a nuc to the left. This way it looks as if it is anchored at the center of the first nuc in the range.

Comment on lines -59 to +60
const x = pos * pixelsPerBase - width / 2
const halfNuc = Math.max(pixelsPerBase, BASE_MIN_WIDTH_PX) / 2 // Anchor on the center of the first nuc
const x = pos * pixelsPerBase - halfNuc
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only non-range marker changed: the nuc substitution marker.

Similarly, I shift it to the left by half a nuc. Note it was already shifted before by (width / 2). But because width is always 1 nuc, I find it more explicit to subtract the half a nuc. Gives the same result, but looks more in line with the rest of markers.

@rneher
Copy link
Member

rneher commented Jan 27, 2022

The challenge with mutation groups was that we need to center the group, not the first mutations. But I don't think this is necessary here. So I think this is good.

@ivan-aksamentov ivan-aksamentov marked this pull request as ready for review January 27, 2022 21:53
Copy link
Member

@corneliusroemer corneliusroemer left a comment

Choose a reason for hiding this comment

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

Looks good in the test build for single nucs.

Ranges are still not perfect but that's simply an impossibility due to magnification.

@ivan-aksamentov ivan-aksamentov merged commit 0cd4226 into master Jan 31, 2022
@ivan-aksamentov ivan-aksamentov deleted the fix/center-markers branch January 31, 2022 06:22
ivan-aksamentov added a commit that referenced this pull request Jan 31, 2022
Addresses some of the concerns for #712

Here I shifted sequence (peptide) view's SVG viewBox 1/n of nucleotide (peptide) width to the right. This allows for the markers close to the begging or end of the view to be visible.

This is required after adjustments made for centering markers in 
#638 #644 #709
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.

Visual: Pixel ranges for unknown aa range and aa substitutions don't seem to match
3 participants