-
Notifications
You must be signed in to change notification settings - Fork 316
Migrate TileMesh-related modules to typescript #2557
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
base: master
Are you sure you want to change the base?
Conversation
3fa4d5a
to
e9074d0
Compare
fb2a1f9
to
cc1767f
Compare
This PR introduces 3 new TODOs (and removes 1, thanks for that!). I also noticed a question added in a comment in |
packages/Main/src/Core/TileMesh.ts
Outdated
return tile; | ||
} else if (tile.level != 0) { | ||
// @ts-expect-error By invariant, parent is always a TileMesh | ||
return this.parent.findCommonAncestor(tile.parent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would a cast be better than a ts-expect-error
?
(this.parent as TileMesh).findCommonAncestor(tile.parent as TileMesh)
(The comment is still useful anyway.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but I think our linter does not authorize such cast, but I will do it either way (with a @ts-expect-error)
* labels to display from its data. For example, it needs to be set to `true` | ||
* for a layer with vector tiles. If it's `true` a new `LabelLayer` is added and attached to this `Layer`. | ||
* You can also configure it with {@link LabelLayer} options described below such as: `addLabelLayer: { performance: true }`. | ||
* @param {boolean} [config.addLabelLayer.performance=false] - In case label layer adding, so remove labels that have no chance of being visible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The language is quite difficult to understand here. If I understood well, it could be:
"In case of adding a label layer, remove labels that have no chance of being visible.
Even in the best case, labels will never be displayed, for example, if there are many labels."
const dimensions = new THREE.Vector2(); | ||
|
||
function setTileFromTiledLayer(tile, tileLayer) { | ||
interface TileLayerLike { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a simplified, POD version of TiledGeometryLayer
? If so, we could mention it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since TiledGeometryLayer
is not typed yet, I decided to go for an sub-type for this function.
63ebaa4
to
5df9326
Compare
@Neptilo I did all changes following your review. =) |
Nice! And what do you think about my previous comment?
Currently I can see two added TODOs in
|
My bad, I forgot about those comments! |
For the For |
f67a7e4
to
1086345
Compare
6944e3f
to
7fcd096
Compare
Description
This PR introduces TypeScript migration of the whole tile conversion process. This includes
TileMesh
,OBB
andconvertToTile
.Depends on #2556.