Pdf creation + small fixes inside SvgSection #916

katehryhorenko · 2022-11-07T15:59:46Z

BACKGROUND:
We need to convert svg to pdf

DESCRIPTION:

Svg-net library is replaced with Skia. Skia supports PDF creation
SvgSection is not static anymore. Now it's possible to create SvgSection class instance. It has all required properties by default to generate stocking plan.
New extensions like ToSvgLine and ToSvgPolygon that can use SvgSection to get scene bounds
OnElementDrawing event that allows to customize elements drawing
Draw method was changed a little bit. Just small fixes:
- document rotation should be around the middle point, otherwise the image is not at the view box. You still can see it if you open svg file, but during conversion to pdf or any other format, image will be lost. Also, ViewBox should be moved a little bit to fit the image size/location
  doc.ViewBox = new SvgViewBox((wOld - ViewBoxWidth) / 2.0f, (ViewBoxHeight - hOld) / 2.0f, ViewBoxWidth, ViewBoxHeight);
  doc.CustomAttributes.Add("transform", $"rotate({rotation} {ViewBoxWidth / 2.0} {ViewBoxHeight / 2.0})");
- in some reason BaseLineOffset is lost during conversion to pdf, so I replaced it with translate(0, 0.2)

REQUIRED:

All changes are up to date in CHANGELOG.md.

This change is

…ent to customize drawing creation;

ikeough

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @katehryhorenko)

Elements.Serialization.SVG/src/Elements.Serialization.SVG.csproj line 21 at r1 (raw file):

    <PackageReference Include="Svg.Skia" Version="0.6.0-preview2"/>
    <PackageReference Include="SkiaSharp" Version="2.88.3"/>
    <PackageReference Include="SkiaSharp.NativeAssets.Linux.NoDependencies" Version="2.88.3"/>

Is this required when you're not running on the lambda? I know that packages can do cool stuff like pull down platform dependencies depending on how they're built. Just wondering if need to always include this.

Elements.Serialization.SVG/src/Elements.Serialization.SVG.csproj line 29 at r1 (raw file):

    <ItemGroup>
    <None Include="libSkiaSharp.so" Link="%(Filename)%(Extension)" CopyToOutputDirectory="PreserveNewest" />

I've never had to do this for a nuget package that targets Linux. Is this required?

I'm getting up to date here: mono/SkiaSharp#312 (comment). But I'm still not 100% sure that this is required.

Elements.Serialization.SVG/src/SvgSection.cs line 38 at r1 (raw file):

    /// <summary>
    ///

Empty docs make me sad. May I suggest. A section of a model serialized to SVG.

Elements.Serialization.SVG/src/SvgSection.cs line 45 at r1 (raw file):

        /// <summary>
        /// This event occurs before element is added to the svg scene.

"...before an element is added..."

Elements.Serialization.SVG/src/SvgSection.cs line 57 at r1 (raw file):

        /// Initializes a new instance of SvgDrawingPlan class
        /// </summary>
        /// <param name="models">A collection of models to include in the plan</param>

"A collection of models to include in the plan." (note the period) All of these doc comments can end in a period.

Elements.Serialization.SVG/src/SvgSection.cs line 88 at r1 (raw file):

        /// <summary>
        /// Gets or sets an svg context which defines settings for grid elements

I usually don't say "Gets or sets" because one day we'll decide we don't want to "set" anymore and we'll forget to update the docs. I recommend "The svg context which defines settings for the grid elements." The developer tooling does a pretty good job of telling the user if you can get and set.

Elements.Serialization.SVG/src/SvgSection.cs line 302 at r1 (raw file):

Whitespace

Elements.Serialization.SVG/src/SvgSection.cs line 401 at r1 (raw file):

        private static BBox3 ComputeSceneBounds(IList<Model> models)
        {
            var max = new Vector3(double.MinValue, double.MinValue);

I've written this exact same code numerous times. Can we add convenience constructors Vector3.Max and Vector3.Min?

Elements.Serialization.SVG/src/SvgSection.cs line 413 at r1 (raw file):

                        geo.UpdateBoundsAndComputeSolid();

                        if (geo._bounds.Max.X > max.X)

Can we replace this block with geo._bounds.Extend(new[]{max,min})?

Elements.Serialization.SVG/src/SvgSection.cs line 444 at r1 (raw file):

            foreach (var model in models)
            {
                var modelWithoutCusromElements = model;

modelWithoutCustomElements

Elements.Serialization.SVG/src/SvgSection.cs line 600 at r1 (raw file):

        #region Private fields

        private readonly List<Model> models = new List<Model>();

Our general practice is to put private fields at the top of the class and to use C# coding guidelines with camel-cased, prefixed members: https://learn.microsoft.com/en-us/dotnet/csharp/fundamentals/coding-style/coding-conventions#camel-case

katehryhorenko

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @ikeough and @katehryhorenko)

Elements.Serialization.SVG/src/Elements.Serialization.SVG.csproj line 21 at r1 (raw file):

Previously, ikeough (Ian Keough) wrote…

Is this required when you're not running on the lambda? I know that packages can do cool stuff like pull down platform dependencies depending on how they're built. Just wondering if need to always include this.

Yes, it's for lambda

Elements.Serialization.SVG/src/Elements.Serialization.SVG.csproj line 29 at r1 (raw file):

Previously, ikeough (Ian Keough) wrote…

I've never had to do this for a nuget package that targets Linux. Is this required?

I'm getting up to date here: mono/SkiaSharp#312 (comment). But I'm still not 100% sure that this is required.

Yes, it didn't work without this file

Elements.Serialization.SVG/src/SvgSection.cs line 38 at r1 (raw file):

Previously, ikeough (Ian Keough) wrote…

Empty docs make me sad. May I suggest. A section of a model serialized to SVG.

Done.

Elements.Serialization.SVG/src/SvgSection.cs line 45 at r1 (raw file):

Previously, ikeough (Ian Keough) wrote…

"...before an element is added..."

Done.

Elements.Serialization.SVG/src/SvgSection.cs line 88 at r1 (raw file):

Previously, ikeough (Ian Keough) wrote…

I usually don't say "Gets or sets" because one day we'll decide we don't want to "set" anymore and we'll forget to update the docs. I recommend "The svg context which defines settings for the grid elements." The developer tooling does a pretty good job of telling the user if you can get and set.

Done.

Elements.Serialization.SVG/src/SvgSection.cs line 302 at r1 (raw file):

Previously, ikeough (Ian Keough) wrote…

Whitespace

Done.

Elements.Serialization.SVG/src/SvgSection.cs line 401 at r1 (raw file):

Previously, ikeough (Ian Keough) wrote…

I've written this exact same code numerous times. Can we add convenience constructors Vector3.Max and Vector3.Min?

Done.

Elements.Serialization.SVG/src/SvgSection.cs line 413 at r1 (raw file):

Previously, ikeough (Ian Keough) wrote…

Can we replace this block with geo._bounds.Extend(new[]{max,min})?

Done.

Elements.Serialization.SVG/src/SvgSection.cs line 444 at r1 (raw file):

Previously, ikeough (Ian Keough) wrote…

modelWithoutCustomElements

Done.

Elements.Serialization.SVG/src/SvgSection.cs line 600 at r1 (raw file):

Previously, ikeough (Ian Keough) wrote…

Our general practice is to put private fields at the top of the class and to use C# coding guidelines with camel-cased, prefixed members: https://learn.microsoft.com/en-us/dotnet/csharp/fundamentals/coding-style/coding-conventions#camel-case

Done.

ikeough

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @katehryhorenko)

Elements/src/Geometry/Vector3.cs line 79 at r2 (raw file):

        /// <summary>
        /// The smallest possible value of a Elements.Vector

Recommend "The smallest possible value of a Vector3."

Elements/src/Geometry/Vector3.cs line 87 at r2 (raw file):

        /// <summary>
        /// The largest possible value of a Elements.Vector

Recommend "The largest possible value of a Vector3."

Elements.Serialization.SVG/src/SvgSection.cs line 112 at r2 (raw file):

        /// <summary>
        /// Gets or sets if gridlines exist, should they be shown in the plan.

Missed one! Recommend "Should grid lines be shown in the section?"

katehryhorenko

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @ikeough)

Elements/src/Geometry/Vector3.cs line 79 at r2 (raw file):

Previously, ikeough (Ian Keough) wrote…

Recommend "The smallest possible value of a Vector3."

Done.

Elements/src/Geometry/Vector3.cs line 87 at r2 (raw file):

Previously, ikeough (Ian Keough) wrote…

Recommend "The largest possible value of a Vector3."

Done.

Elements.Serialization.SVG/src/SvgSection.cs line 57 at r1 (raw file):

Previously, ikeough (Ian Keough) wrote…

"A collection of models to include in the plan." (note the period) All of these doc comments can end in a period.

Done.

Elements.Serialization.SVG/src/SvgSection.cs line 112 at r2 (raw file):

Previously, ikeough (Ian Keough) wrote…

Missed one! Recommend "Should grid lines be shown in the section?"

Done.

katehryhorenko added 5 commits November 4, 2022 16:23

Created SvgDrawingPlan that can generate drawing plan and provides ev…

2b586e1

…ent to customize drawing creation;

Created SvgDrawingPlan that can generate drawing plan and provides ev…

a097f48

…ent to customize drawing creation;

Merge branch 'master' into SVG-PDF

d19d704

SvgDrawingPlan -> SvgSection

9d10cf3

Change log

f55a84f

katehryhorenko requested a review from ikeough November 7, 2022 16:05

ikeough requested changes Nov 9, 2022

View reviewed changes

Fixes from PR comments;

4668763

katehryhorenko commented Nov 9, 2022

View reviewed changes

Fixes from PR comments;

a93aeea

ikeough approved these changes Nov 11, 2022

View reviewed changes

ikeough and others added 3 commits November 15, 2022 11:30

Merge branch 'master' into SVG-PDF

a5d982f

Fixes from PR comments;

ec40ec6

Merge branch 'SVG-PDF' of github.com:hypar-io/Elements into SVG-PDF

915b4b5

katehryhorenko commented Nov 22, 2022

View reviewed changes

katehryhorenko and others added 2 commits November 22, 2022 10:42

Updated change log

b85193b

Merge branch 'master' into SVG-PDF

e239e70

katehryhorenko merged commit aa61a3a into master Nov 22, 2022

katehryhorenko deleted the SVG-PDF branch November 22, 2022 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pdf creation + small fixes inside SvgSection #916

Pdf creation + small fixes inside SvgSection #916

katehryhorenko commented Nov 7, 2022 •

edited

ikeough left a comment

katehryhorenko left a comment

ikeough left a comment

katehryhorenko left a comment

Pdf creation + small fixes inside SvgSection #916

Pdf creation + small fixes inside SvgSection #916

Conversation

katehryhorenko commented Nov 7, 2022 • edited

ikeough left a comment

Choose a reason for hiding this comment

katehryhorenko left a comment

Choose a reason for hiding this comment

ikeough left a comment

Choose a reason for hiding this comment

katehryhorenko left a comment

Choose a reason for hiding this comment

katehryhorenko commented Nov 7, 2022 •

edited