First rough integration + ol.view sync + initial demos #16

klokan · 2014-07-30T16:43:28Z

This PR is made to have our source code directly visible to all memebers of the kick-off. It is not code ready to be merged yet.

It is demonstrating a possible minimalistic version of ol3factory discussed in #6 and #8 with only basic switching between 2D ol3 and 3D cesium with preserved center, resolution and rotation - with sync only during the switch of the view.

Check primarily the source code of the example, deployed at:
http://klokantech.github.io/ol3-cesium/

Before a real code review and merge to master this PR must be significantly improved.
A must have is a compilation via ADVANCED - where we miss ol3 externs. See: #15

…ization

elemoine · 2014-08-04T13:20:11Z

src/camera.js

+   * @type {ol.TransformFunction}
+   * @private
+   */
+  this.toLatLng_ = ol.proj.getTransform(this.view_.getProjection(),


toLonLat would sound more correct to me

Fixed, thanks

elemoine · 2014-08-04T14:58:33Z

src/camera.js

+  var visibleMapUnits = meters / circ;
+  var resolution = visibleMapUnits / this.canvas_.width;
+
+  return resolution;


For my own knowledge I'd be very interested to fully understand these calcDistanceForResolution_ and calcResolutionForDistance_ functions. Any pointers for me? Thanks.

I tried to make the code more easy to read. I hope it's clearer now.

Very nice. Thanks a lot for that. The role of the circ variable was not clear to me. Clearer now.

…s other than meters

…(including terrain)

klokan · 2014-08-05T09:06:57Z

There is a new example for side-by-side live ol.View synchronization and improved 2d/3d switching.
http://klokantech.github.io/ol3-cesium/sidebyside.html

Terrain is turned on - for testing the ol.View sync implementation according #9.

Closure ADVANCED compilation is now applied (blocking #15 closed).

elemoine · 2014-08-05T09:23:28Z

Quick comment: I cannot open this PR's commit pages. I get 404s probably because klokantech/ol3-cesium is a private repo I don't have access to. Any idea on how to solve that problem?

elemoine · 2014-08-05T12:31:50Z

Makefile

@@ -0,0 +1,28 @@
+PLOVR = ./plovr-81ed862.jar


In ol3 we decided to go without Plovr and use the closure-util node package instead. I'd suggest that we do the same for ol3-cesium.

Agreed. We are aware of this.

Our target now was to have as fast as possible a correct ADVANCED compilable JavaScript code. We use plovr in the company heavily - this has been fastest for us for the first PR.

Anybody familiar with npm/node closure compiler is welcome to make the switch. Plovr can be deprecated on the ol3cesium and replaced by node package instead.
Pull request after this accepted for master is welcome.

elemoine · 2014-08-05T13:01:27Z

Thanks a lot for putting this together. I think this is a very good base for discussion. I've left a couple of comments in the code (some of which have already been addressed by @petrsloup already). Here's another one:

In the case of side-by-side ol3/cesium (as in this PR's sidebyside.html example) there are two ol3 maps as one ol3 map is required for ol3cesium. That seems weird to me. In that case I would expect to use an ol3 map for the 2D part, a Cesium globe for the 3D part, and ol3-cesium for the synchronizing the view between the map and the globe.

elemoine · 2014-08-05T15:43:43Z

src/ol3cesium.js

+  );
+
+  var osm = new Cesium.OpenStreetMapImageryProvider();
+  this.scene_.imageryLayers.addImageryProvider(osm);


Defining an initial camera lookAt and adding an OSM imagery provider is assuming too much on the user's intent. This is probably obvious but I thought I could still mention it.

…uding exports)

elemoine · 2014-08-06T14:21:33Z

src/camera.js

+   * need to be calculated _at the target_.
+   */
+  if (target) {
+    var pos = this.cam_.positionWC; //this forces the update


I guess the difference between reading this.cam_.positionWC and reading this.cam_.position is that reading positionWC has a side-effect (there's a defineProperty getter for it). Why do we need to force an update and use positionWC here?

The difference is mainly that positionWC (WC ~ "World Coordinates") can contain different values than position (which is in specific coordinate system of the camera). However, these values should always be equal for Cesium.SceneMode.SCENE3D, which is the only mode we are interested in. So we can probably save some calculations by accessing position directly. I will change this soon.

Thanks. I do not understand what you mean by "specific system of the camera", but that's probably a sufficient explanation for now :-)

…sible without using the quotes

… are always equal for Cesium.SceneMode.SCENE3D)

petrsloup · 2014-08-06T15:15:10Z

I think I have addressed all the comments from this PR and all the modifications we talked about.
Please review.

The compiled demos can be tested here:
http://klokantech.github.io/ol3-cesium/index.html
http://klokantech.github.io/ol3-cesium/sidebyside.html
http://klokantech.github.io/ol3-cesium/exports.html

elemoine · 2014-08-06T15:40:28Z

Thanks for updating the PR. We will continue reviewing it tomorrow. Our review will be complete by tomorrow night, so we'll probably be able to merge the PR on Friday.

elemoine · 2014-08-06T16:08:49Z

src/camera.js

+ */
+olcs.Camera.prototype.readFromView = function() {
+  this.distance_ = this.calcDistanceForResolution_(
+      this.view_.getResolution() || 0,


Not sure about this || 0.

elemoine · 2014-08-07T08:23:48Z

src/camera.js

+
+    // TILT
+    this.tilt_ = Math.acos(Cesium.Cartesian3.dot(targetNormal,
+                                                 targetToCamera)) || 0;


Minor but for the sake of clarity I'd rather do

var tiltAngle = Math.acos(Cesium.Cartesian3.dot(targetNormal, targetToCamera)); this.tilt_ = isNaN(tiltAngle) ? 0 : tiltAngle;

Also, it looks like that the tilt angle calculated here is not the same angle as the tilt angle on the drawing in #17. Is this correct? What's the reason for this? Is it because you want this angle to stay constant when the distance between the camera and the target changes?

What puzzles us (@fredj, @gberaudo and I) is that after a setTilt the updateCamera function uses tilt_ as follows:

if (this.tilt_) { this.cam_.lookUp(this.tilt_); }

Doing cam.lookUp(this.tilt_) does not make sense to me if tilt_ is the angle between the "target normal" vector and the "target to camera" vector?

Are we missing something here? Thanks for any response.

Ok, updateCamera doing this.cam_.lookUp(this.tilt_) now makes sense to me.

Calling this.cam_.setGeographicPosition(carto) actually resets the direction of the camera (towards the world origin). So by doing this.cam_.lookUp(this.tilt_) we rotate the camera around its right vector to reset the actual tilt angle to its original value.

So the behavior is correct.

What is a bit strange though is that the angle the user passes to setTilt is not the angle between the camera direction and the vector that goes from the world origin to the camera. This is a bit confusing IMO. It is that latter definition of tilt which the user expects to deal with.

elemoine · 2014-08-07T13:51:15Z

Again, thanks for the work on this. We've added more comments in the commits.

There's one comment about setTilt. When calling setTilt the user doesn't expect to be changing the angle "at the ground" (i.e. between the "world origin to target" and the "target to camera" vectors). Instead I think the user expects to change the angle between the "camera to world origin" and the "camera to target" vectors.

Another comment is that in "stack mode" the ol3 view is continuously updated when the user navigates in the globe, and the Cesium camera is continuously updated when the user navigates the 2D map. I think this may cause performance problems for more complex rendering (with vector layers with lots of features for example). In a "stack mode" application – with a 2D/3D switch button – you actually just need to do 2D/3D view conversions when the user clicks the 2D/3D switch button.

We agree to merge this PR when our commit comments are addressed. But we still think that in the end the library should be flexible enough to address different use-cases. The "2D/3D switcher" case, where the 2D/3D views need to be synchronized only when the 2D/3D switcher is pressed, is one of them.

We strongly think that flexibility can be obtained by factoring out code in small core static functions that applications can use. These functions can hide complex math and shorten often written code. Their names and descriptions should completely represent their actions.

…to be more consistent with the ol3 style

klokan · 2014-08-08T09:31:24Z

Thank you everybody for the review and comments on this PR - and @petrsloup - great job!

Some more points appeared in the last comment:

Tilt angle at camera vs at target/center

We were discussing this again. The user is naturally moving the target/center and this is essential for OLCesium wrapper and interaction with ol.view - camera is beeing manipulated only secondary. If we would use the titl angle assigned to camera it would hugely complicate the calculations and limit us further in the implementation of navigation. We feel it is unnatural and does not really make sense.
We created a new illustration of what is what on the OLCesium camera. It is now part of #8.
For the reference, this is the difference:

No rendering if invisible & lazy sync

To be discussed and resolved as part of #19

I am merging now this PR. Thanks again to everybody!

First rough integration + ol.view sync + initial demos

elemoine · 2014-08-08T09:52:25Z

Is your tilt angle the same as Google Earth's "lookAt tilt"? See https://developers.google.com/earth/documentation/camera_control#tilt

klokan · 2014-08-08T10:30:11Z

It seems so (beside degrees vs radians)

petrsloup added 5 commits July 26, 2014 13:00

Very rough first version of the architecture and basic cesium initial…

fa855f3

…ization

Implemented outline of camera position calculations (ol3Cesium.Camera)

c492d7f

Automatically "pause" all the ol3 interactions when enabling ol3cesium

cb2e63f

Minor ol3Cesium.Camera refactoring

424dce9

Example modification

75cdd33

klokan added help wanted and removed help wanted labels Jul 30, 2014

elemoine reviewed Aug 4, 2014
View reviewed changes

petrsloup added 5 commits August 4, 2014 15:40

Do the terrain picking

ad00dc8

Cesium.Camera<->ol.View synchronization

f3f75a3

Updated Cesium.externs.js

c53b5e2

Preparing for the advanced compilation: minor type fix + exports

34e212f

Enable the advanced compilation (with "checkTypes":"OFF" at the moment)

1ba3fbe

elemoine reviewed Aug 4, 2014
View reviewed changes

petrsloup added 8 commits August 4, 2014 20:02

Correct heading and tilt calculation (relative to the camera target)

e6ef05f

Stronger typing of the Camera properties

c3cf24d

More correct naming of Camera transformation properties

6ef5660

Modified the resolution calculation to work for projections with unit…

15101d2

…s other than meters

Type fixes

d09dab7

Fix for occasional camera bug when not initialized properly

abc919a

Added new side-by-side example demonstrating ol.View synchronization …

6c65278

…(including terrain)

More additions to the Cesium externs

9964829

Add output wrapper to the built javascript

977f0a4

elemoine reviewed Aug 5, 2014
View reviewed changes

Implemented number of methods for modifying state of the camera (incl…

009d3a6

…uding exports)

elemoine reviewed Aug 6, 2014
View reviewed changes

petrsloup added 2 commits August 6, 2014 16:35

More strongly typed Cesium externs to prevent as much renaming as pos…

d307fd8

…sible without using the quotes

Use Cesium.Camera.position instead of Cesium.Camera.positionWC (these…

f8a63cc

… are always equal for Cesium.SceneMode.SCENE3D)

elemoine reviewed Aug 6, 2014
View reviewed changes

klokan mentioned this pull request Aug 7, 2014

Camera roll + free look interaction #17

Open

elemoine reviewed Aug 7, 2014
View reviewed changes

petrsloup added 4 commits August 8, 2014 08:26

Added missing newlines ad the ends of some files

4751328

Fixed description of the setEnabled method

e71e8f0

Replaced "|| 0" expressions with proper checks (goog.isDef || isNaN) …

8998f42

…to be more consistent with the ol3 style

Do the interaction unpausing only when there can be anything to unpause

1d6ffaf

elemoine mentioned this pull request Aug 8, 2014

Support "switching between 2D and 3D" use-case #18

Open

klokan added a commit that referenced this pull request Aug 8, 2014

Merge pull request #16 from klokantech/master

f363cf2

First rough integration + ol.view sync + initial demos

klokan merged commit f363cf2 into openlayers:master Aug 8, 2014

This was referenced Aug 8, 2014

Core: OLCesium constructor + camera #8

Closed

Core: View synchronization #9

Closed

klokan mentioned this pull request Aug 8, 2014

Share the same user interactions #6

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First rough integration + ol.view sync + initial demos #16

First rough integration + ol.view sync + initial demos #16

klokan commented Jul 30, 2014

elemoine Aug 4, 2014

petrsloup Aug 4, 2014

elemoine Aug 4, 2014

petrsloup Aug 4, 2014

elemoine Aug 5, 2014

klokan commented Aug 5, 2014

elemoine commented Aug 5, 2014

elemoine Aug 5, 2014

klokan Aug 5, 2014

elemoine commented Aug 5, 2014

elemoine Aug 5, 2014

elemoine Aug 6, 2014

petrsloup Aug 6, 2014

elemoine Aug 6, 2014

petrsloup commented Aug 6, 2014

elemoine commented Aug 6, 2014

elemoine Aug 6, 2014

elemoine Aug 7, 2014

elemoine Aug 7, 2014

elemoine Aug 7, 2014

elemoine commented Aug 7, 2014

klokan commented Aug 8, 2014

elemoine commented Aug 8, 2014

klokan commented Aug 8, 2014

First rough integration + ol.view sync + initial demos #16

First rough integration + ol.view sync + initial demos #16

Conversation

klokan commented Jul 30, 2014

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

klokan commented Aug 5, 2014

elemoine commented Aug 5, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elemoine commented Aug 5, 2014

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

petrsloup commented Aug 6, 2014

elemoine commented Aug 6, 2014

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

elemoine commented Aug 7, 2014

klokan commented Aug 8, 2014

elemoine commented Aug 8, 2014

klokan commented Aug 8, 2014