Opened 9 months ago

Closed 9 months ago

Last modified 9 months ago

#7106 closed defect (fixed)

DXF: Delay insertion and merging of blocks until actually required

Reported by: Alan Thomas Owned by: warmerdam
Priority: normal Milestone:
Component: OGR_SF Version: svn-trunk
Severity: normal Keywords: dxf
Cc:

Description (last modified by Alan Thomas)

I have a big patch that solves a few different issues with DXF BLOCKs and INSERTs:

Issue 1: Can't insert a block from within another block defined earlier in the BLOCKS table

Suppose the DXF file defines two blocks, block1 and block2. The block1 block contains an INSERT entity that references block2. Because block2 has not been read in when block1's contents are being translated into feature, it doesn't exist from OGR's perspective, so the INSERT is incorrectly omitted.

This patch defers the processing of INSERTs until they actually need to be inlined, either from the Blocks layer, or TranslateINSERT in the Entities layer.

This also allows for more flexibility for merging geometries. Currently blocks are merged by OGRDXFDataSource::ReadBlocksSection, but I'm imagining that, in the future, OGR could implement a flag like DXF_MERGE_COMPLEX_GEOMETRIES allowing the user to obtain DIMENSIONs, LEADERs etc as a single GeometryCollection?. This would require OGR to decide whether to merge the block on-the-fly, which is easily possible with this patch.

Issue 2: The name of the block referred to by an INSERT inside a BLOCK is not preserved

This issue is alluded to in #4369. Suppose DXF_INLINE_BLOCKS and DXF_MERGE_BLOCK_GEOMETRIES are both set to FALSE. Each INSERT entity is translated into a POINT with the BlockName, BlockAngle and BlockScale fields set.

If an INSERT entity exists within a block and you query the Blocks layer, you will find a POINT entity with correct BlockAngle? and BlockScale?, but the BlockName? field has been replaced by the name of the block which contains the INSERT entity, instead of the block it refers to!

The patch fixes this by placing the name of the owning block on a "Block" field for all entities when DXF_INLINE_BLOCKS is FALSE, instead of confusingly reusing "BlockName?". This change also impacts the DXF writer. This is an unavoidable breaking change -- I'm not sure how that is usually handled.

Issue 3: OCS of INSERT entities is not stored when DXF_INLINE_BLOCKS is false

Without the OCS normal vector, there is not enough information for an application to recreate the block insertion properly.

This patch stores the OCS in a new "BlockOCSNormal" field.

The patch also changes the location of an INSERT entity with DXF_INLINE_BLOCKS set to false to be in the OCS... this is less than ideal for end users, but it is essential to preserve proper read/write round-tripping of DXF files. Alternative suggestions would be welcomed. this is no longer the case, I added the "BlockOCSCoords" field to solve this problem.

Issue 4: INSERT entities are incorrectly merged into a GeometryCollection? when DXF_INLINE_BLOCKS is false

Because these POINTs are only meaningful by virtue of the information in their fields, which is lost upon merging, they should not be merged into a GeometryCollection?, in the same way as we don't merge text features.

Attachments (1)

OGR-DXF-insert-refactor.diff (60.5 KB) - added by Alan Thomas 9 months ago.
Patch

Download all attachments as: .zip

Change History (10)

comment:1 Changed 9 months ago by Alan Thomas

A question about this patch:

Fields called BlockName, BlockScale, BlockAngle and BlockOCS are now unconditionally created. The end user will only see data in these fields when DXF_INLINE_BLOCKS is false, but we need to create them unconditionally because the data is used internally by the DXF processor, even when DXF_INLINE_BLOCKS is true. Is there a way to achieve the same functionality without needlessly exposing these fields to the end user?

comment:2 Changed 9 months ago by Alan Thomas

Description: modified (diff)

Changed 9 months ago by Alan Thomas

Patch

comment:3 Changed 9 months ago by Even Rouault

Is there a way to achieve the same functionality without needlessly exposing these fields to the end user?

You could possibly create a OGRDXFFeature class extending OGRFeature that would store BlockName?, etc... as regular C++ members, which would thus be internal to the DXF driver, and and expose them as regular OGR fields only when that makes sense.

If you don't mind, could you create pull requests on the github mirror at https://github.com/OSGeo/gdal ? That way Travis-CI tests will run.

comment:4 Changed 9 months ago by Alan Thomas

See https://github.com/OSGeo/gdal/pull/251. Let me know if you would like me to put a proper commit message.

I have had a go at implementing OGRDXFFeature as you suggested. Unfortunately, the patch is now getting quite large, but I do not think it is possible to split it into multiple commits because all the changes are so heavily intertwined.

comment:5 Changed 9 months ago by Alan Thomas

Description: modified (diff)

comment:6 Changed 9 months ago by Even Rouault

Resolution: fixed
Status: newclosed

In 40548:

DXF: defer inlining of blocks until actually required (patch by Alan Thomas, fixes #7106, https://github.com/OSGeo/gdal/pull/251)

comment:7 Changed 9 months ago by Even Rouault

In 40549:

Commit file that should have gone with previous commit (refs #7106)

comment:8 Changed 9 months ago by Alan Thomas

Do we have release notes or somewhere where we can highlight the breaking change in this patch?

comment:9 in reply to:  8 Changed 9 months ago by Even Rouault

Replying to atlight:

Do we have release notes or somewhere where we can highlight the breaking change in this patch?

You can prepare an entry in the NEWS file

Note: See TracTickets for help on using tickets.