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

Update to 2.1.0 #38

Merged
merged 20 commits into from
Dec 26, 2021
Merged

Update to 2.1.0 #38

merged 20 commits into from
Dec 26, 2021

Conversation

VisualMelon
Copy link
Contributor

@VisualMelon VisualMelon commented Oct 5, 2021

TODO:

  • review edge rendering modes in CanvasRenderContext
  • check column examples (if any remain)
  • investigate perf (feels pretty poor; may be changes in CanvasRenderContext)
  • investigate crashing examples (in AvaloniaExamples):
    • BarSeriesDemo
    • HeatMapDemo
    • all UserControlDemos are broken
  • can't pan/zoom Plot with default axes (e.g. Line and Area example)

There are many changes that will need some time to 'sink in', so I don't think a 2.1.0 release will be viable soon if we want it to be stable, though we should provide a pre-release to get feedback. May be worth releasing a 2.0 version before the update.

@BobLd
Copy link
Contributor

BobLd commented Dec 19, 2021

Hi @VisualMelon, thanks a lot for you job on that.

I've used the modifications in your branch to swith to 2.1.0 but it seems that the Legend does not implement CreateModel(). I had to comment the internalModel.Legends.Add(l.CreateModel()); line. Let me know if I did something wrong.

Also, let me know if I can help speeding up the move to 2.1.0

@VisualMelon
Copy link
Contributor Author

VisualMelon commented Dec 19, 2021

@BobLd wow, looks like I forgot to add the Legend code in my commits. My local branch was a bit of a mess; I was hoping to have lots of time for 2.1 work, but that hasn't worked out at all.

If you're keen to help, the OP above lists some issues I'd identified, but didn't get around to addressing: any progress on understanding/fixing those would be very welcome, but I'm afraid I really don't have a lot of time at the moment to get stuck into large issues like this.


(In aid of future porting while it occurs to me, I think the properties were converted from the WPF version of the same with this VS Find&Replace regex https://gist.github.com/VisualMelon/c03fb801f689ef4786effa34cf5fc6d1 ; changed call-backs were dealt with separately)

@BobLd
Copy link
Contributor

BobLd commented Dec 19, 2021

@VisualMelon, thanks a lot for your prompt feedback, that's great! Will test it shortly.

I'll have a look at the list above and see what I can do. Thanks again!

@VisualMelon
Copy link
Contributor Author

To clarify, the line you removed should be there: hopefully with the Legend code in-place it'll be happier.

I can't find any column examples, and I do seem to be able to pan default axes, so not sure what they were about. I think the issue that was really doing my head in was the crash with BarSeriesDemo in the AvaloniaExamples project (indeed, I still have some lingering break-points on that stuff); I think I concluded that was due to some weirdness in the new BarSeriesManager, and couldn't work out how to work around it in Avalonia. I don't recall investigating the other crashes.

Perf does feel terrible; probably best to start by working out when the regression was introduced.

@BobLd
Copy link
Contributor

BobLd commented Dec 19, 2021

Regarding the BarSeriesDemo, the issues I have spotted are:

  • Last bar series not rendered when using plot/xaml:
    image
    image
    One (dirty) way to fix that is, in PlotBase, to replace the OnSizeChanged by the following. Unfortunatly, I could not find a better way.
        private void OnSizeChanged(object sender, Size size)
        {
            if (size.Height > 0 && size.Width > 0)
            {
                InvalidatePlot(sender is Plot);
            }
        }
  • System.InvalidOperationException when clicking on legend in plot/xaml mode. I still need to investigate that. NB: I have updated Avalonia to 0.10.11-rc.2.

EDIT: Regarding the performance, what you did in #22 indeed increase performance greatly, I would start with that
EDIT 2: updates available here https://github.com/BobLd/oxyplot-avalonia/commits/oxy2.1

@VisualMelon
Copy link
Contributor Author

VisualMelon commented Dec 19, 2021

Ah, yes. The issues are all with the XAML stuff, of course...:

  • The Area Series demo has default axes, and those refuse to zoom/pan
  • Legends crash, as you observe

Re. bar series, it seems to be due the differences between ItemsControl.Items in Avalonia (which defaults to an empty collection) and ItemsControl.ItemsSource in WPF (which defaults to null): the empty collection makes BarSeriesManager unhappy in the former case, because it expects ItemsSourceItems to have been initialised, but it has not. Setting Items = null in Series..ctor seems to stop the crashes (without seeming to break anything else), but doesn't resolve the missing series.


Thanks for pushing your work! We don't want to reference an RC in the final thing, but if Avalonia releases before this that's fine. Re. the local reference, that's fine to change as well, just note that making it local can make debugging easier.

@VisualMelon
Copy link
Contributor Author

VisualMelon commented Dec 19, 2021

Missing series is just a small error in BarSeriesBase: the ValueField property was only invoking AppearanceChanged, not DataChanged; line 100 of BarSeriesBase should be

ValueFieldProperty.Changed.AddClassHandler<BarSeriesBase>(DataChanged);

The same issue exists in OxyPlot-Contrib, so I guess it's just luck that it has hasn't manifested in that repository.

@BobLd
Copy link
Contributor

BobLd commented Dec 19, 2021

Well spotted, it's fixed in my branch. Thanks for that!

Will try to have a look at the other exception (of course no RC in final package by the way)

@VisualMelon
Copy link
Contributor Author

@BobLd cool; does it work for you without the Items = null assignment? Mine still crashes without it. I think this reveals an issue with the core library (it should tolerate an empty series), but consistency with OxyPlot-Contrib is probably a good thing.

@BobLd
Copy link
Contributor

BobLd commented Dec 20, 2021

@VisualMelon, It runs without crashing and I can see the bar charts.

I did not add Items = null in Series..ctor, and as far as I can see, it's not here in the version I have (just to make sure we are talking about the same thing):

protected Series()
{
    eventListener = new EventListener(OnCollectionChanged);
}

Let me know if I misunderstood something

@VisualMelon
Copy link
Contributor Author

VisualMelon commented Dec 20, 2021

@BobLd no, I think you've understood perfectly. I just tested and your code crashes on my machine; I suppose this must be an inconsistency in some non-deterministic behaviour in the Avalonia property engine.

On my machine, without Items = null in the Series constructor the XAML BarSeries example crashes because Items is initially an empty list, and BarSeriesManager doesn't like that. Including it in your code resolves the issue for me again.

I think we should include the line with a comment explaining this, because it makes it more consistent with Wpf-Contrib. Something like

// Set Items to null for consistency with WPF behaviour in Oxyplot-Contrib
// Works around issue with BarSeriesManager throwing on empty Items collection in OxyPlot.Core 2.1
Items = null;

I should sort out a PR to fix the core side of things. (Looking deeper, it's a bit of tangled web; there are other bits that will flatly crash, so it probably won't be a nice simple fix)

@BobLd
Copy link
Contributor

BobLd commented Dec 20, 2021

That's very strange! For the records, I'm running the code on a Windows 10 laptop, and yes - it seems your solution is the only one short term.

I continue working on the performance side of things in my branch using your PR. e.g. I've created DrawPolygonsByStreamGeometry() to avoid a lots of if statements during drawing and remvoving from the loops code that needs to be run only once

EDIT: Any idea what's the purpose of BeginBatchUpdate() and EndBatchUpdate() in Avalonia?

@VisualMelon
Copy link
Contributor Author

VisualMelon commented Dec 21, 2021

InvalidatePlot is calling Plot.UpdateModel, which calls SynchronizeAxes which deletes the default axes, causing the problems with panning/zooming when default axes are used.

Two problems with this:

  1. InvalidatePlot probably shouldn't be calling UpateModel as implemented in Plot: it should not re-build the entire model every time anything happens.
  2. Syncing axes probably shouldn't purge default axes

I'm trying to work out how to resolve the first by inspecting OxyPlot-Contrib (which doesn't do this), but it's taking a while. This change may have knock-on effects.

The second can probably be resolved by only removing axes that are not listed as default axes. Again, this may have some knock-on effects.

Fixing either should resolve the no-panning problem; the first should definitely be resolved if possible; the second is also present in OxyPlot-Contrib.

@BobLd
Copy link
Contributor

BobLd commented Dec 21, 2021

Update concerning Items = null: I've just added the Oxyplot project as a direct reference (not the NuGet) and it indeed throws in BarSeriesManager if Items = null. It must have thrown silently before. At leaast the behaviour is consistent.

I'm currently working on the Bar series legend exception

@VisualMelon
Copy link
Contributor Author

VisualMelon commented Dec 21, 2021

I think I've resolved that last. It looks like I made a similar set of changes in OxyPlot-Contrib a while back, and I've pretty much just copied those.

@BobLd I've rebased your stuff into my 2.1 branch (corresponding to this PR): I hope you don't mind? This should make it easier for us to discuss details within the confines of the PR.


New XAML issues noticed:

  • Setting Foreground on PieSeries breaks its text, which is otherwise not as expected (grey rather than default of black; see PieDemo)
  • Example for EllipseAnnotation crashes
  • Example for PolylineAnnotation doesn't show

HeatMapDemo seems to work now; not sure what has changed in that regard. Legends are also no longer crashing when toggled with the mouse.

@VisualMelon
Copy link
Contributor Author

@BobLd ah, again I meant the XAML AnnotationDemo example for PolyLine (pictured)
image

None-the-less, thankyou for investigating the weirdness in the ExampleBrowser; that issue has always been there, and I vaguely recall thinking it was to do with focus, but I've never really looked into it properly.

The example browser issue should probably be in its own issue if you want to make one, since it is independent of the 2.1 changes.

@VisualMelon
Copy link
Contributor Author

VisualMelon commented Dec 21, 2021

I can't see anything wrong with the UserControl demos now, but they don't look like their pictures (I suppose we should update the pictures? (low priority)). Marking that as resolved in the OP.

I've fixed EllipseAnnotation by adding more callbacks; OxyPlot-Contrib doesn't have them, so not sure what that is about (though thinking about it, it probably should, so perhaps a case of a slight inconsistency between the order in which things happen in WPF and Avalonia revealing an old bug).

@BobLd
Copy link
Contributor

BobLd commented Dec 21, 2021

haha my bad :D And yes, well spotted for the EllipseAnnotation the Width/Height were NaN

@BobLd
Copy link
Contributor

BobLd commented Dec 21, 2021

@VisualMelon for the polyliine issue, adding the following seems to do the trick:
PointsProperty.Changed.AddClassHandler<PolylineAnnotation>(DataChanged);

@BobLd
Copy link
Contributor

BobLd commented Dec 21, 2021

@VisualMelon - Fixed PieSeries color issues, problem was in ToOxyColor() (cf my branch)

@VisualMelon
Copy link
Contributor Author

@BobLd thanks! (using your fixes over mine)

I'm not sure why the Foreground snaps to Gray in PieSeries when it isn't explicitly, but I don't think that's an issue with the library, so I'm not worried about it for the purpose of this PR.

@VisualMelon
Copy link
Contributor Author

VisualMelon commented Dec 21, 2021

I'm not at this time aware of any remaining serious issues. We might want to do something about #32 before a release, but I think this gets up-to-date with 2.1.

@BobLd thanks for you help with this!

@VisualMelon VisualMelon marked this pull request as ready for review December 21, 2021 16:12
@BobLd
Copy link
Contributor

BobLd commented Dec 21, 2021

@VisualMelon My pleasure! I'm working on the performance improvements. I guess the easiest solution would be to always use DrawPolygon(s) in order to use DrawPolygonsByStreamGeometry.

Also, I think there's an issue in DrawPolygons() when UseStreamGeometry is not activated. My bad! checking right now

@BobLd
Copy link
Contributor

BobLd commented Dec 21, 2021

@VisualMelon - Issue in DrawPolygons() fixed in my branch. I'll create another PR for the performance improvements

@BobLd
Copy link
Contributor

BobLd commented Dec 23, 2021

@VisualMelon - I fixed another issue in CanvasRenderContext that was preexisting: DrawLineSegments() was not overriding the underlying function, meaning it wass never called. This is a huge performance improvement, e.g. for cross markers

see https://github.com/BobLd/oxyplot-avalonia/tree/oxy2.1

@VisualMelon
Copy link
Contributor Author

@BobLd good catch, merged.

@BobLd
Copy link
Contributor

BobLd commented Dec 23, 2021

@VisualMelon - I've added DrawEllipsesByStreamGeometry which improve greatly the rendering performance of circle marker, still in https://github.com/BobLd/oxyplot-avalonia/commits/oxy2.1

@VisualMelon
Copy link
Contributor Author

@BobLd fantastic, that is a massive improvement!

Could we change isDefined to isSolid?

@BobLd
Copy link
Contributor

BobLd commented Dec 23, 2021

@VisualMelon sure, it's now updated

@VisualMelon
Copy link
Contributor Author

@BobLd do you have any other perf issues you are looking into?

If not, I think this is probably ready for a 2.1.0-Preview1 release to nuget. This will give people a chance to use it and abuse it before we commit to 2.1.0.

I'm going to do some testing to see if there is an issue with plots in docks, as we had issues with Wpf in the main repo.

@BobLd
Copy link
Contributor

BobLd commented Dec 26, 2021

@VisualMelon I'm good for the moment and I'm happy for you to merge and making the preview1 available 💪

@VisualMelon VisualMelon merged commit 87f62c1 into oxyplot:master Dec 26, 2021
@VisualMelon
Copy link
Contributor Author

2.1.0-Preview1 pushed to NuGet.

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.

None yet

2 participants