Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Bounding box: calculation of MinMax values and add clamping options #1993

Merged
merged 4 commits into from
Apr 14, 2023

Conversation

ftoromanoff
Copy link
Contributor

@ftoromanoff ftoromanoff commented Jan 27, 2023

Description

I changed the way the NodataValue is taken into account for the computation of the MinMax for the bounding box and also added an option to add clamping min and max value when creating a new layer.

Motivation and Context

There was a probleme with the segmentation of tile in relation with abnormal values in particular on see area.
issue #1945
It should also solve the problem seen on the issue #1981

Screenshots (if appropriate)

@mgermerie mgermerie marked this pull request as draft January 27, 2023 16:28
@ftoromanoff ftoromanoff changed the title WIP Bathy WIP Bounding box and calculation of MinMax values Feb 1, 2023
@ftoromanoff ftoromanoff force-pushed the bathy branch 8 times, most recently from 68f82fc to 67debc5 Compare February 8, 2023 14:44
@ftoromanoff ftoromanoff changed the title WIP Bounding box and calculation of MinMax values WIP Bounding box: calculation of MinMax values and add clamping options Feb 8, 2023
@ftoromanoff ftoromanoff marked this pull request as ready for review February 9, 2023 13:08
@ftoromanoff ftoromanoff changed the title WIP Bounding box: calculation of MinMax values and add clamping options Bounding box: calculation of MinMax values and add clamping options Feb 9, 2023
@mgermerie mgermerie requested review from mgermerie and jailln February 9, 2023 13:08
@mgermerie mgermerie requested a review from gchoqueux February 9, 2023 13:18
Copy link
Contributor

@jailln jailln left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR! Except from my comments, don't forget to rename your commit with the angular convention :)

examples/view_3d_map.html Show resolved Hide resolved
src/Layer/ElevationLayer.js Outdated Show resolved Hide resolved
src/Layer/ElevationLayer.js Show resolved Hide resolved
@ftoromanoff ftoromanoff force-pushed the bathy branch 3 times, most recently from c8b6678 to 67fcdfe Compare February 10, 2023 11:13
itowns.Fetcher.json('./layers/JSONLayers/IGN_MNS_HIGHRES.json').then(addElevationLayerFromConfig);
// itowns.Fetcher.json('./layers/JSONLayers/IGN_MNT.json').then(addElevationLayerFromConfig);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't be commented out ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a probleme with the dataset linked with this configuration file or with the configuration file (IGN_MNT.json), that appear with the new changes. As it is only used in this example and the extra Elevation layer doesn't really seems to be important. I prefered to removed it and it can be eluded when the issue #2000 will be studied.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, could you describe this problem in #2000 please so we don't forget about it ?

Comment on lines -80 to +81
itowns.Fetcher.json('./layers/JSONLayers/WORLD_DTM.json').then(addElevationLayerFromConfig);
itowns.Fetcher.json('./layers/JSONLayers/IGN_MNT_HIGHRES.json').then(addElevationLayerFromConfig);
itowns.Fetcher.json('./layers/JSONLayers/WORLD_DTM.json').then(addElevationLayerFromConfig);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's issue #2000 ?

Comment on lines +45 to +46
* @param {number} [config.clampValues.min] The minimum value to clamp the elevation
* @param {number} [config.clampValues.max] The maximum value to clamp the elevation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you rename to make the parameters more understandable?
you keep zmin and zmax name for the layer properties?

Comment on lines +31 to +32
* @param {number} [options.zmin] The minimum elevation value after which it will be clamped
* @param {number} [options.zmax] The maximum elevation value after which it will be clamped
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you homogenize with clampValues name?

Comment on lines 40 to 42
if (d < layer.zmin) d = layer.zmin;
if (d > layer.zmax) d = layer.zmax;
if (d == layer.noDataValue) d = 0.;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's a clamp method in glsl

it seems to me that d is an interpolated value, the filter nodatavalue will not work here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it with the clamp method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still want the nodata value to be interpreted as an elevation of zero in the texture. Can we check that together ?

@ftoromanoff ftoromanoff force-pushed the bathy branch 2 times, most recently from a3dc15d to a4989ff Compare March 30, 2023 12:54
@mgermerie mgermerie merged commit c6800c9 into iTowns:master Apr 14, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong bounding box computing for negative elevation data
4 participants