Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

AnthonyGlt
Copy link
Contributor

@AnthonyGlt AnthonyGlt commented Mar 18, 2024

Related issue:

#2292
#2312
#2309

Replace the const use for subdivision and texture conversion with a customizable parameter sizeTextureTile

Example:
For MVT layer

 const mvtSource = new itowns.VectorTilesSource({
              style: 'https://data.geopf.fr/annexes/ressources/vectorTiles/styles/PLAN.IGN/standard.json',
              crs: 'EPSG:3857',

          });

  const mvtLayer = new itowns.ColorLayer('mvtLayer',{
      source: mvtSource,
      sizeTextureTile:512,
      addLabelLayer: true,

  });

For the globe:

const view = new itowns.GlobeView(viewerDiv, placement, {sizeTextureTile:512});

@@ -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;
Copy link
Contributor

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?

Copy link
Contributor

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.

@AnthonyGlt AnthonyGlt changed the title fix(TextureSize): Difference in representation of MVT between iTowns and OpenLayer fix(TextureSize): Add parameter to define the textureSize Mar 28, 2024
@AnthonyGlt AnthonyGlt marked this pull request as ready for review March 29, 2024 08:36
@AnthonyGlt AnthonyGlt changed the title fix(TextureSize): Add parameter to define the textureSize feat(TextureSize): Add parameter to define the textureSize Mar 29, 2024
@Desplandis Desplandis self-requested a review April 18, 2024 14:29
@AnthonyGlt
Copy link
Contributor Author

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.
(TODO put a comment in the code)

Copy link
Contributor

@mgermerie mgermerie left a 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 ?

@AnthonyGlt
Copy link
Contributor Author

I agree, the name should be changed. subdivisionThreshold is more appropriate but there is sseSubdivisionThreshold already existing.

@AnthonyGlt AnthonyGlt requested a review from mgermerie May 23, 2024 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants