-
Notifications
You must be signed in to change notification settings - Fork 290
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
feat(TextureSize): Add parameter to define the textureSize #2297
base: master
Are you sure you want to change the base?
Conversation
edffdd6
to
4734a37
Compare
src/Layer/Layer.js
Outdated
@@ -108,6 +108,8 @@ class Layer extends THREE.EventDispatcher { | |||
config.style = new Style(config.style); | |||
} | |||
this.style = config.style || new Style(); | |||
this.sizeTextureTile = config.sizeTextureTile || 256; |
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.
This seems more "source-dependent" than "layer-dependent" because it is related to the source data. I would rather define this in Source
. WDYT @Desplandis ?
Then, we should default it to 512
for VectorTileSource
.
Also, either here or in Source
, can you document it in the public API please?
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.
@jailln This seem to be what openlayers does (see https://openlayers.org/en/latest/apidoc/module-ol_source_VectorTile-VectorTile.html).
However, we rasterize the vector data in itowns in the layer (i.e. convert
), so it could be a parameter of the layer.
We'll keep it in the layer because we have access to the globe layer from there and then we can affect the tile size for the subdivision. |
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.
This PR is good to merge in my opinion except for one thing : the name of the parameter.
It think sizeTextureTile
(which would sound more correct as tileTextureSize
) refers only to the size of a texture. This is not what it describes in one of its two usecases : the tile size above which a globe tile is subdivided.
Perhaps we could benefit from this PR to change the name inherited from the old constant SIZE_TEXTURE_TILE
to something like subdivisionThreshold
or any other suggestion ?
I agree, the name should be changed. |
Related issue:
#2292
#2312
#2309
Replace the const use for subdivision and texture conversion with a customizable parameter
sizeTextureTile
Example:
For MVT layer
For the globe:
const view = new itowns.GlobeView(viewerDiv, placement, {sizeTextureTile:512});