Skip to content

Conversation

Desplandis
Copy link
Contributor

@Desplandis Desplandis commented Jun 11, 2025

Description

This PR introduces TypeScript migration of the whole tile conversion process. This includes TileMesh, OBB and convertToTile.

Depends on #2556.

@Desplandis Desplandis force-pushed the ts-migration/tiled branch 3 times, most recently from 3fa4d5a to e9074d0 Compare June 12, 2025 19:52
@Desplandis Desplandis force-pushed the ts-migration/tiled branch 3 times, most recently from fb2a1f9 to cc1767f Compare July 1, 2025 09:24
@Desplandis Desplandis marked this pull request as ready for review July 1, 2025 09:50
@jailln jailln requested a review from Neptilo September 9, 2025 14:36
@Neptilo
Copy link
Contributor

Neptilo commented Sep 11, 2025

This PR introduces 3 new TODOs (and removes 1, thanks for that!).
In general, I’d recommend avoiding TODOs in code unless they’re tied to a clear follow-up process (e.g. a linked issue). Otherwise, they often get forgotten and accumulate over time. A good alternative is to either address them before merging, or create an issue and link it directly in the TODO.

I also noticed a question added in a comment in LayerUpdateStrategy. Similar to TODOs, that kind of question is more likely to get answered if raised in chat or as part of an issue.

return tile;
} else if (tile.level != 0) {
// @ts-expect-error By invariant, parent is always a TileMesh
return this.parent.findCommonAncestor(tile.parent);
Copy link
Contributor

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.)

Copy link
Contributor Author

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.
Copy link
Contributor

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 {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@Desplandis Desplandis force-pushed the ts-migration/tiled branch 4 times, most recently from 63ebaa4 to 5df9326 Compare September 25, 2025 11:37
@Desplandis
Copy link
Contributor Author

@Neptilo I did all changes following your review. =)

@Neptilo
Copy link
Contributor

Neptilo commented Sep 25, 2025

@Neptilo I did all changes following your review. =)

Nice!

And what do you think about my previous comment?

This PR introduces 3 new TODOs (and removes 1, thanks for that!). In general, I’d recommend avoiding TODOs in code unless they’re tied to a clear follow-up process (e.g. a linked issue). Otherwise, they often get forgotten and accumulate over time. A good alternative is to either address them before merging, or create an issue and link it directly in the TODO.

Currently I can see two added TODOs in OBB:

  • in updateZ: "why not add the geoid height to the min and max parameters? The implementation of GeoidLayer is leaking here."
  • in setFromExtent: "The crs are hardcoded here."
  • a TODO in disguise ;) : "we should investigate the use of the THREE.OBB class."

@Desplandis
Copy link
Contributor Author

My bad, I forgot about those comments!

@Desplandis
Copy link
Contributor Author

@Neptilo

For the updateZ, it will be fixed when we'll have the possibility to combine multiple elevation layers (additive or as a mask). The geoid layers will be considered as elevation layers instead (and removing this implementation leakage). I can add a short notice to explain this.

For setFromExtent, it will surely not be fixed. Unless we use three OBB implementation (but this is my TODO in disguise ;)). I will remove both comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants