-
Notifications
You must be signed in to change notification settings - Fork 940
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 AxisArea, AxisLineArea and AxisTitleArea property in Axis class (… #1966
base: develop
Are you sure you want to change the base?
Conversation
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.
Not a complete review, just found a few minutes to take a look.
Source/OxyPlot/Axes/Rendering/HorizontalAndVerticalAxisRenderer.cs
Outdated
Show resolved
Hide resolved
Thanks for updating: I'll try to review everything again soon. |
Source/OxyPlot/Axes/Rendering/HorizontalAndVerticalAxisRenderer.cs
Outdated
Show resolved
Hide resolved
Source/OxyPlot/Axes/Rendering/HorizontalAndVerticalAxisRenderer.cs
Outdated
Show resolved
Hide resolved
This is looking good now (few typos, but otherwise public API looks nice), and provides a nice foundation we can build on in future (can imagine providing regions/areas (maybe that implies some name changes) for other elements).... but I'm not sure for what purposes at the minute. I'm struggling to think of a nice simple example we can include in the example library to help demonstrate this functionality... it is obviously useful for your custom interaction example, but as it can't be a substitute for the terrifying Suggestions very welcome. Location information is useful for user interaction, and for positioning other UI elements, e.g. custom annotations. |
…beeing strictly immutable.
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.
I think we should merge this. It's non-trivial, so should merge sooner than later so it can be tested before any release.
Still keen to have a nice example, and possibly incorporating a nicer hit-testing API for Axes (per #1419), but unless you think they'd warrant significant changes, I think we should get this in soon.
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.
I think it would be important to have at least some example to demonstrate and verify the functionality. I hacked something together very quickly, which can be used for 'normal' Axes:
[Example("AxisArea")]
public static PlotModel AxisArea()
{
var plotModel = FourAxes();
plotModel.Annotations.Add(new RenderingCapabilities.DelegateAnnotation(context =>
{
foreach (var axis in plotModel.Axes)
{
context.DrawRectangle((OxyRect)axis.AxisArea, OxyColors.Undefined, OxyColor.FromAColor(128, OxyColors.Red), 1, EdgeRenderingMode.Automatic);
context.DrawRectangle((OxyRect)axis.TitleArea, OxyColors.Undefined, OxyColor.FromAColor(128, OxyColors.Green), 1, EdgeRenderingMode.Automatic);
context.DrawRectangle((OxyRect)axis.AxisLineArea, OxyColors.Undefined, OxyColor.FromAColor(128, OxyColors.Blue), 1, EdgeRenderingMode.Automatic);
}
}));
return plotModel;
}
This gives me this output, which seems all in order:
If I understand correctly, this should also work for AngleAxis
, returning an Annulus
? I tried something based on the ArchimedeanSpiral
example, but I received EmptyShape
for the AxisShape
. Is this intended, or am I misunderstanding something?
break; | ||
|
||
} | ||
// TODO: ensure min area of 5px x X to capture clicks, o this above acc. to axis position |
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.
Did this TODO get resolved?
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.
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.
@VisualMelon I added two examples based on @Jonarw's code above and added the AxisArea to be set for the MagnitudeAxisRenderer.
The TODO is resolved as well.
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.
Thanks indeed! Shall try to take a proper look myself later today.
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.
Found a few bugs; fixes given in inline comments.
Missing implementation for AngleAxisFullPlotArea
and MagnitudeAxisFullPlotArea
in MagnitudeAxisFullPlotAreaRenderer
. AngleAxis
should be much the same, but Magnitude Axis
would need to be a rect with a hole in it. We could create a general-purpose 'difference' shape, or a specific shape which is a rect with a rect cut out of it (somehow I suspect this doesn't have a familiar name). (I'd be happy to leave this to another PR to avoid another round of discussion here if it's OK with @Jonarw )
There are issues with AngleAxis.Angle
property (30 in this example), but I don't think they are specific to this PR so shouldn't hold it up.
.
If possible, could you rebase against develop
so that the CI is happy to run again (should just be changes in the changelog)
@@ -274,6 +274,40 @@ protected virtual void RenderAxisTitle(Axis axis, double titlePosition) | |||
|
|||
var lpt = this.GetAxisTitlePositionAndAlignment(axis, titlePosition, ref angle, ref halign, ref valign); | |||
|
|||
var titleSize = this.RenderContext.MeasureText(axis.ActualTitle, axis.ActualTitleFont, axis.TitleFontSize, axis.TitleFontWeight); |
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.
Needs to be var titleSize = this.RenderContext.MeasureText(axis.ActualTitle, axis.ActualTitleFont, axis.ActualTitleFontSize, axis.TitleFontWeight);
(ActualTitleFontSize)
@@ -0,0 +1,158 @@ | |||
using OxyPlot; |
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.
Needs copyright header (see AxisExample.cs for example), and filename should be AxisAreaExamples.cs
@@ -112,9 +119,26 @@ public override void Render(Axis axis, int pass) | |||
ha = HorizontalAlignment.Right; | |||
} | |||
|
|||
maxTextLength = Math.Max(maxTextLength, this.RenderContext.MeasureText(text).Width); |
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.
Needs to be maxTextLength = Math.Max(maxTextLength, this.RenderContext.MeasureText(text, axis.ActualFont, axis.ActualFontSize, axis.ActualFontWeight).Width);
|
||
} | ||
|
||
// ensure minimum height/width of 5 for the title area , centered to the initial location |
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.
Should lose the space before the comma
public void Move(double dx,double dy) | ||
{ | ||
this.x += dx; | ||
this.y += dy; | ||
} |
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.
There is discussion about making these structs readonly (mutable structs are a nightmare and all that): would prefer this returned an updated copy (obviously all usages will need to be modified). I'd also prefer the name Offset
. I'm also going to suggest making this internal
, because a solution built around ScreenVector
would make me happier but is definitely out of scope here.
Should be a space after the comma.
ScreenPoint pt0; | ||
ScreenPoint pt1; |
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.
Can we move these guys to just above their usage
@@ -403,6 +455,9 @@ protected virtual void RenderMajorItems(Axis axis, double axisPosition, double t | |||
SnapTo(plotAreaBottom, ref transformedValue); | |||
} | |||
|
|||
string text = axis.FormatValue(value); | |||
var labelSize = this.RenderContext.MeasureText(text, axis.ActualFont, axis.ActualFontSize, axis.ActualFontWeight); |
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.
Using the overload that takes an angle and passing it axis.Angle
(last param) means we can change the block below to the following, which is simpler and makes the code consistent with the automatic margin codes. (There is also a bug in the Bottom
block at present: should be like Top
but is like Left
and Right
). The behaviour of this still seems a bit funny, but because it matches the margin code it should be left alone in this PR. May be something for us to investigate further another time @Jonarw
case AxisPosition.Left:
pt = new ScreenPoint(axisPosition + a1 - axis.AxisTickToLabelDistance, transformedValue);
this.GetRotatedAlignments(axis.Angle, -90, out ha, out va);
labelEndPosition = Math.Min(labelEndPosition, pt.x - labelSize.Width);
break;
case AxisPosition.Right:
pt = new ScreenPoint(axisPosition + a1 + axis.AxisTickToLabelDistance, transformedValue);
this.GetRotatedAlignments(axis.Angle, 90, out ha, out va);
labelEndPosition = Math.Max(labelEndPosition, pt.x + labelSize.Width);
break;
case AxisPosition.Top:
pt = new ScreenPoint(transformedValue, axisPosition + a1 - axis.AxisTickToLabelDistance);
this.GetRotatedAlignments(axis.Angle, 0, out ha, out va);
labelEndPosition = Math.Min(labelEndPosition, pt.y - labelSize.Height);
break;
case AxisPosition.Bottom:
pt = new ScreenPoint(transformedValue, axisPosition + a1 + axis.AxisTickToLabelDistance);
this.GetRotatedAlignments(axis.Angle, -180, out ha, out va);
labelEndPosition = Math.Max(labelEndPosition, pt.y + labelSize.Height);
break;
@@ -1,7 +1,7 @@ | |||
<Application x:Class="WpfExamples.App" | |||
xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation" | |||
xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" | |||
StartupUri="MainWindow.xaml"> | |||
StartupUri="Examples/AxisAreaDemo/MainWindow.xaml"> |
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.
Looks like this file didn't make it into the commit
Fixes #1949 .
Checklist
Changes proposed in this pull request:
@oxyplot/admins