Terrain #510

kring · 2013-02-09T14:41:46Z

There are three major parts to this:

A significant refactoring of the terrain/imagery loading system, making it easier to understand, easier to add terrain providers, and probably less buggy.
Lots of changes in ShaderProgram and UniformState and the like to support lighting 2D and Columbus View as if they're 3D. The overall goal is to make 2D and Columbus View look just like 3D, except the Earth is a plane instead of a globe. The water rendering is part of this, sort of.
Terrain providers, so we can get real, actual terrain in Cesium.

Previously, the base layer would not be mapped to tiles that didn't overlap it at all.

This is very hacked up at the moment.

It should now be usable in normal material and as used by the central body where the water color is supplied as a parameter instead of a uniform.

It went away back the water material's normalMap uniform lost its default.

Conflicts: Source/Scene/CentralBody.js Source/Shaders/CentralBodyFS.glsl

kring · 2013-02-13T19:57:54Z

Ok, this is ready for another (final?) look.

mramato · 2013-02-13T20:44:37Z

Okay, I'll do a quick review since I haven't really looked at this yet.

mramato · 2013-02-13T20:48:04Z

The Sandcastle example still throws an exception when you hit one of the zoom buttons in 2D.

mramato · 2013-02-13T21:12:08Z

Source/Core/Ellipsoid.js

+     *                    instance should be created.
+     * @returns {Ellipsoid} The cloned Ellipsoid.
+     */
+    Ellipsoid.clone = function(ellipsoid, result) {


Does it make sense for this to be a "static" function? Everything else in this file is on the prototype. At the very least we should provide a prototype version (and my vote would be to get rid of the static version). Also, we usually treat Ellipsoid as an immutable type (obviously it can't really be one in javascript), does providing a result parameter here break that paradigm? Don't we have a bunch of baked in assumptions that Ellipsoids don't change?

I'm using this to reconstitute an ellipsoid after passing it to a web worker. All the functions get stripped away and only the fields are left. By taking this neutered Ellipsoid and passing it through Ellipsoid.clone, I get back an actual Ellipsoid that is equivalent to the original. A prototype method won't work for that.

Good enough for me.

kring · 2013-02-13T21:17:50Z

The Sandcastle example still throws an exception when you hit one of the zoom buttons in 2D.

Fixed. It's ridiculous the hoops we have to jump through to create a similar view in all 3 modes, of course.

mramato · 2013-02-13T21:20:32Z

Source/Scene/CentralBody.js

+         *
+         * @type Number
+         */
+        this.tileCacheSize = 100;


I'm more curious than anything else, did we do any testing to pick this number?

Cozzi asked the same thing. The answer is no, I totally made it up. Most of the time we "need" more than 100 tiles for rendering anyway, so the value of 100 basically means unload everything you don't need right now. A higher number would keep tiles in memory when you, say, zoom out and then back in, which would be nice. On the other than, having memory usage drop when you zoom out is a nice feature, too. I don't know the right answer.

Then the question is simple, do we care more about visual quality or memory? I think quality comes first so I would opt for a higher number by default. If people raise concerns over memory usage, we can always change the default.

mramato · 2013-02-13T21:27:18Z

I completely agree about the camera. We need to make sure that at the "highest" programmatic level, all functionality is exposed in world-space and the difference between 2D/3D/Columbus are completely hidden.

mramato · 2013-02-13T21:33:58Z

I still have a race condition of some sort where if I start with an empty cache, switch to 2D, and zoom all the way in, I get major culling issues and half the screen just goes black. However, I just tried master and it appears to happen there too, so it shouldn't hold this up.

mramato · 2013-02-13T21:42:38Z

Source/Scene/CesiumTerrainProvider.js

+    /**
+     * A {@link TerrainProvider} that produces geometry by tessellating height maps
+     * retrieved from a Cesium terrain server.  The format of the terrain tiles is described on the
+     * {@link https://github.com/AnalyticalGraphicsInc/cesium/wiki/Cesium-Terrain-Server Cesium wiki}.


This link is formatted incorrectly and leads to creating new wiki page.

mramato · 2013-02-13T22:02:30Z

I didn't review it as closely as @pjcozzi did, but I looked over the code and did our "release" testing so confirm everything works. Other than the 2 doc issues, this is fine with me.

mramato · 2013-02-13T22:12:49Z

Also; I tried mobile

Tegra 3: water looks pretty bad, and it's slow; but everything seems to work. I don't think we made anything on mobile worse (which is the low bar I was setting when I tried it out).

Adreno 225: Looks freakin' awesome, everything works.

pjcozzi · 2013-02-14T13:23:12Z

@kring tests look good. Looks like you used the WebGL category accordingly. As @mramato said, they pass in Chrome and Firefox.

Coverage is reasonable enough. I'd probably at least add tests for the DeveloperError in TilingScheme.createRectangleOfLevelZeroTiles. All the imagery/terrain providers don't cover their "must not be called before the imagery provider is ready." I'm not sure how hard that is to test. We can say that it's outside the scope of this pull request though.

Let me know when the doc fixes are in, and we'll merge this. In the meantime, I'll get the tutorial ready to go - just need to update the date.

kring · 2013-02-14T21:30:00Z

I'd probably at least add tests for the DeveloperError in TilingScheme.createRectangleOfLevelZeroTiles.

Ha, that's some serious bike shedding. I'll add it tomorrow if this isn't already merged by then.

pjcozzi · 2013-02-14T22:23:25Z

Ha, that's some serious bike shedding. I'll add it tomorrow if this isn't already merged by then.

Sorry, there's actually 3 developer errors there. We should really cover all developer errors. I'm not sure that I ever opened a pull request that did not cover reasonable ones to test like these.

Conflicts: CHANGES.md

kring · 2013-02-15T15:14:32Z

Sorry, there's actually 3 developer errors there. We should really cover all developer errors. I'm not sure that I ever opened a pull request that did not cover reasonable ones to test like these.

Ok, this is taken care of. Just for the record, this code was added with the imagery_layers merge, and wasn't touched here.

Also, we clearly disagree on the importance of these DeveloperErrors. I was just making the case to Scott the other day that, in most cases, adding these undefined checks is a waste of time (developer time and a tiny bit of run time), and adding tests for them is even more so a waste of developer time.

Meanwhile, this pull request includes some very "tricky" tests of the tile load pipeline. Feedback on those would be valuable, as would another pair of eyes looking at what important aspects I may have missed (even though the coverage is good). Looking at coverage numbers of trivial code without looking at the hard stuff is classic bike shedding. I understand why it happens, and it's likely I'd do the same thing while reviewing someone else's pull request. So I probably sounded like an ass above, but we shouldn't kid ourselves.

pjcozzi · 2013-02-15T15:31:21Z

Looks good. Merging. Email the dev list once we get the tutorial up.

As for getting rid of developer error, I'm all ears. I talked about doing this in limited cases - for optimized release builds - a really long time ago. If you think it's pressing, bring it up on the dev list.

Terrain

kring and others added 30 commits November 6, 2012 09:55

Fix terrain example after widget changes.

43d4595

Use child bits in each tile.

7a4be28

Use terrain hosted on cesium.cloudapp.net.

b0f6603

Don't unload tiles with transitioning imagery.

06a2e3f

Fix base layer stretching beyond its extent.

5d56c64

Previously, the base layer would not be mapped to tiles that didn't overlap it at all.

Merge remote-tracking branch 'origin/master' into terrain

312b0e7

Update package list and fix widget startup.

6b91652

Merge branch 'scheduleRenderLoop' into terrain

4a8ac94

Merge remote-tracking branch 'origin/ocean' into terrainAndOcean

eef53e0

Apply water material to terrain tiles.

b8f6489

Use high-res water mask derived from SRTM data.

22d5dba

This is very hacked up at the moment.

Slow down the wave animation.

24f9e1f

Use the actual imagery color as the ocean base color.

8197d4c

Tweak water fragment shader.

0d9ee12

It should now be usable in normal material and as used by the central body where the water color is supplied as a parameter instead of a uniform.

Merge remote-tracking branch 'origin/master' into terrainAndOcean

2330d10

Fix apps that don't use CesiumTerrainProvider.

2612e19

Merge branch 'master' into terrain

41af797

Temporarily use GTOPO30 terrain.

92a5629

Merge remote-tracking branch 'origin/master' into terrainAndOcean

12f0633

Restore the fancy water.

7e3c189

It went away back the water material's normalMap uniform lost its default.

Lighting tweaks.

aa655ac

Switch to using terrain/water data on cesium.agi.com.

17172c3

Merge remote-tracking branch 'origin/master' into terrain

35937d0

Merge branch 'terrain' into terrainAndOcean

9f8db91

Merge branch 'master' into terrainAndOcean

e9bfec5

Fix test failures and add a new test.

4609b4c

Merge branch 'terrain' into terrainAndOcean

45bb4bb

Fix Sandcastle examples and test failures.

205475f

Merge branch 'lighting' into terrainAndOcean

e26104c

Conflicts: Source/Scene/CentralBody.js Source/Shaders/CentralBodyFS.glsl

Fix water shading.

354c296

kring added 2 commits February 13, 2013 14:46

Add tests for HeightmapTerrainData.

5102766

Clean up attribute array construction.

0727084

Fix CentralBody doc problem.

00742b4

mramato reviewed Feb 13, 2013
View reviewed changes

Make zoom buttons work in 2D.

a6c59eb

mramato reviewed Feb 13, 2013
View reviewed changes

Fix doc problems.

a2fd7cf

kring added 4 commits February 15, 2013 09:39

Add TilingScheme tests.

f9471f4

Update CHANGES.md.

7ca3e4d

Remove dead code from CentralBodySurface.

9ca73fb

Merge remote-tracking branch 'origin/master' into terrain

ddbaee3

Conflicts: CHANGES.md

pjcozzi added a commit that referenced this pull request Feb 15, 2013

Merge pull request #510 from AnalyticalGraphicsInc/terrain

1c45e05

Terrain

pjcozzi merged commit 1c45e05 into master Feb 15, 2013

pjcozzi deleted the terrain branch February 15, 2013 15:31

pjcozzi mentioned this pull request Nov 13, 2015

Terrain Exaggeration #3200

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Terrain #510

Terrain #510

kring commented Feb 9, 2013

kring commented Feb 13, 2013

mramato commented Feb 13, 2013

mramato commented Feb 13, 2013

mramato Feb 13, 2013

kring Feb 13, 2013

mramato Feb 13, 2013

kring commented Feb 13, 2013

mramato Feb 13, 2013

kring Feb 13, 2013

mramato Feb 13, 2013

mramato commented Feb 13, 2013

mramato commented Feb 13, 2013

mramato Feb 13, 2013

mramato commented Feb 13, 2013

mramato commented Feb 13, 2013

pjcozzi commented Feb 14, 2013

kring commented Feb 14, 2013

pjcozzi commented Feb 14, 2013

kring commented Feb 15, 2013

pjcozzi commented Feb 15, 2013

Terrain #510

Terrain #510

Conversation

kring commented Feb 9, 2013

kring commented Feb 13, 2013

mramato commented Feb 13, 2013

mramato commented Feb 13, 2013

mramato Feb 13, 2013

Choose a reason for hiding this comment

kring Feb 13, 2013

Choose a reason for hiding this comment

mramato Feb 13, 2013

Choose a reason for hiding this comment

kring commented Feb 13, 2013

mramato Feb 13, 2013

Choose a reason for hiding this comment

kring Feb 13, 2013

Choose a reason for hiding this comment

mramato Feb 13, 2013

Choose a reason for hiding this comment

mramato commented Feb 13, 2013

mramato commented Feb 13, 2013

mramato Feb 13, 2013

Choose a reason for hiding this comment

mramato commented Feb 13, 2013

mramato commented Feb 13, 2013

pjcozzi commented Feb 14, 2013

kring commented Feb 14, 2013

pjcozzi commented Feb 14, 2013

kring commented Feb 15, 2013

pjcozzi commented Feb 15, 2013