Skip to content
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

DxfLwPolyline not correctly saved and loaded #87

Open
lzl1918 opened this issue May 21, 2018 · 7 comments
Open

DxfLwPolyline not correctly saved and loaded #87

lzl1918 opened this issue May 21, 2018 · 7 comments

Comments

@lzl1918
Copy link

lzl1918 commented May 21, 2018

FileStream stream = File.Create("out.dxf");
DxfFile dxfFile = new DxfFile();
dxfFile.Entities.Add(new DxfLwPolyline(new List<DxfLwPolyline>()
{
    new DxfLwPolyline() {X = 0, Y = 1},
    new DxfLwPolyline() {X = 1, Y = 1},
    new DxfLwPolyline() {X = 1, Y = 0},
}));
dxfFile.Save(stream);
stream.Flush();
stream.Dispose();

I have codes like above.
By setting breakpoints, there is 1 entity in dxfFile.Entities when dxfFile.Save is called.
However, when the file gets read by calling DxfFile.Load, there is nothing in the entity set.
I have tested that entities of type DxfLine, DxfCircle, DxfArc, and even DxfPolyline can be correctly exported, but entities of type DxfLwPolyline are always missing when load from file streams.

@brettfo
Copy link
Member

brettfo commented May 21, 2018

LWPOLYLINE entities were first supported in AutoCAD R14, but the default value for the file version is R12. The issue is that when saving the file, unsupported entities are simply not written.

The short answer to your problem is this:

DxfFile dxfFile = new DxfFile();
dxfFile.Header.Version = DxfAcadVersion.R14; // <-- add this line

As a longer-term problem, others have suggested that DxfFile.Save() should throw an exception if an unsupported entity is encountered instead of skipping it. What are your thoughts? I'd like to hear what other people would like/expect before I make a potential change.

@lzl1918
Copy link
Author

lzl1918 commented May 22, 2018

What if unsupported entities cause exceptions in debug build, while be simply skipped in release build?

// I have no idea if nuget allows developers to release packages more than one configurations

@brettfo
Copy link
Member

brettfo commented May 23, 2018

As far as I know NuGet doesn't allow the Debug/Release distinction. I also don't want to break anybody by changing .Save() to throw where it didn't before. What are your thoughts on the save method returning a collection of warnings/errors/etc.? E.g.,

Change this:

public void Save(Stream stream)

to this:

public IEnumerable<DxfWarning> Save(Stream stream)

where DxfWarning is either a collection of strings (easy to write, difficult to consume) or a collection of strongly-typed messages like UnsupportedEntity | InsufficientData | etc. (difficult to write, easy to consume)?

That way existing users aren't getting exceptions, but they can optionally check the diagnostics if they like?

I'm also going to tag @nzain on this as he's given me great feedback in the past.

@lzl1918
Copy link
Author

lzl1918 commented May 24, 2018

Maybe in some cases, a save operation needs to be aborted or rolled back when warnings/errors occur.
Thus, in my opinion, such warnings should be accessible during save operations instead of after operations getting completed.

Personally, I have come up with an idea just now that, adding events for DxfFile objects. However, I don't think raising events for such light-weight operations is appropriate. And then the idea changes that override methods of Save can be added which pass delegates to deal warnings and/or errors.

A sample may like:

public enum OperationContinuation 
{
    Continue, 
    Throw,
    RollBack,
}

public delegate OperationContinuation SavingWarningHandler(IEnumerable<DxfWarning> warnings);
public delegate Task<OperationContinuation> SavingWarningAsyncHandler(IEnumerable<DxfWarning> warnings);

public void Save(Stream stream);
public void Save(Stream stream, SavingErrorHandler handler);
public Task SaveAsync(Stream stream, SavingWarningAsyncHandler handler);

@nzain
Copy link
Contributor

nzain commented May 24, 2018

I guess this depends on the use case. In my scenario, I would use the very same fraction of the API (add polylines in different colors/layers) for 99% of my tasks with little/zero user interaction. Once I've learned how to .Save() without exceptions, this would work for all cases and customers (end-users) would not see those exceptions.

If end-users have a deeper interface into the library to possibly create invalid objects that would throw on .Save(), they want to see localized error messages (why?) and the sophisticated enum DxfWarning would win here. Good enum names speak for themselves and the dev should get the idea without a long message.

Something like this

public void Save(Stream stream, Action<DxfWarning> warningCallback);

would give developers a lot of flexibility. I could be lazy and call like this

dxfFile.Save(stream, w => { throw new Exception(w.ToString()); } );

or write to some log file and let the method continue. Or show a ok/cancel popup to the user, let him decide.

@nzain
Copy link
Contributor

nzain commented May 24, 2018

Any callback could potentially throw (intentionally or not) on the consumers side. If the callback throws, this library should not have any resources (stream or similar) open - or close them via finally logic. The latter will leave a broken file! Better: check preconditions before writing, eventually run callbacks, and finally write the file. Probably, this is more work ;-)

@brettfo
Copy link
Member

brettfo commented May 24, 2018

I like the idea of public void Save(Stream stream, Action<DxfWarning> warningCallback). Since the Save() method just takes a stream I won't have to worry about closing any resources because I'm not the one that opened them; that'll be the responsibility of the caller to wrap in a try/finally.

For the initial pass I think I'll just write to the stream and issue the callback whenever necessary and not worry about pre-checking the file; that will get expensive and I don't think there will be much benefit. I'll start working on this as soon as I get a chance.

marcdurham added a commit to marcdurham/Drawer that referenced this issue Jan 19, 2021
Issue was here:
ixmilia/dxf#87
I needed to set a newer version in the header
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants