Elevation measure widget #1881

jailln · 2022-08-09T13:15:29Z

Description

Add a widget to measure elevation anywhere in the 3D scene. Also contains a few modifications of the API (Picking and View).

I will write tests and squash commits once we agree on the implementation :) (i.e. after a first review)

Note that I opened a PR on itowns2-sample-data for the assets related to this widget (sprite, logo and dataset for the example).

Motivation and Context

Implementation of the first tool described in #1868

Screenshots (if appropriate)

Mesure elevation on the terrain:

Mesure elevation on any 3D object:

mgermerie · 2022-10-14T13:06:52Z

There is an issue with the Elevation widget : if you use it while zoomed out, the elevation is not red on the most detailed tile (since it is not necessarily loaded yet). The value will therefore differ on the same point, whether you use the widget while zoomed out or zoomed in at maximum.

mgermerie · 2023-01-18T10:06:50Z

src/Core/View.js

     */
-    pickCoordinates(mouse, target = new Coordinates(this.tileLayer.extent.crs)) {
+    pickTerrainCoordinates(mouse, target = new Coordinates(this.referenceCrs)) {


Renaming this method introduces a breaking change. Could you still define pickCoordinates with a deprecation warning and a calling to the new method ?

Yep and I will also add a breaking change in the commit message.

mgermerie · 2023-01-18T10:23:22Z

src/Core/View.js

-        coordinates.crs = this.referenceCrs;
-        coordinates.setFromVector3(positionVector);
-        coordinates.as(target.crs, target);
+        target.setFromVector3(positionVector);


We need to leave the transformation here, since use should be able to call the method with any target coordinate. We can't assume the CRS of target. For example if I type :

const coordinates = new itowns.Coordinates('EPSG:4326'); view.pickTerrainCoordinates({ /* some mouse position */ }, coordinates);

it will return some coordinates with an EPSG:4326 CRS but whose x, y and z values are intended as EPSG:4978, so most probably not the coordinates under the mouse position.

yep you're right, I got mixed up :)

mgermerie · 2023-01-18T10:27:34Z

src/Core/View.js

     */
-    pickCoordinates(mouse, target = new Coordinates(this.tileLayer.extent.crs)) {
+    pickTerrainCoordinates(mouse, target = new Coordinates(this.referenceCrs)) {


We can indeed change the default target CRS with the view's reference CRS, as it seems more logical.
However we should keep these two following points in mind :

this introduces a breaking change for some users (who for example feed the default output of this method to other processes without transforming it).

In a PlanarView context, I believe that view.referenceCrs and view.tileLayer.extent.crs are always the same. Having the default output CRS to view.tileLayer.extent.crs allows to output coordinates in longitude, latitude and altitude, whose representation is more easy to understand than coordinates in EPSG:4978.

Therefore, do you still think we should change it ? In any case, could you please add the previously missing documentation on the default output CRS ?

I think yes because it is clearer for users since in my opinion users don't really know the crs of the extent of the tileLayer which is quite internal to itowns, they should expect to get coordinates in the view CRS which they actually know of. You're right that it is a breaking change, I will add one.

mgermerie · 2023-01-18T10:33:36Z

src/Core/Geographic/Coordinates.js

     * @param {number|Array<number>|Coordinates|THREE.Vector3} [v0=0] -
     * x or longitude value, or a more complex one: it can be an array of three
     * numbers, being x/lon, x/lat, z/alt, or it can be `THREE.Vector3`. It can
     * also simply be a Coordinates.
     * @param {number} [v1=0] - y or latitude value.
     * @param {number} [v2=0] - z or altitude value.
     */
-    constructor(crs, v0 = 0, v1 = 0, v2 = 0) {
+    constructor(crs = 'EPSG:4978', v0 = 0, v1 = 0, v2 = 0) {


The documentation is a good addition, thanks !
On another hand, I don't think we should assume a default CRS for coordinates. No coordinates make sense without a CRS and in my opinion, user should always have to specify the CRS associated with the coordinates values he is dealing with.

Yes I agree with you, I will remove the default CRS.

mgermerie · 2023-01-18T10:34:15Z

src/Core/Geographic/Coordinates.js

+     * Sets the Coordinate Reference System.
+     * @param {String} crs Coordinate Reference System (e.g. 'EPSG:4978')
+     */
+    setCrs(crs) {


Good idea to add this check of parameter validity !

mgermerie · 2023-01-18T11:06:33Z

src/Core/Picking.js

@@ -187,9 +188,16 @@ export default {
                // if baseId matches objId, the clicked point belongs to `o`
                for (let i = 0; i < candidates.length; i++) {
                    if (candidates[i].objId == o.baseId) {
+                        // Compute distance to the camera
+                        const pointCoordinate = new Coordinates(view.referenceCrs, o.position.x, o.position.y, o.position.z);


I think this method for computing distance could cause issues : the position property for threejs Object3D is expressed in the object's parent coordinates system.
Here, the point could be centered within a Group, and the Group could be positioned at the correct world coordinates. the position of the point would therefore be (0, 0, 0). Using world position for the point, as you did for the camera would prevent this.

Also, do you know of an usage of this picking method other than with PointCloudLayer ?

Yes, you're right, I added the world transformation and updated a bit this part to fix a bug that I had not seen for point cloud picking.

jailln · 2023-01-19T14:51:49Z

@mgermerie I made the modifications you recommended, squashed and rebased.

jailln · 2023-01-20T09:00:07Z

Some functional tests don't pass (timeout), while they pass locally. I tried to update a few things but it didn't change anything. Any ideas @mgermerie @gchoqueux ?

BREAKING CHANGE: View.pickCoordinates has been renamed to View.pickTerrainCoordinates and returns the coordinates in the referenceCrs of the view instead of in the crs of the tiledLayer extent.

…points picking

…3D scene

jailln · 2024-10-04T07:31:59Z

I won't have time to finish this feature in the short term, closing for now.

jailln mentioned this pull request Oct 14, 2022

Picking on TileLayer should return more informations #1037

Closed

3 tasks

jailln force-pushed the feat/elevation-measure branch from 03be1e9 to 6d1178d Compare October 24, 2022 12:54

jailln added wip 🚧 Still being worked on feature 🍏 Adds a new feature widget labels Oct 26, 2022

mgermerie reviewed Jan 18, 2023

View reviewed changes

jailln force-pushed the feat/elevation-measure branch 4 times, most recently from d79c41b to 86ac17f Compare January 19, 2023 14:50

jailln force-pushed the feat/elevation-measure branch from 3cf694e to 86ac17f Compare January 20, 2023 08:59

jailln force-pushed the feat/elevation-measure branch from 86ac17f to 8543d2a Compare February 2, 2023 14:12

jailln added 5 commits February 2, 2023 15:41

fix(picking): fix picking on multiple layers

3026678

refacto(view): rename pickCoordinates to pickTerrainCoordinates

916c479

BREAKING CHANGE: View.pickCoordinates has been renamed to View.pickTerrainCoordinates and returns the coordinates in the referenceCrs of the view instead of in the crs of the tiledLayer extent.

example(potree_25d_map): declare map projection code

4878689

feat(picking): Add distance and point position to returned object by …

c620209

…points picking

feat(Coordinates): Add a setCrs method

494712e

jailln force-pushed the feat/elevation-measure branch from 8543d2a to 11cdf5f Compare February 2, 2023 14:42

jailln mentioned this pull request Feb 2, 2023

Elevation measure - tests #1996

Closed

jailln force-pushed the feat/elevation-measure branch from 11cdf5f to 5595e57 Compare February 3, 2023 10:15

feat(widgets): Add a widget to measure elevation at any point in the …

2dada14

…3D scene

jailln force-pushed the feat/elevation-measure branch from 5595e57 to 2dada14 Compare February 7, 2023 17:41

jailln marked this pull request as draft February 13, 2023 09:25

jailln mentioned this pull request Feb 13, 2023

Picking bugfix and API improvements #2004

Merged

jailln closed this Oct 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Elevation measure widget #1881

Elevation measure widget #1881

jailln commented Aug 9, 2022

mgermerie commented Oct 14, 2022

mgermerie Jan 18, 2023

jailln Jan 18, 2023

mgermerie Jan 18, 2023

jailln Jan 19, 2023

mgermerie Jan 18, 2023

jailln Jan 18, 2023

mgermerie Jan 18, 2023

jailln Jan 18, 2023

mgermerie Jan 18, 2023

mgermerie Jan 18, 2023

mgermerie Jan 18, 2023

jailln Jan 19, 2023

jailln commented Jan 19, 2023

jailln commented Jan 20, 2023

jailln commented Oct 4, 2024

Elevation measure widget #1881

Elevation measure widget #1881

Conversation

jailln commented Aug 9, 2022

Description

Motivation and Context

Screenshots (if appropriate)

mgermerie commented Oct 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jailln commented Jan 19, 2023

jailln commented Jan 20, 2023

jailln commented Oct 4, 2024