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

Replace GeometryStream with GeometryMap. #36

Open
wants to merge 2 commits into
base: core/GeometryDetailRecord
Choose a base branch
from

Conversation

GerardasB
Copy link

This PR suggests to change GeometryStream property of GeometryDetailRecord element to GeometryMap.
The main reason for the change is - possibility of huge number of elements and resulting performance issues.

There are some major complications when using GeometryMap, instead of i.e. using relationships to group related geometries.

  1. Geometry of GeometryMap is not an Element. I.e. additional properties need to be serialized.
  2. GeometryMap entry id's are not unique in the iModel - i.e. to reference a geometry in a GeometryMap one needs to provide entry id of GeometryMap AND element id of GeometryDetailRecord. Also managing such relationships.

Suggested FlatBuffer schema for GeometryMap:

table GeometryMap {
  entries: [GeometryMapEntry];
}

table GeometryMapEntry {
  id: int;
  entries: [ElementGeometryDataEntry];
}

table ElementGeometryDataEntry {
  opcode: int;
  data: [ubyte];
}

@@ -566,14 +565,14 @@

<ECEntityClass typeName="GeometryDetailRecord" modifier="Abstract" displayLabel="Geometry Detail Record" description="An abstract InformationRecordElement that holds geometry for use by the 'feature tree' stored in the `GeometricElement3d.GeometryOperations` property of its parent Element.">
<BaseClass>InformationRecordElement</BaseClass>
<ECProperty propertyName="GeometryStream" typeName="binary" extendedTypeName="GeometryStream" displayLabel="Geometry Stream" description="Binary stream used to persist the geometry of this bis:Element.">
<ECProperty propertyName="GeometryMap" typeName="binary" extendedTypeName="GeometryMap" displayLabel="Geometry Map" description="Binary stream used to persist the geometries of this bis:Element.">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has Brien's code to work with GeometryDetailRecords also been updated to work with GeometryMap instead of GeometryStream?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bbastings Looks like there won't be any issues to update the code if/when we decide to merge that branch.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree.

@@ -566,14 +565,14 @@

<ECEntityClass typeName="GeometryDetailRecord" modifier="Abstract" displayLabel="Geometry Detail Record" description="An abstract InformationRecordElement that holds geometry for use by the 'feature tree' stored in the `GeometricElement3d.GeometryOperations` property of its parent Element.">
<BaseClass>InformationRecordElement</BaseClass>
<ECProperty propertyName="GeometryStream" typeName="binary" extendedTypeName="GeometryStream" displayLabel="Geometry Stream" description="Binary stream used to persist the geometry of this bis:Element.">
<ECProperty propertyName="GeometryMap" typeName="binary" extendedTypeName="GeometryMap" displayLabel="Geometry Map" description="Binary stream used to persist the geometries of this bis:Element.">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need a .remarks.md file documenting the format or linking to its documentation. Should also link to docs for the API for working with this stuff.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the remarks, the API is not published to public registry yet.

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

Successfully merging this pull request may close these issues.

None yet

3 participants