-
Notifications
You must be signed in to change notification settings - Fork 74
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
Element drawing #930
Element drawing #930
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @katehryhorenko)
Elements/src/Units.cs
line 111 at r1 (raw file):
/// <summary> /// Convert from decimal feet to feet and fractional inches /// </summary>
I know these comments were taken from the original location, but can we expand them to include comments for all the parameters? It would be nice for the user to know what "precision" means for example.
Elements/src/Units.cs
line 157 at r1 (raw file):
/// <summary> /// Convert from decimal inches to fractional inches /// </summary>
Same note as above about expanding comments.
We would like to use the work that is done in this PR related to dimension layout to further improve the work earlier done on public class Face
{
...
List<LinearDimension> CreateDimensions()
{
// Insert code taken from SvgElementSection, replacing the normal used in SvgElementSection with the normal of the face.
}
}
public class SvgFaceElevation
{
public SvgFaceElevation(Face face, Vector3 up)
{
// Use our new method here to create all the dimensions of the face.
var dimensions = face.CreateDimensions();
// Use the up vector to to orient the face on the 'page' when generating the SVG or PDF
}
} |
# Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed SvgElementSection with SvgFaceElevation. Maybe it should be SvgFacePlan or SvgFaceDrawing?
I started CreateDimensions implementation. But this change doesn't look easy. Currently I'm drawing a few dimensions on one line, but LinearDimension is one line per dimension. And a lot of nuances like this. I'm suggestion to go ahead with this PR. The CreateDimensions will not change the interface of this class, so I think it shouldn't be a blocker 🤔
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @ikeough)
Elements/src/Units.cs
line 111 at r1 (raw file):
Previously, ikeough (Ian Keough) wrote…
I know these comments were taken from the original location, but can we expand them to include comments for all the parameters? It would be nice for the user to know what "precision" means for example.
Done.
Elements/src/Units.cs
line 157 at r1 (raw file):
Previously, ikeough (Ian Keough) wrote…
Same note as above about expanding comments.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @katehryhorenko)
Elements/src/Units.cs
line 110 at r3 (raw file):
/// <summary> /// Convert from decimal feet to feet and fractional inches
"...and fractional inches." (note period)
Elements/src/Units.cs
line 113 at r3 (raw file):
/// </summary> /// <param name="decimalFeet">The value to convert to a fractional inches representation.</param> /// <param name="roundDigits"></param>
"The number of fractional digits in the return value." (stolen from the c# docs, as this just sets the value for Math.Round)
Elements/src/Units.cs
line 163 at r3 (raw file):
/// </summary> /// <param name="decimalInches">The value to convert to a fractional inches representation.</param> /// <param name="roundDigits"></param>
"The number of fractional digits in the return value." (stolen from the c# docs, as this just sets the value for Math.Round)
Elements.Serialization.SVG/src/SvgFaceElevation.cs
line 52 at r3 (raw file):
/// Initializes a new instance of SvgElementSection class. /// </summary> /// <param name="face">The element to include to the drawing.</param>
Recommend: "The face to draw."
Elements.Serialization.SVG/src/SvgFaceElevation.cs
line 413 at r3 (raw file):
foreach (var line in lines) { var dimensionLine = FindDimenssionLine(line, false);
Suggested rename of the method to FindDimensionLine
Elements.Serialization.SVG/src/SvgFaceElevation.cs
line 552 at r3 (raw file):
} private DimensionLine FindDimenssionLine(Line line, bool isOpening)
"FindDimensionLine"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @katehryhorenko)
Elements.Serialization.SVG/src/SvgFaceElevation.cs
line 50 at r3 (raw file):
/// <summary> /// Initializes a new instance of SvgElementSection class.
"Initializes a new instance of SvgFaceElevation class."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @ikeough)
Elements/src/Units.cs
line 113 at r3 (raw file):
Previously, ikeough (Ian Keough) wrote…
"The number of fractional digits in the return value." (stolen from the c# docs, as this just sets the value for Math.Round)
Done.
Elements/src/Units.cs
line 163 at r3 (raw file):
Previously, ikeough (Ian Keough) wrote…
"The number of fractional digits in the return value." (stolen from the c# docs, as this just sets the value for Math.Round)
Done.
Elements.Serialization.SVG/src/SvgFaceElevation.cs
line 50 at r3 (raw file):
Previously, ikeough (Ian Keough) wrote…
"Initializes a new instance of SvgFaceElevation class."
Done.
Elements.Serialization.SVG/src/SvgFaceElevation.cs
line 52 at r3 (raw file):
Previously, ikeough (Ian Keough) wrote…
Recommend: "The face to draw."
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @katehryhorenko)
CHANGELOG.md
line 21 at r4 (raw file):
- `RoutingHintLine.IsNearby` - `RoutingHintLine.Affects` - `SvgElementSection`
Replace withSvgFaceElevation
.
DESCRIPTION:
This PR contains class that allows to create SVG / PDF drawing of an element. How it works:
TESTING:
FUTURE WORK:
REQUIRED:
CHANGELOG.md
.This change is