The Wayback Machine - https://web.archive.org/web/20200927051935/https://github.com/oxyplot/oxyplot/pull/1662
Skip to content

/ oxyplot

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

Optimize clipping calls #1661 #1662

Open
wants to merge 3 commits into
base: develop
from
Open

Optimize clipping calls #1661 #1662

wants to merge 3 commits into from
+759 −1,142

Conversation

@Jonarw
Copy link
Member

Jonarw commented Sep 20, 2020

Fixes #1661.

Checklist

  • I have included examples or tests (there are enough examples demonstration clipping I think)
  • I have updated the change log
  • I am listed in the CONTRIBUTORS file
  • I have cleaned up the commit history (use rebase and squash)

Changes proposed in this pull request:

  • Move responsibility for clipping out of the Render methods of Series and Annotations
  • Remove DrawClippedXxx() extensions
  • Avoid unnecessary clipping calls

Remarks

  • The PathAnnotation.ClipText property for obvious reasons was broken by these changes. As it didn't quite seem to fit the overall design anyway and appears not to be very widely used, I decided to remove it. The only place that actually used this property was the Train Schedule example. As I quite liked this example I fixed it using an Axis for the station labels, which seems more sensible to me anyway.
  • The CandleStickAndVolumeSeries could only be fixed by a rather ugly hack. If anything, this indicates to me that this series violates some of the design principles the library is built around, so I applied the hack and marked it as obsolete with the recommendation to use separate CandleStickSeries and VolumeSeries instead.
  • I noticed that Annotations could return different clipping rectangles depending on whether you use Annotation.GetClippingRect() or the ITransposablePlotElement.GetClippingRect() extension. As this is not an ideal situation, I slightly changed the design here to make it more consistent.

@oxyplot/admins
Jonarw added 2 commits Sep 20, 2020
@Jonarw
Optimize clipping calls #1661
Loading status checks…
2147a9c
@Jonarw
Mention CandleStickAndVolumeSeries in changelog
Loading status checks…
41b6c24
@VisualMelon
VisualMelon reviewed Sep 20, 2020
View changes
Copy link
Contributor

VisualMelon left a comment

Basically looks good to me. I'm glad that everything still has control over it's clipping rectangle, even if it doesn't set it.
Source/OxyPlot/Annotations/ImageAnnotation.cs Outdated
return base.GetClippingRect();
}

return new OxyRect(0, 0, double.PositiveInfinity, double.PositiveInfinity);

This comment has been minimized.

Sign in to view
@VisualMelon

VisualMelon Sep 20, 2020 Contributor

Should we introduce a standard Everything OxyRect (like DataPoint.Invalidate) to which we can compare in the code that sets the clipping rectangle to check it isn't an 'Everything' OxyRect?

Also, the ImageSharp render context won't cope with out-of-bounds clipping rects; I should probably fix that. It also looks like it blits back too much of the clipped-buffer... I should probably fix that too.

This comment has been minimized.

Sign in to view
@VisualMelon

VisualMelon Sep 20, 2020 Contributor

Thinking about it further, that would be useful if GetClippingRect where moved to PlotElement, but might also create confusion if it can't be used to splat a rectangle over the whole screen when passed as an argument to a drawing method (which I happen to do in some code I have, though I can't remember why). Perhaps a better option would be to use PlotModel.PlotBounds.

This comment has been minimized.

Sign in to view
@Jonarw

Jonarw Sep 20, 2020 Author Member

Should we introduce a standard Everything OxyRect (like DataPoint.Invalidate) to which we can compare in the code that sets the clipping rectangle to check it isn't an 'Everything' OxyRect?

Good idea!

Also, the ImageSharp render context won't cope with out-of-bounds clipping rects

Actually I guess it doesn't have to when we implement your first suggestion.

Thinking about it further, that would be useful if GetClippingRect where moved to PlotElement, but might also create confusion if it can't be used to splat a rectangle over the whole screen when passed as an argument to a drawing method (which I happen to do in some code I have, though I can't remember why). Perhaps a better option would be to use PlotModel.PlotBounds.

I am not quite sure I get your point here? We do clip to plot bounds anyway here, so out-of-bounds clipping rects will be trimmed down here.

This comment has been minimized.

Sign in to view
@VisualMelon

VisualMelon Sep 20, 2020 •
edited
Contributor

That last comment was a bit jumbled, I shouldn't worry about; trimming should cover everything smartly. (I somehow missed it was already being done)
Source/OxyPlot/PlotModel/PlotModel.Rendering.cs Outdated

foreach (var plotElement in plotElements)
{
var currentClippingRect = (plotElement as ITransposablePlotElement)?.GetClippingRect();

This comment has been minimized.

Sign in to view
@VisualMelon

VisualMelon Sep 20, 2020 Contributor

Is there any reason we shouldn't move GetClippingRect to PlotElement? It doesn't make sense for this to be restricted to transposable elements.

This comment has been minimized.

Sign in to view
@VisualMelon

VisualMelon Sep 20, 2020 Contributor

I'm a bit unsure that Annotation should implement ITransposablePlotElement directly (Series doesn't). TileMapAnnotation, for example, isn't transpoable, and we have to work around that in ExampleLibrary.Utilities.PlotModelUtilities.

This comment has been minimized.

Sign in to view
@Jonarw

Jonarw Sep 20, 2020 Author Member

Is there any reason we shouldn't move GetClippingRect to PlotElement? It doesn't make sense for this to be restricted to transposable elements.

I can remember considering this briefly, but for some reason I can't remember I decided against it. But if I think about it now it actually does make a lot of sense, so I'll give it a try.

I'm a bit unsure that Annotation should implement ITransposablePlotElement directly (Series doesn't). TileMapAnnotation, for example, isn't transpoable, and we have to work around that in ExampleLibrary.Utilities.PlotModelUtilities.

Well Series doesn't even have XAxis and YAxis properties, which Annotation does have. I guess we could introduce a IXyAxisPlotElement and make ITransposablePlotElement inherit from this. Annotation would then only implement IXyAxisPlotElement and we'd have also have TransposableAnnotation. I'll give it a try and see how it looks.
@Jonarw
Introduce OxyRect.Everything, reorganise Transform functions
Loading status checks…
b27e8b5
@Jonarw
Copy link
Member Author

Jonarw commented Sep 26, 2020

So this went a bit further than planned, but I do think the design makes more sense now.

Basically I realized that it can be problematic that the Transform() and InverseTransform() can not be overridden by Series and Annotations, as they are defined as static extension methods. This made it kind of difficult to implement your feedback.
If I remember correctly the original goal of this design was to enable code sharing between Series and Annotations. But I guess we can also have this with instance methods as long as we keep the actual functionality in a static utility method, which I've done in my latest commit.
@Jonarw
Copy link
Member Author

Jonarw commented Sep 26, 2020

Should we disable/comment out the TileMapAnnotation and related examples for now as it is broken until someone puts in some effort to fix it?
@VisualMelon
Copy link
Contributor

VisualMelon commented Sep 26, 2020

@Jonarw feel free to disable them for the moment, but I take it there is no reason your changes make it unviable?

I'll and get some stuff done tomorrow (e.g. review this, tidy up the margin PR, fix maps): I'm a bit busy today.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VisualMelon VisualMelon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Optimize Clipping Calls
2 participants
@Jonarw @VisualMelon
You can’t perform that action at this time.