Start parsing EmfPlusPen, EmfPlusBrush, and EmfPlusDrawlines #4
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hey @willibrandon hope you've been well. Checking if you had any more progress with this. |
|
@willibrandon So sorry I missed this. I was on an extended vacation. 🙏 |
JeremyKuhne
requested changes
Feb 17, 2021
|
|
||
| if((_penDataFlags & flag) != 0) | ||
| { | ||
| if ((_penDataFlags & PenDataFlags.PenDataTransform) != 0) |
There was a problem hiding this comment.
Minor nit- .HasFlag() is easier to read and is fully optimized by the JIT now.
| // TODO: Refactor this. | ||
| private unsafe int GetOptionalDataOffset(PenDataFlags flag) | ||
| { | ||
| int offset = 8; |
| private readonly byte _data; | ||
|
|
||
| // TODO: Refactor this. | ||
| private unsafe int GetOptionalDataOffset(PenDataFlags flag) |
There was a problem hiding this comment.
It would probably be clearer if you have this method return the pointer to the data or null if the given option doesn't exist. private unsafe byte* GetOptionalData(PenDataFlags flag).
| return offset; | ||
|
|
||
| if ((_penDataFlags & PenDataFlags.PenDataStartCap) != 0) | ||
| offset += sizeof(uint); |
There was a problem hiding this comment.
Use sizeof(LineCap) to be clearer (presuming the enum type is the right size).
| [StructLayout(LayoutKind.Sequential)] | ||
| public readonly ref struct MetafilePlusPenData | ||
| { | ||
| private readonly byte _data; |
There was a problem hiding this comment.
Better here to define the fixed data as a normal struct.
public readonly ref struct MetafilePlusPenData
{
public PenDataFlags PenDataFlags;
public UnitType PenUnit;
public float PenWidth;
public MetafilePlusPenOptionalData* OptionalData
{
// This needs to be fixed- omitting for clarity
get => (MetafilePlusPenOptionalData*)((byte*)&PenDataFlags + sizeof(MetafilePlusPenData));
// If this struct was _not_ a ref struct you can avoid the fixed statements using Unsafe.AsPointer
get => (MetafilePlusPenOptionalData*)((byte*)Unsafe.AsPointer(ref this) + sizeof(MetafilePlusPenData));
}
}
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is in regards to dotnet/winforms#4341
I've started enumerating and parsing the GDI+ records using the EMF+ specification and would greatly appreciate your feedback when you have a moment. Specifically I'm looking for some feedback on how I'm walking the byte pointer and casting to the structures, and whether or not there is a better way to do this. I haven't used pointers in this fashion before and feel like there is room for improvement here.
In the meantime, I'll start looking at enumerating and parsing the same data via the equivalent callback.