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.

Sign up for GitHub

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 for the label clipping in the label layout algorithm #3273

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Fix for the label clipping in the label layout algorithm #3273

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0bb3109
Fix for the label clipping and some minor behavior cleanup.
rita-gwen Dec 11, 2024
5d8092e
Merge branch 'master' into Skyline/work/20241210_labelClippingFix
nickshulman Dec 16, 2024
9150078
Fix for the Volcano plot and some code review fixes.
rita-gwen Dec 17, 2024
48ef1e4
Merge branch 'Skyline/work/20241210_labelClippingFix' of https://gith…
rita-gwen Dec 17, 2024
6e96b77
Fix for label clipping in small plots
rita-gwen Dec 20, 2024
ef51d6e
Merge branch 'master' into Skyline/work/20241210_labelClippingFix
rita-gwen Dec 20, 2024
267bb41
Fix to avoid the point marker overlap in the tutorial snapshots
rita-gwen Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions pwiz_tools/Shared/zedgraph/ZedGraph/GraphPane.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,10 @@ public class GraphPane : PaneBase, ICloneable, ISerializable

private LabelLayout _labelLayout;

/// <summary>
/// Indicates if this graph pane has the LabelLayout object attached.
/// Setting it to false removes the layout object.
/// </summary>
public bool EnableLabelLayout
{
get => _labelLayout != null;
Expand Down Expand Up @@ -1561,7 +1565,10 @@ public void AdjustLabelSpacings(List<LabeledPoint> labPoints, List<LabeledPoint.
labeledPoint.Label.IsVisible = true;
}
else
{
labeledPoint.Label.IsVisible = false;
GraphObjList.Remove(labeledPoint.Connector);
}
}

if (visiblePoints.Any())
Expand Down
20 changes: 15 additions & 5 deletions pwiz_tools/Shared/zedgraph/ZedGraph/LabelLayout.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,9 @@ private bool IndexesWithinGrid(Point pt)
return pt.X >= 0 && pt.X < _densityGridSize.Width && pt.Y >= 0 && pt.Y < _densityGridSize.Height;
}

/// <summary>
/// We use square of uniformly distributed randoms to get distribution with a peak
/// </summary>
private int GetRandom(float range)
{
return (int)((_randGenerator.NextDouble() - 0.5) * (_randGenerator.NextDouble() - 0.5) * range * 1.5);
Expand All @@ -266,6 +269,9 @@ private static Rectangle ToRectangle(RectangleF rect)
return new Rectangle((int)rect.X, (int)rect.Y, (int)rect.Width, (int)rect.Height);
}

public const int SEARCH_COUNT_COARSE = 80;
public const int SEARCH_COUNT_FINE = 15;

/**
* Algorighm overview:
* Divide the graph into a grid with cell size equals the label height (the smallest dimension).
Expand All @@ -276,22 +282,23 @@ private static Rectangle ToRectangle(RectangleF rect)
* of the label relative to it's data point.
* The algorighm works in screen coordinates (pixels). There is no need to use user coordinates here.
* Returns true if the label has been successfully placed, false otherwise.
* Note that TextObj location is top-center, not top-left
*/
public bool PlaceLabel(LabeledPoint labPoint, Graphics g)
{
var labelRect = _graph.GetRectScreen(labPoint.Label, g);
var targetPoint = _graph.TransformCoord(labPoint.Point.X, labPoint.Point.Y, CoordType.AxisXYScale);
var labelLength = (int)Math.Ceiling(1.0 * labelRect.Width / _cellSize);
var labelLength = (int)Math.Ceiling(1.0 * labelRect.Width / _cellSize); // label length in grid units

var pointCell = new Point((int)((targetPoint.X - _chartOffset.X) / _cellSize),
(int)((targetPoint.Y - _chartOffset.Y) / _cellSize));
if (!new Rectangle(Point.Empty, _densityGridSize).Contains(pointCell))
return false;
var goal = float.MaxValue;
var goalCell = Point.Empty;
var gridRect = new Rectangle(Point.Empty, _densityGridSize);
var gridRect = new Rectangle(labelLength / 2, 0, _densityGridSize.Width - labelLength, _densityGridSize.Height);
var points = new List<Point>();
for (var count = 80; count > 0; count--)
for (var count = SEARCH_COUNT_COARSE; count > 0; count--)
{
var randomGridPoint = pointCell +
new Size(GetRandom(_densityGridSize.Width), GetRandom(_densityGridSize.Height));
Expand Down Expand Up @@ -319,10 +326,13 @@ public bool PlaceLabel(LabeledPoint labPoint, Graphics g)
var roughGoal = goal;
// Search the cell neighborhood for a better position
var goalPoint = _densityGrid[goalCell.Y][goalCell.X]._location;
for (var count = 15; count > 0; count--)
var chartRect = _graph.Chart.Rect;
var allowedRect = new RectangleF(chartRect.X + labelRect.Width / 2, chartRect.Y,
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure I understand why labelRect.Width / 2 gets added to the X-coordinate, but a similar sort of thing does not get added to the Y-coordinate.
This code might be correct, but I wanted to point out that it looks suspicious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because label coordinates are top-middle, so Y coordinate is not adjusted.

chartRect.Width - labelRect.Width, chartRect.Height - labelRect.Height);
for (var count = SEARCH_COUNT_FINE; count > 0; count--)
{
var p = goalPoint + new Size(GetRandom(_cellSize * 2), GetRandom(_cellSize * 2));
if (!_graph.Chart.Rect.Contains(p))
if (!allowedRect.Contains(p)) // label should not overlap chart's borders
continue;
var goalEstimate1 = GoalFuncion(p, targetPoint, labelRect.Size);
if (goalEstimate1 < goal)
Expand Down
19 changes: 13 additions & 6 deletions pwiz_tools/Skyline/Controls/Graphs/SummaryRelativeAbundanceGraphPane.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,13 @@ private void UpdateFormatting(IEnumerable<MatchRgbHexColor> colorRows)
public void OnLabelOverlapPropertyChange(object sender, PropertyChangedEventArgs e)
{
if (e.PropertyName == @"GroupComparisonAvoidLabelOverlap")
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably have been:

if (e.PropertyName == nameof(Settings.Default.GroupComparisonAvoidLabelOverlap))

so that "Find References" and other sorts of refactoring would work a little bit better.

{
Settings.Default.PropertyChanged -= OnLabelOverlapPropertyChange;
Settings.Default.GroupComparisonSuspendLabelLayout = false;
_labelsLayout = null;
GraphSummary.UpdateUI();
Settings.Default.PropertyChanged += OnLabelOverlapPropertyChange;
Copy link
Contributor

Choose a reason for hiding this comment

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

If I have to write code which removes a listener and adds it back I usually put it inside of a try/finally:

try 
{
    Settings.Default.PropertyChanged -= OnLabelOverlapPropertyChange;
// do other stuff
}
finally
{
    Settings.Default.PropertyChanged += OnLabelOverlapPropertyChange;
}

That way, even if some sort of exception happens, things still get back into an expected state.

For something like this, I would probably just have a field:
private bool _inPropertyChange;
and then in the property change handler:

if (_inPropertyChange) 
{
   return;
}
try 
{
    _inPropertyChange = true;
    if (e.PorpertyName == nameof(Settings.Default.GroupComparisonAvoidLabelOverlap)) 
    {
        // do some stuff
    }
    else if (e.PropertyName == nameof(Settings.Default.GroupComparisonSuspendLabelLayout)) 
    {
        // do other stuff
    }
}
finally 
{
    _inPropertyChange = false;
}

But the way you have this is fine too.

}
else if (e.PropertyName == @"GroupComparisonSuspendLabelLayout")
{
if (!Settings.Default.GroupComparisonSuspendLabelLayout)
Expand Down Expand Up @@ -364,14 +370,15 @@ public override void UpdateGraph(bool selectionChanged)
UpdateAxes();
if (Settings.Default.GroupComparisonAvoidLabelOverlap)
{
if (Settings.Default.GroupComparisonSuspendLabelLayout)
{
AdjustLabelSpacings(_labeledPoints, _labelsLayout);
_labelsLayout = GraphSummary.GraphControl.GraphPane.Layout?.PointsLayout;
}
AdjustLabelSpacings(_labeledPoints, _labelsLayout);
_labelsLayout = GraphSummary.GraphControl.GraphPane.Layout?.PointsLayout;
}
else
DotPlotUtil.AdjustLabelLocations(_labeledPoints, GraphSummary.GraphControl.GraphPane.YAxis.Scale, GraphSummary.GraphControl.GraphPane.Rect.Height);
{
EnableLabelLayout = false;
DotPlotUtil.AdjustLabelLocations(_labeledPoints, GraphSummary.GraphControl.GraphPane.YAxis.Scale,
GraphSummary.GraphControl.GraphPane.Rect.Height);
}
}

private void this_AxisChangeEvent(GraphPane pane)
Expand Down