Geoid2 #511

qdnguyen · 2017-10-06T13:36:32Z

Description

This PR is needed to visualize geoid color and DTM.

The changes were done

Add zfactor to deform geoid DTM
Add geoid example

Screenshots

peppsac

Thanks for the rebase :)

Please use proper wording for your commits (see http://codeinthehole.com/writing/a-useful-template-for-commit-messages/ for some help) and split them in separate topics (e.g: 1 for the zFactor feature and 1 for the new example)

peppsac · 2017-10-06T13:38:22Z

src/Renderer/Shader/TileVS.glsl

@@ -14,6 +14,8 @@ uniform sampler2D   dTextures_00[1];
 uniform vec3        offsetScale_L00[1];
 uniform int         loadedTexturesCount[8];

+uniform float       zFactor;


Please move all the zFactor stuff to a specific commit.

peppsac · 2017-10-06T13:39:05Z

src/Renderer/Shader/TileVS.glsl

@@ -59,7 +59,8 @@ void main() {
                }

            #elif defined(DATA_TEXTURE_ELEVATION)
-                float   dv  = max(texture2D( dTextures_00[0], vVv ).w, 0.);
+                float   dv  = texture2D( dTextures_00[0], vVv ).w;
+                dv *=zFactor;


You should be doing dv *=zFactor; below (just before vPosition = vec4( position + vNormal * dv ,1.0 )).
This way it would work with all elevation data sources.

i did it before like you said and it work but i dont know why it does not work now if i put it before vPosition = vec4( position + vNormal * dv ,1.0 )). I think three is a problem with vNormal

Hmm.. I don't know what is the underlying issue here, but you need to fix this so the z factor feature works independently of the elevation data source.

i set zFactor value by default to 1, so it will not impact on elevation data.

My point is: we want this to work regardless of the data source used for elevation.
As an example, zFactor feature should work on planar.html demo.

peppsac

Quick review, focusing mostly on style issues.

peppsac · 2017-10-18T13:44:09Z

.gitignore

@@ -6,3 +6,5 @@
 /node_modules/
 /dist/
 /lib/
+/nbproject/private/


This doesn't belong to the itowns's .gitignore - gitignore is only populated with folders/files that are by-product or temp files of itowns.

(You can add your own ignore stuff in the .gitignore in your home folder)

peppsac · 2017-10-18T13:44:34Z

examples/geoid.html

@@ -0,0 +1,71 @@
+<!DOCTYPE html>
+<!--


Please remove this generated header.

peppsac · 2017-10-18T13:44:49Z

examples/geoid.html

+-->
+<html>
+    <head>
+        <title>Globe</title>


And fix the title :)

peppsac · 2017-10-18T13:45:20Z

examples/geoid.html

+            promises.push(itowns.Fetcher.json('layers/JSONLayers/GeoidTexture.json').then(function(result) { return globeView.addLayer(result); }));
+            //promises.push(itowns.Fetcher.json('layers/JSONLayers/Ortho.json').then(function(result) { return globeView.addLayer(result); }));
+            promises.push(itowns.Fetcher.json('layers/JSONLayers/GeoidMNT.json').then(function(result) { return globeView.addLayer(result); }));
+            //promises.push(itowns.Fetcher.json('layers/JSONLayers/IGN_MNT.json').then(function(result) { return globeView.addLayer(result); }));


And remove dead code in all this file.

peppsac · 2017-10-18T13:46:02Z

nbproject/project.properties

@@ -0,0 +1,3 @@
+files.encoding=UTF-8


This nbproject folder must not be added to iTowns.

thanks for your advice

peppsac · 2017-10-18T13:47:39Z

src/Renderer/Shader/TileVS.glsl

@@ -59,7 +59,8 @@ void main() {
                }

            #elif defined(DATA_TEXTURE_ELEVATION)
-                float   dv  = max(texture2D( dTextures_00[0], vVv ).w, 0.);
+                float   dv  = texture2D( dTextures_00[0], vVv ).w;
+                dv *=zFactor;


Hmm.. I don't know what is the underlying issue here, but you need to fix this so the z factor feature works independently of the elevation data source.

peppsac · 2017-10-18T13:47:56Z

utils/debug/Main.js

@@ -2,3 +2,4 @@ export { default as Debug } from './Debug';
 export { default as PointCloudDebug } from './PointCloudDebug';
 export { default as createTileDebugUI } from './TileDebug';
 export { default as create3dTilesDebugUI } from './3dTilesDebug';
+


Do not add empty lines please.

peppsac · 2017-10-18T13:47:59Z

src/Renderer/Shader/TileVS.glsl

@@ -69,6 +70,7 @@ void main() {
            #endif

            vPosition = vec4( position +  vNormal  * dv ,1.0 );
+


Do not add empty lines please.

peppsac · 2017-10-18T13:49:02Z

src/Core/TileMesh.js

-    return this.geometry.OBB;
+    var zFactor = this.material.uniforms.zFactor.value;
+    var OBB = this.geometry.OBB;
+        OBB.box3D.min.z *= zFactor;


Indentation is wrong here.

don't understant! We need it to adapt Bbox to geoid tiles'bbox, zFactor is set by default to 1 so it will not have an impact on DTM

The code indentation is wrong.

Btw, the code is wrong too since you're going to modify OBB.box3D.min.z (and max) each time the OBB() function is called.
So for instance if min.z = 1.0 and zFactor = 3.0 initially, min.z will be: 3 then 9 then 27...

is there a posibility that in TileMesh i can have a notification when zFactor is changed ?

@peppsac is there a copy function of OBB? what do you think if i make a copy of OBB and modify it?

IMHO, I don't think you want to build a copy of the OBB.
You should be able to use the OBB.scale attribute to store the zFactor (ie: scale = (1, 1, zFactor)).

Thank @peppsac for help, i tried it before but it did not work (i have holes as you can see in image), i think i need to modify OBB max min to adapt bbox for SSE.

My reasonning is the following: OBB is an Object3D. If you can't get Object3D properties (like scale) to work then it's a bug in OBB.
And this bug needs to be fixed (instead of adding another function to OBB).

No, there is no bug and i understand what you said but I tried the scale and it did not work, because the problem is Box3D in OBB. Do not update it, the calculation of SSE will be wrong and the results as you can see in the picture above.

Btw, i also do not think the code above is wrong, because in the case there is no geoid, zFactor is always equal to 1, there is no effect on MNT. In geoid case, when zFactor is greater than 1, i saw it runs just fine as you can see in picture below.

please dont take into account my last comment, i forgot update OBB, it work now by using OBB.scale and OBB.update .

peppsac · 2017-10-18T13:49:51Z

src/Renderer/LayeredMaterial.js

@@ -491,5 +493,9 @@ LayeredMaterial.prototype.setSelected = function setSelected(selected) {
    this.uniforms.selected.value = selected;
 };

+LayeredMaterial.prototype.setLayerZFactor = function setLayerZFactor(zFactor) {


Is this method useful? It's just a one-line method and you don't use it https://github.com/iTowns/itowns/pull/511/files#diff-caf5227be4f9e2f283d82df32a678e4bL523 so I'd suggest removing it for now.

yes, i used it in updateLayeredMaterialNodeElevation to deform geoid

I meant: it's a one-liner and you only use once.
So I'd prefer you to remove it (and replace it's usage in updateLayeredMaterialNodeElevation with material.uniforms.zFactor.value = layer.zFactor;)

peppsac · 2017-10-25T08:51:46Z

Just tested:

you should upload your data to https://github.com/iTowns/iTowns2-sample-data, so you'll be able to use a URL that works for everyone (see https://github.com/iTowns/itowns/blob/master/examples/pointcloud.html#L105 for an example)
you have a merge commit in your PR: you need to do a rebase
your commits do not respect the git messages light convention we're trying to adopt: see Adopt angular convention for git commit messages #518

Thanks!

nguyenign · 2017-10-31T10:25:54Z

ok, pr is ready to merge

peppsac

This PR is improving, thanks.

You still need to fix a few minor issues.
Then you can rebase you commits on master and:

keep only 2 commits:
- 1 adding the zFactor feature
- 1 adding an example
make sure your commit wording matches the template from Adopt angular convention for git commit messages #518

peppsac · 2017-10-31T10:42:17Z

src/Core/Prefab/GlobeView.js

@@ -107,7 +107,7 @@ export function createGlobeLayer(id, options) {
        }
    }

-    const wgs84TileLayer = new GeometryLayer(id, options.object3d || new THREE.Group());
+    const wgs84TileLayer = new GeometryLayer(id, options.object3d);


Why did you changed this?

it's changed automatically when i did rebase, i will change it back!

peppsac · 2017-10-31T10:42:36Z

src/Core/Prefab/GlobeView.js

@@ -194,6 +194,8 @@ function GlobeView(viewerDiv, coordCarto, options = {}) {
    // Setup View
    View.call(this, 'EPSG:4978', viewerDiv, options);

+    options.object3d = options.object3d || this.scene;


Same question: why did you change this?

I don't think you need to modify GlobeView, except for adding the setZFactor method.

peppsac · 2017-10-31T10:46:21Z

src/Core/TileMesh.js

-    return this.geometry.OBB;
+    var zFactor = this.material.uniforms.zFactor.value;
+    var OBB = this.geometry.OBB;
+        OBB.scale.z = zFactor;


Code indentation is still wrong (#511 (comment)).
And please use const instead of var.

what wrong? it work for me!

peppsac · 2017-10-31T10:46:43Z

utils/debug/Main.js

@@ -1,4 +1,4 @@
 export { default as Debug } from './Debug';
 export { default as PointCloudDebug } from './PointCloudDebug';
 export { default as createTileDebugUI } from './TileDebug';
-export { default as create3dTilesDebugUI } from './3dTilesDebug';
+export { default as create3dTilesDebugUI } from './3dTilesDebug';


You don't need to modify this file.

nguyenign · 2017-10-31T16:35:32Z

ok, it's ready to merge now :)

nosy-b · 2017-11-21T15:58:23Z

@qdnguyen Any news on that nice feature?

nguyenign · 2017-11-21T16:02:53Z

no, as i said it's ready to merge!

nosy-b · 2017-11-21T16:04:50Z

@qdnguyen Could you rebase?

nguyenign · 2017-11-21T16:26:08Z

rebased!

nosy-b · 2017-11-21T16:33:09Z

You have some lint problems
### 302:1 error Trailing spaces not allowed no-trailing-spaces
303:5 error Expected exception block, space or tab after '//' in comment spaced-comment

Plus, could you add the example.html in the example/index.html page with the right screenshot as it's a new feature?

Changing the `scale` property of an `ElevationLayer` can changed the aspect of the visual, allowing exageration of the elevation of the globe, as in the tiff.html example. This is an up-to-date version of iTowns#511

gchoqueux · 2019-07-03T11:25:28Z

moved in #1174

Changing the `scale` property of an `ElevationLayer` can changed the aspect of the visual, allowing exageration of the elevation of the globe, as in the tiff.html example. This is an up-to-date version of iTowns#511

Changing the `scale` property of an `ElevationLayer` can changed the aspect of the visual, allowing exageration of the elevation of the globe, as in the tiff.html example. This is an up-to-date version of #511

peppsac suggested changes Oct 6, 2017

View reviewed changes

qdnguyen force-pushed the geoid2 branch from 006a7af to c05f752 Compare October 6, 2017 14:27

qdnguyen force-pushed the geoid2 branch from c05f752 to 02138e5 Compare October 17, 2017 15:42

peppsac reviewed Oct 18, 2017

View reviewed changes

qdnguyen force-pushed the geoid2 branch 2 times, most recently from c8c79f4 to fa5920b Compare October 31, 2017 10:24

peppsac suggested changes Oct 31, 2017

View reviewed changes

qdnguyen force-pushed the geoid2 branch from fa5920b to fdcdd3d Compare October 31, 2017 13:36

gchoqueux mentioned this pull request Nov 10, 2017

Geoid #190

Closed

qdnguyen force-pushed the geoid2 branch from fdcdd3d to d8d574f Compare November 21, 2017 16:21

autra and others added 2 commits November 21, 2017 17:23

geoid : add geoid example

975516a

geoid : add zFactor feature

3b9cb6f

qdnguyen force-pushed the geoid2 branch from d8d574f to 3b9cb6f Compare November 21, 2017 16:25

gchoqueux added this to the 2.xx.x milestone Feb 1, 2019

zarov mentioned this pull request Jul 3, 2019

feat(layer): add full support to scaling for elevation layers #1174

Merged

gchoqueux closed this Jul 3, 2019

gchoqueux deleted the geoid2 branch July 30, 2019 14:13

		@@ -69,6 +70,7 @@ void main() {
		#endif

		vPosition = vec4( position + vNormal * dv ,1.0 );

Geoid2 #511

Geoid2 #511

Conversation

qdnguyen commented Oct 6, 2017 • edited Loading

peppsac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qdnguyen Oct 6, 2017 • edited Loading

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

peppsac left a 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

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

peppsac Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qdnguyen Oct 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peppsac Oct 30, 2017 • edited Loading

Choose a reason for hiding this comment

nguyenign Oct 31, 2017 • edited Loading

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

peppsac Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peppsac commented Oct 25, 2017

nguyenign commented Oct 31, 2017

peppsac left a 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

nguyenign commented Oct 31, 2017

nosy-b commented Nov 21, 2017

nguyenign commented Nov 21, 2017

nosy-b commented Nov 21, 2017

nguyenign commented Nov 21, 2017

nosy-b commented Nov 21, 2017

gchoqueux commented Jul 3, 2019

qdnguyen commented Oct 6, 2017 •

edited

Loading

qdnguyen Oct 6, 2017 •

edited

Loading

peppsac Oct 23, 2017 •

edited

Loading

qdnguyen Oct 25, 2017 •

edited

Loading

peppsac Oct 30, 2017 •

edited

Loading

nguyenign Oct 31, 2017 •

edited

Loading

peppsac Oct 23, 2017 •

edited

Loading