Skip to content

fix(grid-reader): read grid guid - #304

Merged
agviegas merged 1 commit into
ThatOpen:mainfrom
ShaMan123:fix/grid-guid
Sep 25, 2026
Merged

agviegas merged 1 commit into
ThatOpen:mainfrom
ShaMan123:fix/grid-guid

Conversation

@ShaMan123

Copy link
Copy Markdown
Contributor

Description

Read grid guid when parsing ifc

@ShaMan123 ShaMan123 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

should the guid accessor be guarded?

@ShaMan123

Copy link
Copy Markdown
Contributor Author

I decided not to guard following the rest of the accessors in the parser

agviegas added a commit that referenced this pull request Sep 25, 2026
…obalId

GridsController rebuilds GridData with JSON.parse from the data attribute
of already-exported .frag files, which carry no guid, so a required field
would misdescribe every model exported before #304. web-ifc 0.0.77 also
returns a wrapper with value null for an IFCGRID whose GlobalId is unset
(probed directly), which would otherwise serialize null into every grid.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@agviegas
agviegas merged commit b5726f6 into ThatOpen:main Sep 25, 2026
1 check passed
@agviegas

Copy link
Copy Markdown
Contributor

Merged, and your call about not guarding was right: we probed web-ifc 0.0.77 directly and an IFCGRID with an unset GlobalId comes back as a wrapper with value: null, so the read never throws and the surrounding per-grid catch never fires.

The problem is what that null does afterwards: it gets serialized into every grid. So we amended with grid.GlobalId?.value ?? undefined (the optional chain is cheap insurance, the coalesce is the part that matters) and made guid optional in GridData, which it had to be anyway: GridsController rebuilds that type with JSON.parse from already-exported .frag files, which carry no guid at all, so a required field would misdescribe every model exported before this change. Added a regression test with a guidless-grid fixture. Thanks!

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.

2 participants