fix(3d-tiles): Rewrite of parseImplicitTiles for readability #3086
Conversation
There was a problem hiding this comment.
Thank you for this fix @olsen232 .
Regarding #3085 (comment) : you can build code on linux or mac os. Use wsl on Windows. You have a ts error right now.
We will check this fix on the tile-converter.
fixes visgl#3085 Adds comments.
olsen232
force-pushed
the
fix-implicit-tile-parsing
branch
from
fc8adfd
to
7538b19
Compare
There was a problem hiding this comment.
@olsen232 Appreciate the contribution. the changes are fine from a high-level perspective, since they are localized inside existing function.
I will rely on @belom88 for the detailed review of the logic.
Do we have test cases for this? I think we need at least something to make sure future maintainers can catch regressions.
Not sure why our contributing page is not part of the official docs:
Line 4 in eb08437
| ]; | ||
| let childMinimumHeight: number; | ||
| let childMaximumHeight: number; | ||
| if (subdivisionScheme === 'OCTREE') { |
There was a problem hiding this comment.
Nit: prefer a switch statement since this is not a boolean.
|
Tested on a dataset with quadtree implicit tiles (CDB_Yemen_elevation) |
belom88
changed the title
Fixes #3085.
It's possible to fix that issue with smaller changes, but I believe I found and fixed some more bugs by rewriting it for clarity.
Adds comments.