Simplification of the loading process for rasterized tiles and mesh tiles #2609
Simplification of the loading process for rasterized tiles and mesh tiles #2609gchoqueux wants to merge 7 commits intoiTowns:masterfrom
Conversation
489581c to
afdfc01
Compare
|
Rebased |
|
I’ve come up with a possible way to simplify this — what do you think of these changes? |
afdfc01 to
6f64fdf
Compare
2cec783 to
15dfa04
Compare
|
c7b4635 to
4bd5c32
Compare
|
|
e9aab85 to
3d3be67
Compare
eecc503 to
4c86123
Compare
5235375 to
7a3ea9a
Compare
|
Hi! I'm starting to look into the loading processes to get familiar with them, in order to improve them after this simplification is completed. Thanks for the work done so far! |
7a07fe7 to
47507b0
Compare
Remaining work in this PR
FAQ
I don't remember exactly i remove the commentary
I think it's not a bug
|
Next steps to move forward on the PR
Assistance can be provided on the following points:
|
47507b0 to
5b6ea96
Compare
92e8c4c to
a9618c4
Compare
a9618c4 to
c581383
Compare
Desplandis
left a comment
There was a problem hiding this comment.
The proposed changes are fine by me. It makes the code more easy to understand (e.g. no need to track the changes of layerUpdateState across 4 different files). I did some high-level comments on the code (and some low-level too sorry), those are some questions about the behaviour before and after this PR.
Note that I did not tested it across our examples, nor did I run the tests.
Could you please provide a list of the remaining things to address in this PR?
| if (geometry && !geometry.index) { | ||
| throw new Error('Geometry found in cache without index buffer'); | ||
| } |
There was a problem hiding this comment.
Is this really possible to have a geometry without index here?
There was a problem hiding this comment.
no, i remove this test
| const buffers = computeBuffers( | ||
| builder, | ||
| { ...params, center }, | ||
| cachedBuffers !== undefined | ||
| ? { | ||
| index: cachedBuffers.index.array as | ||
| Uint8Array | Uint16Array | Uint32Array, | ||
| uv: cachedBuffers.uv.array as Float32Array, | ||
| } | ||
| : undefined, | ||
| ); | ||
|
|
There was a problem hiding this comment.
It seems that this code could fail before this PR, are those exceptions catched somewhere?
There was a problem hiding this comment.
I not sure there are failures
| } | ||
| } | ||
|
|
||
| export class TileMatrixSetLimits { |
There was a problem hiding this comment.
This greatly simplifies the code in TMSSource!
In the future (maybe for the next hackaton), I will surely try to add a tile grid module similar to openlayer's TileGrid which will incorporates your changes here.
There was a problem hiding this comment.
yes I add news features in TileMatrixSetLimits
|
|
||
| update(context, layer, node, parent) { | ||
| return updateLayeredMaterialNodeElevation(context, this, node, parent); | ||
| anyVisibleData(node) { |
There was a problem hiding this comment.
We should find a better name. x)
| @@ -1,4 +1,4 @@ | |||
| import { EMPTY_TEXTURE_ZOOM } from 'Renderer/RasterTile'; | |||
| import { EMPTY_TEXTURE_ZOOM, RasterTile } from 'Renderer/RasterTile'; | |||
There was a problem hiding this comment.
| import { EMPTY_TEXTURE_ZOOM, RasterTile } from 'Renderer/RasterTile'; | |
| import { EMPTY_TEXTURE_ZOOM, type RasterTile } from 'Renderer/RasterTile'; |
| } | ||
|
|
||
| initFromParent(parent, extents) { | ||
| load(requester, view) { |
There was a problem hiding this comment.
Similarly to load, is there a method to RasterTile to "dispose" the textures?
| this.disposeRedrawnTextures(textures); | ||
| for (let i = 0, il = textures.length; i < il; ++i) { | ||
| this.setTexture(i, textures[i], pitchs[i]); | ||
| this.setTexture(i, textures[i], this.tiles[i].offsetToParent(textures[i].extent)); |
There was a problem hiding this comment.
So pitches/offsetScales parameters were always offsetToParent of texture extent?
| // [TEMP] find best place to update uniforms | ||
| node.material.layersNeedUpdate = node.material.visible; |
There was a problem hiding this comment.
This is similar to the old behaviour, but I agree that we should reconsider where we should request a new update of the material!
|
|
||
| update(context, layer, node) { | ||
| if (layer.visible && !layer.freeze && this.anyVisibleData(node)) { | ||
| const rasterTile = node.material.getTile(this.id) || this.setupRasterNode(node); |
There was a problem hiding this comment.
So setupRasterNode is an abstract method of RasterLayer
| anyVisibleData(node) { | ||
| const current = node.material.getElevationTile()?.layer; | ||
| return super.anyVisibleData(node) | ||
| && (current?.source == this.source || this.source.zoom.min > (current?.source.zoom.max || -1)); |
There was a problem hiding this comment.
What is the meaning of -1 here?
There was a problem hiding this comment.
it's for this.source.zoom.min > -1 would be always true
Description
This PR proposes a possible simplification of the loading process for rasterized tiles and mesh tiles.
Summary of changes
TileMeshinstances are now created directly inTiledGeometryLayer, without using commands or providers.TileMesh.pendingSubdivisionhas been removed.TiledGeometryLayer#hasEnoughTexturesToSubdividehas been removed.updateLayeredMaterialNodeImageryandupdateLayeredMaterialNodeElevationhave been merged into a single lightweight method.TileProviderhas been deleted.RasterTile#load.