ACadSharp icon indicating copy to clipboard operation
ACadSharp copied to clipboard

M-Text Value Reader Writer

Open DJGosnell opened this issue 2 years ago • 11 comments

M-Text values have their own format data and reading it is not straightforward at times. This PR brings a full parser to allow for a zero copy serializer and deserializer for walking through the values & their formats.

One of the major goals of this is to be a zero copy reader & writer so it makes extensive usage of the Span<char> and Memory<char> structs where possible. This required adding the System.Memory NuGet package for the .NET Framework 4.8 targeting. This package is not used by any of the other targets. The 4.8 target is the only target which copies ranges of the source for parsing of numbers. All the other targets do not copy.

Copying is only done when using the MText.ValueReader.Deserialize method. This is because the it uses the DeserializeWalker method which will re-use spans to eliminate copies, but this at the cost of the parsed tokens being invalid outside of the DeserializeWalker method. The Deserialize method addresses this by copying the values when walking and thus allowing the token usage outside of the DeserializeWalker method.

There are also several compiler directives where parse methods do use the Span<char> structs and the values have to be copied to parse. These only happen on the 4.8 target.

#nullable enable was added to these new file headers for added safety.

Benchmark https://github.com/DJGosnell/ACadSharp/commit/2a7162a618799c2ba567f39b0afceecbf24ce92d

Method Mean Error Gen0 Gen1 Allocated
ReaderDeserializeWalker 30.08 us NA - - -
ReaderDeserialize 37.94 us NA 2.6245 0.1221 22441 B
WriterSerializeWalker 23.28 us NA - - -
WriterSerialize 44.18 us NA 2.0142 0.0610 17192 B

Reader ToDo:

  • [x] Verify against M-Text entities in a variety of real-world values.

Writer ToDo:

  • [x] Create writer which takes the token values and writes out a valid text representation.
  • [x] Add tests

DJGosnell avatar Jan 02 '23 03:01 DJGosnell

Going to do some dogfooding on this for a bit to see if there are any other issues.

DJGosnell avatar Jan 09 '23 21:01 DJGosnell

Finally think this is ready for review.

Also noticed that you're merging the changes instead of squashing & merging. Just wondering if there a reason for this? I would try to keep the commits more clean if I knew this were the case.

DJGosnell avatar Jan 13 '23 18:01 DJGosnell

As a generic comment I would also suggest to add the interface IEquatable to those classes that override Equals and GetHashCode.

DomCR avatar Jan 15 '23 19:01 DomCR

Sorry for the delay but this is a quite big PR and it will take some time for me to review it all.

DomCR avatar Jan 15 '23 19:01 DomCR

Finally think this is ready for review.

Also noticed that you're merging the changes instead of squashing & merging. Just wondering if there a reason for this? I would try to keep the commits more clean if I knew this were the case.

I didn't consider to use squashing & merging, I'll look into it, but don't worry about being clean with the commits is not an issue for me.

DomCR avatar Jan 15 '23 19:01 DomCR

Sorry for the delay but this is a quite big PR and it will take some time for me to review it all.

No need. This is a very big PR and has been on my back log for years to do so no rush.

As a generic comment I would also suggest to add the interface IEquatable to those classes that override Equals and GetHashCode.

Will do. Good practice.

Updated per comments. Ready for any additional comments.

DJGosnell avatar Jan 16 '23 15:01 DJGosnell

I'm going to simplify the integer and float writers. I'm running into too many edge cases where the numbers are rounded incorrectly when written. It will be a hit on the performance, but minimally.

DJGosnell avatar Jan 17 '23 15:01 DJGosnell

Never going to try to write a float to string writer again. Benchmarks

Method Mean Error Gen0 Gen1 Allocated
ReaderDeserializeWalker 25.74 us NA - - 1 B
ReaderDeserialize 32.33 us NA 2.6245 0.1221 22441 B
WriterSerializeWalker 28.40 us NA - - -
WriterSerialize 40.89 us NA 2.0142 0.0610 17192 B

Ready for final review.

DJGosnell avatar Jan 18 '23 18:01 DJGosnell

I left 2 minor comments in the PR, I'm not sure that I grasped the whole implementation but it does not break anything and adds useful features to the project so for me is good to go once the comments are resolved.

This PR also adds some .NET Framework dependencies and differences with .NET, if needed, please open an issue to add the frameworks in the test project.

DomCR avatar Jan 24 '23 15:01 DomCR

I'll address the comments. Thank you for your time to review. I'm thinking that since this is such a big PR, that I'll keep this in draft mode for a while so I can continue dogfooding and work out any edge cases.

DJGosnell avatar Jan 24 '23 20:01 DJGosnell

Slash being included where it should not be.

Viewer image CAD image

DJGosnell avatar Feb 03 '23 15:02 DJGosnell

I opted to make this an internal component in my system since it was only partially working here in this PR. I may split it off and add a separate project to parse and write formatted MText values.

DJGosnell avatar Apr 22 '24 16:04 DJGosnell