X Tutup
Skip to content

Optimize clipping calls #1661#1662

Merged
Jonarw merged 4 commits intooxyplot:developfrom
Jonarw:clippingoptimization
Sep 27, 2020
Merged

Optimize clipping calls #1661#1662
Jonarw merged 4 commits intooxyplot:developfrom
Jonarw:clippingoptimization

Conversation

@Jonarw
Copy link
Member

@Jonarw 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

Copy link
Contributor

@VisualMelon VisualMelon left a comment

Choose a reason for hiding this comment

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

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.

@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 try to get some stuff done tomorrow (e.g. review this, tidy up the margin PR, fix maps): I'm a bit busy today.

Copy link
Contributor

@VisualMelon VisualMelon left a comment

Choose a reason for hiding this comment

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

Looks great now! Everything is so much tidier.

@Jonarw Jonarw force-pushed the clippingoptimization branch from 3c5fb7e to 171e3d3 Compare September 27, 2020 15:09
@Jonarw
Copy link
Member Author

Jonarw commented Sep 27, 2020

(rebased)

@Jonarw Jonarw merged commit d02fe7d into oxyplot:develop Sep 27, 2020
@Jonarw Jonarw deleted the clippingoptimization branch October 12, 2020 07:29
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.

Optimize Clipping Calls

2 participants

X Tutup