Implementation of MULTILEADER and MULTILEADERSTYLE #169

mme1950 · 2023-09-15T16:01:15Z

No description provided.

Implement Multileader

DomCR · 2023-09-28T15:47:40Z

Please check the conflicts with master

DomCR · 2023-09-28T15:48:09Z

ACadSharp/DxfPropertyBase.cs

@@ -153,10 +153,6 @@ public void SetValue<TCadObject>(int code, TCadObject obj, object value)
 			{
 				this._property.SetValue(obj, Enum.ToObject(this._property.PropertyType, value));
 			}
-			else if (this._property.PropertyType.IsEquivalentTo(typeof(ushort)))


Why is this if statement being removed?

I am afraid, we did something wrong. We did not change this file. We have to check how this happened.

DomCR · 2023-09-28T15:51:16Z

ACadSharp/Entities/MultiLeader.cs

+
+
+		/// <summary>
+		/// TODO Do we need this class to represent a list of arrow heads


Please don't add a TODO comment in the summary, this should only describe the element and not give development notes.

I've encountered multiple instances of this error, please apply the same solution for all of them.

I pulled the TODO out of the summary.
I think we do not need the ArrowheadAssociation class.
The Arrowheads property should have the type IList (or whatever type an arrowhead is)

DomCR · 2023-09-28T15:53:24Z

ACadSharp/Entities/MultiLeader.cs

+		/// <summary>
+		/// Text attachment direction for MText contents
+		/// 	0 = Horizontal
+		/// 	1 = Vertical


Values shouldn't be specified in the summary.

I've encountered multiple instances of this error, please apply the same solution for all of them.

It is copied from the documentation, just to be sure that the created enum types are correct.
I moved itnto the value tag, OK?

Value tag should only be used to add a commentary about the value restriction, if is an enumeration you don't need to specify the values, you can remove the tag.

DomCR · 2023-09-28T15:58:12Z

ACadSharp/Entities/MultiLeader.cs

+
+		public override CadObject Clone()
+		{
+			MultiLeader clone = (MultiLeader)base.Clone();


There are elements in this object that need to be cloned separately, for an example you can check how the type Entity handles the cloning method.

DomCR · 2023-09-28T15:58:54Z

ACadSharp/Header/CadHeader.cs

@@ -1573,7 +1573,7 @@ public XYZ ModelSpaceYAxis
 		/// </remarks>
 		[CadSystemVariable("$TIMEZONE", 70)]
 		public int TimeZone { get; set; }
-
+		


Not a big deal but to make the review easier avoid this Indentation changes.

There should not be any changes in this class.

DomCR · 2023-09-28T16:13:54Z

ACadSharp/IO/DXF/DxfStreamReader/DxfSectionReaderBase.cs

-					{
-						last.CurveTangent = this._reader.ValueAsDouble;
-					}
-					return true;


Why has this been removed?

As before. We did not make any changes to this class.

DomCR · 2023-09-28T16:15:06Z

ACadSharp/IO/DXF/DxfStreamWriter/DxfHeaderSectionWriter.cs

@@ -10,11 +9,8 @@ internal class DxfHeaderSectionWriter : DxfSectionWriterBase

 		public CadHeader Header { get { return this._document.Header; } }

-		public DxfWriterOptions Options { get; }
-
-		public DxfHeaderSectionWriter(IDxfStreamWriter writer, CadDocument document, CadObjectHolder holder, DxfWriterOptions options) : base(writer, document, holder)


Options should not be removed from this class.

As before. We did not make any changes to this class.

DomCR · 2023-09-28T16:15:27Z

ACadSharp/IO/DXF/DxfStreamWriter/DxfHeaderSectionWriter.cs

@@ -23,9 +19,6 @@ protected override void writeSection()

 			foreach (KeyValuePair<string, CadSystemVariable> item in map)
 			{
-				if (!this.Options.WriteAllHeaderVariables && !this.Options.HeaderVariables.Contains(item.Key))


This if is needed to avoid errors in the writer, please revert the changes.

DomCR · 2023-09-28T16:16:06Z

ACadSharp/IO/DXF/DxfStreamWriter/DxfObjectsSectionWriter.cs

@@ -37,7 +37,7 @@ protected void writeObject<T>(T co)
 					this.writeMappedObject<DictionaryVariable>(dictvar);
 					break;
 				case SortEntitiesTable sortensTable:
-					//this.writeSortentsTable(sortensTable);
+					this.writeSortentsTable(sortensTable);


Same as the reader writeSortentsTable(sortensTable) has some errors, please do not uncomment this line.

DomCR · 2023-09-28T16:16:39Z

ACadSharp/IO/Templates/CadInsertTemplate.cs

-				insert.Block = block;
-			}
-			else if (!string.IsNullOrEmpty(this.BlockName) && builder.TryGetTableEntry(this.BlockName, out block))
+			if (builder.TryGetCadObject(this.BlockHeaderHandle, out BlockRecord block))


This is an old if, please revert to the new one.

DomCR · 2023-09-28T16:23:20Z

First of all, great work!! this looks great!! to continue with the development I would like to add a few notes:

Add test to check that the MLEADER and MLEADERSTYLE are read without errors, the current test files only contain MLEADERSTYLE, you should add a file in each version (if possible) containing MLEADER in it.
The DxfReader and DxfWriter haven't been updated, do you plan to do it in this PR or keep it separately?
When updating the branch be careful for which are the last changes in master, some of my comments were related to this issue.

Overall looks good, I will continue to review the PR while you work on it and continue to give feedback to you.

Thanks for the contribution.

mme1950 · 2023-09-28T19:34:02Z

I am afraid, we did something wrong. Some of the changes you mentioned above have not been made by us. We have to check how this happened.

Maybe we have to redo the fork, create a branch and enter a PR.
I will return to it tomorrow.

mme1950 · 2023-09-29T09:22:18Z

Add test to check that the MLEADER and MLEADERSTYLE are read without errors, the current test files only contain MLEADERSTYLE, you should add a file in each version (if possible) containing MLEADER in it.

OK, we must view the test project.

The DxfReader and DxfWriter haven't been updated, do you plan to do it in this PR or keep it separately?

We then also must add writeMultiLeader and writeMultiLeaderStyle to DwgObjectWriter, right?
I would prefer do do it separately.

When updating the branch be careful for which are the last changes in master, some of my comments were related to this issue.

OK

DomCR · 2023-10-01T11:09:06Z

I've been reviewing the PR, for some reason your PR is pointing to an old version of master and registering a lot of changes that should not be there... I don't know why is this way, I'll try to find out more about this issue.

In the meantime keep an eye to the test, I've approved the build but is failing, check what is happening, it may be caused due this unnecessary changes.

mme1950 · 2023-10-02T11:39:04Z

For a full test we have to create a DWG files with "all" variants of Mutileader also for older AutoCAD versions, right?
To make the test run a subset may be sufficient.
For all variants we have to learn about block content, arrow heads and some other things we did not try yet.

DomCR · 2023-10-04T06:31:41Z

For a full test we have to create a DWG files with "all" variants of Mutileader also for older AutoCAD versions, right? To make the test run a subset may be sufficient. For all variants we have to learn about block content, arrow heads and some other things we did not try yet.

If you want to just test in a single version you have to make sure that the reader will ignore the entities for all other versions to avoid issues or bugs.

… at MultiLeader

DomCR · 2023-10-01T11:03:12Z

ACadSharp/ACadSharp.csproj

This document shouldn't have changes.

DomCR · 2023-10-09T10:21:46Z

ACadSharp/Entities/MultiLeader.cs

@@ -320,10 +320,6 @@ public override CadObject Clone()
 		{
 			MultiLeader clone = (MultiLeader)base.Clone();

-			clone.Style = (MultiLeaderStyle)this.Style?.Clone();
-			clone.LineType = (LineType)this.LineType?.Clone();
-			clone.TextStyle = (TextStyle)this.TextStyle?.Clone();


This should be cloned, otherwise the styles won't be detached form the document.

DomCR · 2023-10-16T14:19:09Z

samples/sample_AC1032.dwg

This document should not be changed, I imagine that you added some MLeader entities in a file and override the existing one.

Instead of overriding the file, create a new one, you can use a name like sample_MLeader_{version}.dwg

DomCR · 2023-10-16T14:23:51Z

ACadSharp/Objects/MultiLeaderAnnotContext.cs

+	/// Common AcDbAnnotScaleObjectContextData data (see paragraph 20.4.71).
+	/// 300 DXF: “CONTEXT_DATA{“
+	/// </summary>
+	public partial class MultiLeaderAnnotContext : CadObject


The documentation for this class should be revised, I see that is copied from the PDF.

DomCR · 2023-10-16T14:25:32Z

ACadSharp/Objects/MultiLeaderAnnotContextClasses.cs

+	/// Common AcDbAnnotScaleObjectContextData data (see paragraph 20.4.71).
+	/// 300 DXF: “CONTEXT_DATA{“
+	/// </summary>
+	public partial class MultiLeaderAnnotContext : CadObject


The documentation for this class should be revised, I see that is copied from the PDF.

DomCR · 2023-10-16T14:28:58Z

ACadSharp/Tables/MultiLeaderStyle.cs

+		/// <value>
+		/// 	0 = Horizontal
+		/// 	1 = Vertical
+		/// </value>


The value tag for the documentation should be only used to add value restrictions like a range of values, enumeration types don't need to be specified.

mme1950 · 2023-10-30T07:30:35Z

It seems that some tests are failing, one of them is the DwgReaderTest the test is making a comparison with a json file that does not belong to the new sample, the best way to go it will be to move the sample to another folder and create an specific test for it.

Also I see that the Clone() test is failing too for the MultiLeader it goes in some kind of loop.

Let me know if you need any help with that.

Hi @DomCR,

I am sorry, I did not have time to work on this project the last two weeks.
I return today to analyse these problems with Nihad.

Then we should try to finish.
I would like to solve the problems regarding the version compliance by simply not supporting mutileaders form AutoCAD versions older than R2010.
I think the object model is OK now.

DomCR · 2023-10-30T08:34:07Z

It seems that some tests are failing, one of them is the DwgReaderTest the test is making a comparison with a json file that does not belong to the new sample, the best way to go it will be to move the sample to another folder and create an specific test for it.
Also I see that the Clone() test is failing too for the MultiLeader it goes in some kind of loop.
Let me know if you need any help with that.

Hi @DomCR,

I am sorry, I did not have time to work on this project the last two weeks. I return today to analyse these problems with Nihad.

Then we should try to finish. I would like to solve the problems regarding the version compliance by simply not supporting mutileaders form AutoCAD versions older than R2010. I think the object model is OK now.

The PR and the implementation looks good, and if you want to focus on one feature at a time that's ok, even if you want to split the PR that would be OK for me too.

Just as a general note, check the documentation comments and avoid to use the /// tag for enums and keep in mind to delete the development methods.

mme1950 · 2023-10-30T10:24:08Z

Hi @DomCR,

is it OK for you, to restrict ths support of multileader entities to version R2010 Plus?
Then I will change the implementation accordingly.

Here are some remarks to our implementation:

Object Model

According to the PDF (OpenDesign_Specification_for_.dwg_files) and online documentation the MultiLeader entity embeds an object that provides a subset ob the properties of a MLeaderAnnotContext object. The MLeaderAnnotContext includes a list of embedded LEADER objects. LEADER objects contain a list of LEDER_LINE objects.

In the DXF file with a MULTILEADER entity we also found a complete MLeaderAnnotContext object with the common properties and properties identical to the object embedded into the MULTILEADER.

This structure is reproduced in the object model:

The MultiLeader class has a Style property referencing a MultiLeaderStyle object.
A MultiLeaderAnnotContext class is defined holding the subset of properties from the MLeaderAnnotContext. Each MultiLeader references exactly one MultiLeaderAnnotContext.
The MultiLeaderAnnotContext has a list of LeaderRoot objects.
A LederRoot has a list of LeaderLine objects.

Several properties of the MultiLeader class also exist in the MultiLeaderAnnotContext class. Values may be redundant or conflicting.

Remarks

In AutoCAD a MultiLeader can have one or more leaders evolving to leader lines (LEADER_LINE). It seems there is no way to add more than one leader (LEADER), having one or more leader lines each.

Thus: There can be only one LEADER. More than one LEADER would require an individual Contents. But the MultiLeader can have has one "Contents", either a Text contents or a contents block.

The MultiLeader class has a LineType property enherited from Entity referencing a LineType object. A second property of type LineType (LeaderLineType) is defined in the MultiLeader class. The referenced LineType objects are different.

Open Issues

Version Compliance

Due to lack of documentation and because we cannot create and test DWG-files with old AutoCAD versions we can support multileaders only for R2010 Plus.

List of Arrowheads

In the PDF (OpenDesign_Specification_for_.dwg_files) documentation and also in the various DXF documentations a list of arrowheads is mentioned. This data are obviously not present in the DWG. We assume that the documentation is wrong.

Two Unknown Bytes in MultiLeader

Between 291 Enable Dogleg and 41 Dogleg Length we must call Advance(2).

mme1950 · 2023-10-30T15:54:11Z

Hi @DomCR,

final changes (hopefully):

removed all enum values from /// comments, except where only a subset is mentioned in the documentation.
fixed formatting (opening brace on new line).
removed analyse methods.
readMultiLeader and readMultiLeaderStyle return null when version is older than R2010.

regards
Matthias

mme1950 · 2023-11-02T22:17:15Z

Hi @DomCR,

please commit this PR if the code is OK for you now.

regards
Matthias

DomCR · 2023-11-03T07:50:12Z

ACadSharp/Color.cs

I think this file should not be modified, please revert the changes.

DomCR · 2023-11-03T07:51:53Z

ACadSharp/Entities/MultiLeader.cs

+
+		/// <summary>
+		/// Text Left Attachment Type
+		/// TODO Property Name


Please remove this line

DomCR · 2023-11-03T07:53:07Z

ACadSharp/Entities/MultiLeader.cs

+		[DxfCodeValue(173)]
+		public TextAttachmentType TextLeftAttachment { get; set; }
+
+		//  TODO Property Name


Please remove this line

DomCR · 2023-11-03T07:55:28Z

ACadSharp/Entities/MultiLeader.cs

+		#region Block Content Properties
+
+		/// <summary>
+		/// Block Content ID (optional) Type = BlockRecord


Is not needed to specify the type as Type = BlockRecord and the optional flag you can add the DxfReferenceType as a parameter in the DxfCodeValue attribute

DomCR · 2023-11-03T07:57:02Z

ACadSharp/IO/DWG/DwgStreamReaders/DwgMergedReader.cs

 		}

 		public void Advance(int offset)
 		{
-			throw new InvalidOperationException();
+			_mainReader.Advance(offset);


I think that this is the only line that should be modified in this file

I agree, we did not change anything else in this file.

DomCR · 2023-11-03T08:05:59Z

ACadSharp/IO/DWG/DwgStreamReaders/DwgObjectReader.cs

@@ -1206,12 +1212,27 @@ private void readCommonAttData(AttributeBase att)

 			switch (att.AttributeType)
 			{
+				case AttributeType.SingleLine:


the method readCommonAttData(AttributeBase att) should not change

DomCR · 2023-11-03T08:06:41Z

ACadSharp/IO/DWG/DwgStreamReaders/DwgObjectReader.cs

-				//numentries handles in the file (soft owner)
+				//Handle refs H NULL(soft pointer)
+				//xdicobjhandle(hard owner)
+				//the apps(soft owner)


readDocumentTable<T> should not change

DomCR · 2023-11-03T08:07:05Z

ACadSharp/IO/DWG/DwgStreamReaders/DwgObjectReader.cs

@@ -1393,7 +1408,7 @@ private void readInsertCommonData(CadInsertTemplate template)
 			template.OwnedObjectsCount = 0;

 			//R2004+:
-			if (this.R2004Plus && template.HasAtts)
+			if (this.R2004Plus & template.HasAtts)


readInsertCommonData(CadInsertTemplate template) should not change

DomCR · 2023-11-03T08:07:24Z

ACadSharp/IO/DWG/DwgStreamReaders/DwgObjectReader.cs

@@ -1562,15 +1577,12 @@ private CadTemplate readPolyline3D()
 			this.readCommonEntityData(template);

 			//Flags RC 70 NOT DIRECTLY THE 75. Bit-coded (76543210):
-			byte flags = this._objectReader.ReadByte();
-
+			byte num1 = this._objectReader.ReadByte();


readPolyline3D() should not change

DomCR · 2023-11-03T08:09:37Z

ACadSharp/IO/DWG/DwgStreamReaders/DwgObjectReader.cs

There are a lot of changes in methods outside the scope for this file, I think is due some merge conflicts.

Please check the changes, this file should only be changes for the MULTILEADER and MULTILEADERSTYLE implementation methods

DomCR · 2023-11-03T08:13:25Z

Great Job with this one!! looks really good!!

I've reviewed the PR, most of the comments are related to the documentation or fix some merging errors that you may have had during the development, I'll try to block any PRs that mess with you files until this is merge into master.

DomCR · 2023-11-03T08:18:36Z

There are some test failing:

Failed ACadSharp.Tests.ColorTests: this are due the merging errors, once you restore those classes you will be good.
Failed ACadSharp.Tests.IO.IOTests: this test is using the DwgWriter and DxfWriter for testing, the ACadSharp.IO.DXF.DxfSectionWriterBase.writeEntity and DwgObjectWriter.Entities are throwing an exception for your new entities, please add them into the ignore list so this exception is avoided.

DomCR · 2023-11-03T08:19:47Z

If you need a hand resolving this issues, please let me know and I'll commit the changes directly to this PR.

mme1950 · 2023-11-03T17:36:17Z

Hi @DomCR,

we hopefully fixed the merge conflicts and changed the comments as you requested.
However the comments are in general very poor. I will improve them later when we learnt how properties specified in AutoCAD arrive in the data.

regards
Matthias

DomCR

The PR looks good but there is still a problem with the clone for ACadSharp.Entities.MultiLeader I'll take a look and see if I can help

DomCR · 2023-11-03T18:00:27Z

Here is a fix for the ACadSharp.Entities.MultiLeader.Clone() error:

		public override CadObject Clone()
		{
			MultiLeader clone = (MultiLeader)base.Clone();

			clone.ContextData = (MultiLeaderAnnotContext)this.ContextData?.Clone();

			foreach (var att in BlockAttributes)
			{
				clone.BlockAttributes.Add(att);
			}

			return clone;
		}

mme1950 · 2023-11-03T18:08:44Z

Oh.

DomCR · 2023-11-03T18:10:56Z

Oh.

Sorry I wanted to try if I was able to push directly here and make the merge if the test passed.

DomCR · 2023-11-03T18:14:01Z

Great job!!

mme1950 · 2023-11-03T19:34:26Z

Hi @DomCR

So you made the change yourself and merged everythng, Thank you wery much.
We will now concentrate on our DWG-to-SVG converter and so intensively test the multileder reader.
Then we shall implement the DXF-reader and the writers.

Again thank you very much for your help and the review. It is great to contribute to such an active project.
Greetings from Berlin to Barcelona.

Nihad Karajko and others added 2 commits September 15, 2023 17:52

Implement Multileader

43614d7

Merge pull request #5 from mme1950/20230915_mme_Multileader

4d25097

Implement Multileader

mme1950 marked this pull request as ready for review September 25, 2023 00:10

DomCR linked an issue Sep 28, 2023 that may be closed by this pull request

Help needed: How to read substructures (e. g. CONTEXT_DATA in MULTILEADERSTYLE) #144

Closed

DomCR added the feature New feature added label Sep 28, 2023

DomCR self-requested a review September 28, 2023 16:02

DomCR reviewed Sep 28, 2023

View reviewed changes

Nihad Karajko added 2 commits September 29, 2023 17:33

Update Multileader Summary and DwgMergedReader Advance Method

15b59ba

Update from Original Repository

b93c5b4

Nihad Karajko and others added 13 commits October 4, 2023 15:16

Draw Multileader to sample_AC1032.dwg

24b4f8f

Improve Clone Function for Multileader

1e827f7

Styles do not need to be cloned

24fc426

- some refactoring

06f7112

Change Arrowhead Data Type for BlockRecord

1bd0fe8

Replace BlockRecord with ContentsBlock

68d8ed0

Add SubclassMarker for Multileader

318a9f6

Add SubclassMarker for MLeaderStyle

ec01a04

Add Constructor for MultiLeaderStyle

ca176e4

Add Constructor for MultiLeaderAnnotContext and Clone BlockAttributes…

0f36506

… at MultiLeader

Implement Missing Clone Methods

99e190f

Merge branch 'master' into master

d14b04e

Improve MultiLeader

38987f9

DomCR reviewed Oct 16, 2023

View reviewed changes

Merge branch 'master' into master

b729831

Corrections

8e1a0e6

Nihad Karajko added 3 commits October 31, 2023 10:45

Fix MultiLeader Clone

8c2b328

Add MultiLeader Sample DWG

b1ded16

Implement Asserts for MultiLeader

b6c5987

DomCR requested changes Nov 3, 2023

View reviewed changes

Nihad Karajko added 6 commits November 3, 2023 10:56

Corrections

ac6fa2d

Fix Color Tests and Add MultiLeader to Ignore List

759b778

Update Comments

9a63d0b

- comments improved

b3f82d2

Baseline Merge, Comments Improved

50f57b6

Fix DwgObjectReader

e60a94a

DomCR reviewed Nov 3, 2023

View reviewed changes

Update MultiLeader.cs

f5fb8e0

DomCR approved these changes Nov 3, 2023

View reviewed changes

DomCR merged commit f0504fa into DomCR:master Nov 3, 2023
2 checks passed



		/// <summary>
		/// TODO Do we need this class to represent a list of arrow heads

Implementation of MULTILEADER and MULTILEADERSTYLE #169

Implementation of MULTILEADER and MULTILEADERSTYLE #169

Conversation

mme1950 commented Sep 15, 2023

DomCR commented Sep 28, 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DomCR commented Sep 28, 2023

mme1950 commented Sep 28, 2023

mme1950 commented Sep 29, 2023

DomCR commented Oct 1, 2023

mme1950 commented Oct 2, 2023

DomCR commented Oct 4, 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

Choose a reason for hiding this comment

mme1950 commented Oct 30, 2023

DomCR commented Oct 30, 2023

mme1950 commented Oct 30, 2023

Object Model

Remarks

Open Issues

Version Compliance

List of Arrowheads

Two Unknown Bytes in MultiLeader

mme1950 commented Oct 30, 2023

mme1950 commented Nov 2, 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

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

Choose a reason for hiding this comment

DomCR commented Nov 3, 2023

DomCR commented Nov 3, 2023

DomCR commented Nov 3, 2023

mme1950 commented Nov 3, 2023

DomCR left a comment

Choose a reason for hiding this comment

DomCR commented Nov 3, 2023

mme1950 commented Nov 3, 2023

DomCR commented Nov 3, 2023

DomCR commented Nov 3, 2023

mme1950 commented Nov 3, 2023 • edited Loading

mme1950 commented Nov 3, 2023 •

edited

Loading