ACadSharp
ACadSharp copied to clipboard
M-Text Value Reader Writer
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
Going to do some dogfooding on this for a bit to see if there are any other issues.
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.
As a generic comment I would also suggest to add the interface IEquatable
to those classes that override Equals
and GetHashCode
.
Sorry for the delay but this is a quite big PR and it will take some time for me to review it all.
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.
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 overrideEquals
andGetHashCode
.
Will do. Good practice.
Updated per comments. Ready for any additional comments.
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.
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.
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.
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.
Slash being included where it should not be.
Viewer
CAD
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.