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

Fix PolyfaceAuxData index blocking flatbuffer format / Reorganize test data #6726

Merged
merged 57 commits into from
May 21, 2024

Conversation

dassaf4
Copy link
Member

@dassaf4 dassaf4 commented May 14, 2024

Addresses #6583.

TypeScript FlatBuffer serialization of PolyfaceAuxData indices lacked the 0-blocking of the other Polyface index arrays, which led to crashes during mesh processing. Now TypeScript FlatBuffer serialization outputs the correct index blocking, and both TypeScript and native FlatBuffer APIs read both old and new PolyfaceAuxData index formats. The old format is detected using heuristics.

Although there is a similar imodel-native PR, the fixes in each repo are independent of the other.

Also:

  • Typescript FlatBuffer and JSON deserialization APIs were incorrectly reading fixed-sized Polyface indices, which have numPerFace > 2 and are only written by the native FlatBuffer API. Move index array reblocking logic to SerializationHelpers for common usage.
  • New crossPlatform.test.ts verifies that Typescript API can read native API geometry serializations. (The same test exists in imodel-native to verify the native API can read TypeScript API geometry serializations.) Each test case consists of at least four files: FlatBuffer and JSON serializations of a single geometry, written by both APIs. The test verifies that a given API deserializes each file in a test case into the same geometry. Initial test cases include Polyface variations in numPerFace value, PolyfaceAuxData format, and all combinations thereof.
  • IndexedMeshProps was incomplete and used the wrong label for TaggedNumericDataProps.
  • Add "Props" interfaces for PolyfaceAuxData and use them in the JSON deserializer.
  • Use correct assert(!"foo") idiom
  • Split out index validation from IndexedPolyface.terminateFacet so that it can be called once after all facets are added, instead of after each facet. This new index validation caught at least one bug in a mesh created for a PolyfaceQuery test.
  • Prefer class members of form foo?: bar over foo: bar | undefined
  • PolyfaceData.compress now compresses PolyfaceAuxData if exactly one AuxChannelData is present
  • Sphere ctor now validates latitude sweep to avoid common user error. Sphere.createEllipsoid was erroneously capturing inputs.
  • Add type predicates to Checker.testDefined and Checker.testUndefined to simplify many unit tests. The linter pointed out the simplifications to the tests.
  • GeometryCoreTestIO: add helper functions for tests that perform FlatBuffer/JSON <-> file/GeometryQuery conversions
  • Move all unit test input data into one "data" directory, similar to native geomlibs. Hardcoded data paths in tests are updated.
  • Improve documentation for PolyfaceAuxData. In particular emphasize that the indices must have the same blocking as the other Polyface index arrays.

@dassaf4 dassaf4 enabled auto-merge (squash) May 15, 2024 20:09
Copy link
Contributor

@saeeedtorabi saeeedtorabi left a comment

Choose a reason for hiding this comment

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

I only reviewed the last commit where you extended getPointCurveClosestApproachXYNewton to support LineString. Looks good!

Copy link
Member

@pmconne pmconne left a comment

Choose a reason for hiding this comment

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

Approving but consider putting the massive code reorganization changes into a separate PR or at the very least changing the PR title to describe what's actually being changed.

@dassaf4 dassaf4 changed the title Fix PolyfaceAuxData index blocking flatbuffer format Fix PolyfaceAuxData index blocking flatbuffer format / Reorganize test data May 20, 2024
@dassaf4
Copy link
Member Author

dassaf4 commented May 20, 2024

Approving but consider putting the massive code reorganization changes into a separate PR or at the very least changing the PR title to describe what's actually being changed.

Thanks. Yeah, this one got out of control. Will definitely do next time.

@dassaf4 dassaf4 merged commit a58a868 into master May 21, 2024
16 checks passed
@dassaf4 dassaf4 deleted the da4/fb-auxdata-index-blocking branch May 21, 2024 19:59
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