DYN-6015 LiveCharts2 migration for net6 #14209

zeusongit · 2023-07-28T21:17:26Z

Purpose

https://jira.autodesk.com/browse/DYN-6015

Updated dependency for chart nodes to LiveCharts2 from version 1 that was not net6 compatible.
This change will introduce new SkiaSharp graphic library

Old Charts:

New Charts:

All charts Export Output Image:

Declarations

Check these if you believe they are true

The codebase is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning and are documented in the API Changes document.
This PR modifies some build requirements and the readme is updated
This PR contains no files larger than 50 MB

Release Notes

Updated Chart Nodes for Net6

Reviewers

@DynamoDS/dynamo

FYIs

@dnenov

QilongTang · 2023-07-31T15:01:41Z

hi @zeusongit Even after I retried the build action, it failed again.. Also I assume this is safe to bring to 2.19 release?

src/DynamoCoreWpf/UI/Themes/Modern/LiveChartsStyle.xaml

QilongTang · 2023-07-31T15:58:26Z

src/Libraries/CoreNodeModelsWpf/Charts/Utilities/ChartStyle.cs

+    /// <summary>
+    /// Helper class providing chart style parameters
+    /// </summary>
+    public static class ChartStyle


Nice 👍🏻

src/Libraries/CoreNodeModelsWpf/Charts/Controls/BarChartControl.xaml.cs

QilongTang · 2023-07-31T16:10:52Z

src/Notifications/NotificationCenterController.cs

@@ -110,6 +110,8 @@ private void View_Closing(object sender, System.ComponentModel.CancelEventArgs e

        async void SuspendCoreWebviewAsync()
        {
+            if (notificationUIPopup == null) return;


Did you foresee a bug somewhere?

Yes, this was throwing an exception during Debug

QilongTang · 2023-07-31T16:11:16Z

src/Libraries/CoreNodeModelsWpf/CoreNodeModelsWpf.csproj

    <PackageReference Include="DotNetProjects.Extended.Wpf.Toolkit" Version="5.0.103" />
+    <PackageReference Include="LiveChartsCore" Version="2.0.0-beta.855" />


Any binary diff?

Since the VS2022 build is failing, the bindiff job is failing as well, the PR does bring in some Livecharts2 and skiasharp binaries.

QilongTang

Looks solid with some comments, just curious have you tested in D4R yet?

zeusongit · 2023-07-31T16:23:17Z

hi @zeusongit Even after I retried the build action, it failed again.. Also I assume this is safe to bring to 2.19 release?

I did not downgrade the Newtonsoft to 13.0.1, it is unchanged in the PR, did some other PR downgrade it before this PR?

QilongTang · 2023-07-31T16:35:21Z

hi @zeusongit Even after I retried the build action, it failed again.. Also I assume this is safe to bring to 2.19 release?

I did not downgrade the Newtonsoft to 13.0.1, it is unchanged in the PR, did some other PR downgrade it before this PR?

I thought we have been using Newtonsoft 13.0.1 for a while, any reason that made you mention downgrade?

src/DynamoCoreWpf/DynamoCoreWpf.csproj

src/DynamoCoreWpf/UI/Themes/Modern/LiveChartsStyle.xaml

mjkkirschner · 2023-07-31T16:39:48Z

src/Libraries/CoreNodeModelsWpf/Charts/BarChartNodeModel.cs

@@ -60,7 +56,7 @@ public class BarChartNodeModel : NodeModel
        /// <summary>
        /// Bar chart color values.
        /// </summary>
-        public List<SolidColorBrush> Colors { get; set; }
+        public List<Color> Colors { get; set; }


Since this is public - ideally we would obsolete the old property, and redirect it to to use and update the new one.

Make sense since this was officially added in 2.18

in addition @zeusongit @QilongTang - if this is going into 2.19 then we'll need to keep the old live charts binaries in the legacy_remove_me folder so they get copied.

If this is only going into a 3.0 release then we can forget the API break, and not include the binaries. I'm not sure that is an option though...? @jasonstratton @pinzart90 ?

Maybe we can RC branch 2.19 early and make this a Dynamo 3.0 change, the downside is that chart nodes will not be tested in test fest early because that event is targeting 2.19 NET6 build.

can always merge and then revert - it's not clear to me this will be done by than anyway...?

src/Libraries/CoreNodeModelsWpf/Charts/BarChartNodeModel.cs

src/Libraries/CoreNodeModelsWpf/Charts/BasicLineChartNodeModel.cs

src/Libraries/CoreNodeModelsWpf/Charts/Controls/BarChartControl.xaml.cs

src/Libraries/CoreNodeModelsWpf/Charts/Utilities/Export.cs

zeusongit · 2023-08-01T18:16:16Z

hi @zeusongit Even after I retried the build action, it failed again.. Also I assume this is safe to bring to 2.19 release?

I did not downgrade the Newtonsoft to 13.0.1, it is unchanged in the PR, did some other PR downgrade it before this PR?

That is what I saw in the error log:

NU1605: Warning As Error: Detected package downgrade: Newtonsoft.Json from 13.0.2 to 13.0.1. Reference the package directly from the project to select a different version.
     DSCoreNodesTests -> DynamoCoreWpf -> LiveChartsCore.SkiaSharpView.WPF 2.0.0-beta.855 -> LiveChartsCore.SkiaSharpView 2.0.0-beta.855 -> LiveChartsCore 2.0.0-beta.855 -> Newtonsoft.Json (>= 13.0.2)

github-actions · 2023-08-01T20:54:16Z

⚠️ [run-bin-diff-net60-windows] - Files Added/Deleted::10 new file(s) have been added and 2 file(s) have been deleted!
(Updated: 2023-08-11-14:39:21)

pinzart90 · 2023-08-01T20:55:36Z

⚠️ [run-bin-diff] - Files Added/Deleted::28 new file(s) have been added and 2 file(s) have been deleted!
⚠️ [run-bin-diff-net60-windows] - Files Added/Deleted::10 new file(s) have been added and 2 file(s) have been deleted!

28 files added? That is a lot

zeusongit · 2023-08-01T21:07:01Z

⚠️ [run-bin-diff] - Files Added/Deleted::28 new file(s) have been added and 2 file(s) have been deleted!
⚠️ [run-bin-diff-net60-windows] - Files Added/Deleted::10 new file(s) have been added and 2 file(s) have been deleted!

28 files added? That is a lot

Yep
For net48:

RelativePath	Status
.\arm64\libSkiaSharp.dll	Added
.\LiveChartsCore.dll	Added
.\libSkiaSharp.dylib	Added
.\HarfBuzzSharp.dll	Added
.\libHarfBuzzSharp.dylib	Added
.\nodes\arm64\libHarfBuzzSharp.dll	Added
.\nodes\arm64\libSkiaSharp.dll	Added
.\nodes\libSkiaSharp.dylib	Added
.\nodes\libHarfBuzzSharp.dylib	Added
.\System.Numerics.Vectors.dll	Added
.\System.Memory.dll	Added
.\System.Drawing.Common.dll	Added
.\System.Buffers.dll	Added
.\SkiaSharp.Views.WPF.dll	Added
.\SkiaSharp.Views.Desktop.Common.dll	Added
.\SkiaSharp.HarfBuzz.dll	Added
.\SkiaSharp.dll	Added
.\nodes\x64\libHarfBuzzSharp.dll	Added
.\nodes\x64\libSkiaSharp.dll	Added
.\nodes\x86\libHarfBuzzSharp.dll	Added
.\nodes\x86\libSkiaSharp.dll	Added
.\arm64\libHarfBuzzSharp.dll	Added
.\x64\libHarfBuzzSharp.dll	Added
.\x64\libSkiaSharp.dll	Added
.\x86\libHarfBuzzSharp.dll	Added
.\x86\libSkiaSharp.dll	Added
.\LiveChartsCore.SkiaSharpView.dll	Added
.\LiveChartsCore.SkiaSharpView.WPF.dll	Added
.\LiveCharts.Wpf.dll	Deleted
.\LiveCharts.dll	Deleted

For net6:

RelativePath	Status
.\LiveChartsCore.SkiaSharpView.dll	Added
.\HarfBuzzSharp.dll	Added
.\libHarfBuzzSharp.dll	Added
.\libSkiaSharp.dll	Added
.\SkiaSharp.Views.WPF.dll	Added
.\SkiaSharp.Views.Desktop.Common.dll	Added
.\LiveChartsCore.dll	Added
.\LiveChartsCore.SkiaSharpView.WPF.dll	Added
.\SkiaSharp.HarfBuzz.dll	Added
.\SkiaSharp.dll	Added
.\LiveCharts.Wpf.dll	Deleted
.\LiveCharts.dll	Deleted

This reverts commit 06355fd.

pinzart90 · 2023-08-08T15:46:53Z

src/DynamoCoreWpf/DynamoCoreWpf.csproj

@@ -140,10 +140,6 @@
 	<PackageReference Include="SharpDX.Mathematics" Version="4.2.0" />
    <Reference Include="Dynamo.Microsoft.Xaml.Behaviors">
      <HintPath>..\..\extern\Microsoft.Xaml.Behaviors\$(TargetFramework)\Dynamo.Microsoft.Xaml.Behaviors.dll</HintPath>
-    </Reference>
-    <PackageReference Include="Prism.Core" Version="8.1.97" Condition=" '$(TargetFramework)' != 'net48' " />


I think you should have left in there
Maybe just remove the condition attribute (ex )

I removed it since it is already referenced in the line below it.
https://github.com/DynamoDS/Dynamo/pull/14209/files/e23d79094af0f648d893c2a3fcb07c9c47627ba9..efebe65238c7dee671845e6834945248f5828120#diff-3a61f1c3321e4a236dbd60644db81ea7d71eb5eb709191187126610ff76abd09R145

pinzart90 · 2023-08-08T18:00:30Z

@zeusongit @mjkkirschner @QilongTang a lot of public APIs have been changed (all in the UI layer)
Do you think this is ok for 3.0 ?
Maybe not cherry-packable in 2.19 as is. Not sure why we would want to...

pinzart90

If we consider the breaking the API for 3.0 is ok, then LGTM

mjkkirschner · 2023-08-08T19:05:40Z

@zeusongit @mjkkirschner @QilongTang a lot of public APIs have been changed (all in the UI layer) Do you think this is ok for 3.0 ? Maybe not cherry-packable in 2.19 as is. Not sure why we would want to...

I don't think we have any intention of moving this to 2.19
(all in the UI layer) this is exactly why we have to formalize "API" boundary immediately post 3.0 release. I believe we have communicated or internally agreed that the view/UI was not semantically versioned - of course we decided this AFTER we were forced to because of the visual refresh.

QilongTang · 2023-08-08T19:07:13Z

Please check the milestone field of the PR for the release target. @mjkkirschner @pinzart90 not targeting 2.19

mjkkirschner · 2023-08-08T19:08:21Z

@zeusongit were you able to resolve the newtonsoft issue?

zeusongit · 2023-08-08T22:14:12Z

@zeusongit were you able to resolve the newtonsoft issue?

Yes, by including version 13.0.2

mjkkirschner · 2023-08-08T22:15:15Z

@zeusongit were you able to resolve the newtonsoft issue?

Yes, by including version 13.0.2

so... now we reference both versions? This project references 13.0.2 and every other project references 13.0.1? What does Revit reference?

zeusongit · 2023-08-08T22:19:57Z

@zeusongit were you able to resolve the newtonsoft issue?

Yes, by including version 13.0.2

so... now we reference both versions? This project references 13.0.2 and every other project references 13.0.1? What does Revit reference?

Everything else uses 13.0.1

mjkkirschner · 2023-08-09T14:11:25Z

@zeusongit were you able to resolve the newtonsoft issue?

Yes, by including version 13.0.2

so... now we reference both versions? This project references 13.0.2 and every other project references 13.0.1? What does Revit reference?

Everything else uses 13.0.1

@zeusongit I don't think that is good. It could work out okay, but it would be best if we can unify on 13.0.1.
I am hopeful we can do something better than just adding 13.0.2 and not shipping it.

Can you look into it a bit and we can meet up later this week to discuss options?
@pinzart90 advice here?

pinzart90 · 2023-08-09T14:24:03Z

@zeusongit were you able to resolve the newtonsoft issue?

Yes, by including version 13.0.2

so... now we reference both versions? This project references 13.0.2 and every other project references 13.0.1? What does Revit reference?

Everything else uses 13.0.1

@zeusongit I don't think that is good. It could work out okay, but it would be best if we can unify on 13.0.1. I am hopeful we can do something better than just adding 13.0.2 and not shipping it.

Can you look into it a bit and we can meet up later this week to discuss options? @pinzart90 advice here?

Just use version 2.0.0-beta.603 of LiveChartsCore and LiveChartsCore.SkiaSharpView.WPF. These have a dep on NewtonSoft 13.0.1. Hopefully no drastic changes in between these versions of livecharts.

This reverts commit 582c5d8.

zeusongit · 2023-08-10T20:02:38Z

I changed LiveCharts version from the latest beta 2.0.0-beta.8xx to 2.0.0-beta.6xx of LiveChartsCore and LiveChartsCore.SkiaSharpView.WPF. These have a dep on NewtonSoft 13.0.1

I see that you reverted this, did it not work?

aparajit-pratap · 2023-08-11T01:48:22Z

src/Libraries/CoreNodeModelsWpf/Charts/Utilities/Export.cs

-                // Save document
-                string filename = dialog.FileName;
-                image.Save(filename, ImageFormat.Png);
+                Console.WriteLine("Chart Image Export Error:" + ex.Message);


Do we need this try-catch or was this also for your debugging?

Is Console.WriteLine visible in the Dynamo console UI ? or at least in the console logs ?
Do we have access to the Dynamo Logger here ?
I say we should let the exception bubble up, unless it is something very specific you are trying to catch here @zeusongit .

This was for test as well, LC2 has summed up all charts into 2 types, SKCartesian and Pie, but there is one more Geo charts which are not yet implemented, so was testing if any of the existing charts throw an exception while exporting image.

zeusongit added 2 commits July 28, 2023 17:07

Add LCv2 support

39c0c24

Merge branch 'master' of https://github.com/DynamoDS/Dynamo into lc2-mig

00229e3

zeusongit requested review from mjkkirschner, QilongTang and pinzart July 28, 2023 21:17

QilongTang reviewed Jul 31, 2023

View reviewed changes

src/DynamoCoreWpf/UI/Themes/Modern/LiveChartsStyle.xaml Outdated Show resolved Hide resolved

QilongTang reviewed Jul 31, 2023

View reviewed changes

src/Libraries/CoreNodeModelsWpf/Charts/Controls/BarChartControl.xaml.cs Outdated Show resolved Hide resolved

QilongTang reviewed Jul 31, 2023

View reviewed changes

mjkkirschner reviewed Jul 31, 2023

View reviewed changes

src/Libraries/CoreNodeModelsWpf/Charts/Utilities/Export.cs Outdated Show resolved Hide resolved

reddyashish added this to the 3.0 milestone Aug 1, 2023

zeusongit added 2 commits August 1, 2023 16:45

comments

827babf

Update LiveChartsStyle.xaml

dad9773

avidit changed the title ~~LiveCharts2 migration for net6~~ DYN-6015 LiveCharts2 migration for net6 Aug 2, 2023

zeusongit added 6 commits August 2, 2023 11:29

Merge branch 'master' of https://github.com/DynamoDS/Dynamo into lc2-mig

2c063b8

fix

6361735

Merge branch 'master' of https://github.com/DynamoDS/Dynamo into lc2-mig

b902f3c

Update DynamoCoreWpf.csproj

06355fd

Revert "Update DynamoCoreWpf.csproj"

071f6b0

This reverts commit 06355fd.

Update DynamoCoreWpf.csproj

e23d790

zeusongit added 2 commits August 8, 2023 11:20

remove net48

efebe65

Merge branch 'master' of https://github.com/DynamoDS/Dynamo into lc2-mig

a255b19

pinzart90 reviewed Aug 8, 2023

View reviewed changes

pinzart90 approved these changes Aug 8, 2023

View reviewed changes

zeusongit added 3 commits August 8, 2023 18:07

cleanup

f96ba19

Merge branch 'master' of https://github.com/DynamoDS/Dynamo into lc2-mig

dea1456

Update DynamoCoreWpfTests.csproj

e7066d7

removing test deps-

3cf2673

pinzart added 2 commits August 10, 2023 14:11

Merge branch 'master' into pr/14209

7b20c59

Update CoreNodeModelsWpf.csproj

582c5d8

This comment was marked as outdated.

Sign in to view

Revert "Update CoreNodeModelsWpf.csproj"

6d56a26

This reverts commit 582c5d8.

mjkkirschner approved these changes Aug 10, 2023

View reviewed changes

remove try catch

8645fd1

aparajit-pratap reviewed Aug 11, 2023

View reviewed changes

zeusongit added 3 commits August 11, 2023 10:03

Update Export.cs

6009de0

Merge branch 'master' of https://github.com/DynamoDS/Dynamo into lc2-mig

686e149

Merge branch 'master' of https://github.com/DynamoDS/Dynamo into lc2-mig

c55a216

QilongTang approved these changes Aug 14, 2023

View reviewed changes

zeusongit merged commit 4df50cc into DynamoDS:master Aug 14, 2023

		<PackageReference Include="DotNetProjects.Extended.Wpf.Toolkit" Version="5.0.103" />
		<PackageReference Include="LiveChartsCore" Version="2.0.0-beta.855" />

DYN-6015 LiveCharts2 migration for net6 #14209

DYN-6015 LiveCharts2 migration for net6 #14209

Conversation

zeusongit commented Jul 28, 2023

Purpose

Declarations

Release Notes

Reviewers

FYIs

QilongTang commented Jul 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QilongTang left a comment

Choose a reason for hiding this comment

zeusongit commented Jul 31, 2023

QilongTang commented Jul 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeusongit commented Aug 1, 2023

github-actions bot commented Aug 1, 2023 • edited Loading

pinzart90 commented Aug 1, 2023

zeusongit commented Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinzart90 commented Aug 8, 2023 • edited Loading

pinzart90 left a comment

Choose a reason for hiding this comment

mjkkirschner commented Aug 8, 2023 • edited Loading

QilongTang commented Aug 8, 2023

mjkkirschner commented Aug 8, 2023

zeusongit commented Aug 8, 2023

mjkkirschner commented Aug 8, 2023

zeusongit commented Aug 8, 2023

mjkkirschner commented Aug 9, 2023

pinzart90 commented Aug 9, 2023

This comment was marked as outdated.

zeusongit commented Aug 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 1, 2023 •

edited

Loading

zeusongit commented Aug 1, 2023 •

edited

Loading

pinzart90 commented Aug 8, 2023 •

edited

Loading

mjkkirschner commented Aug 8, 2023 •

edited

Loading