TerrainExtension (2/3) #7605

Pessimistress · 2023-01-28T23:58:14Z

See docs for API review.
Tests will be in the next PR.

Change List

Add TerrainExtension
Override the getBounds method in SimpleMeshLayer to account for the non-instanced use case (used by the TerrainLayer)
Documentation

Pessimistress · 2023-01-30T17:53:57Z

modules/extensions/src/terrain/terrain-cover.ts

+
+const MAX_SIZE = 2048;
+
+// TODO - import from loaders when Tileset2D is split out


FYI @ibgreen - I'm not importing the types from geo-layers because geo-layers currently depend on extensions. TypeScript really does not like circular dependencies. Also a good argument to strip Tileset2D out of that increasingly complex module.

As long as you use import type you might be OK, as TypeScript does support for circular import type declarations.

But I agree we should try very hard to avoid this circularity as it is really confusing and will lead to other things creeping in down the line.

BTW, I got a good way on the PR separating Tileset2D from Viewport but then ran into issues:

both node and browser tests are badly broken on Apple M1, so I really couldn't iterate in an efficient way, replying on CI takes like 10-15 minutes per cycle.

Also there is one part where the Tileset2D interacts back and forth with the viewport which makes it hard to make the required values static input parameters, would be great to get your thoughts on that.

+1 on moving Tileset2D/TileHeader into core

Pessimistress · 2023-01-30T19:46:49Z

modules/extensions/src/terrain/terrain.ts

+    if (!terrainFittingMode) {
+      // props.extruded is used as an indication that the layer is 2.5D
+      // @ts-ignore `extruded` may not exist in props
+      const is3d = this.props.extruded as boolean;


I want to note here that MVTLayer renders a single SolidPolygonLayer for both extruded and non-extruded polygons (by returning 0 from getElevation). This makes it impossible to render the park layer as draped and the building layer as offset. We have discussed separating features by MVT layer, and this is an example where it is necessary.

@felixpalmer @alasarr

coveralls · 2023-01-31T00:05:25Z

Coverage: 88.855% (-1.3%) from 90.146% when pulling 8ddedf1 on x/terrain-2 into bd5bcf1 on master.

ibgreen

Some quick nits from first read through.

ibgreen · 2023-01-31T13:40:13Z

modules/extensions/src/terrain/terrain-cover.ts

+
+const MAX_SIZE = 2048;
+
+// TODO - import from loaders when Tileset2D is split out


As long as you use import type you might be OK, as TypeScript does support for circular import type declarations.

But I agree we should try very hard to avoid this circularity as it is really confusing and will lead to other things creeping in down the line.

BTW, I got a good way on the PR separating Tileset2D from Viewport but then ran into issues:

both node and browser tests are badly broken on Apple M1, so I really couldn't iterate in an efficient way, replying on CI takes like 10-15 minutes per cycle.

Also there is one part where the Tileset2D interacts back and forth with the viewport which makes it hard to make the required values static input parameters, would be great to get your thoughts on that.

modules/extensions/src/terrain/terrain-cover.ts

ibgreen · 2023-01-31T13:54:36Z

modules/extensions/src/terrain/terrain-cover.ts

+  return layers;
+}
+
+/** If layer is the descendent of a TileLayer, return the corresponding tile. */


This is pretty deep knowledge about layer types... Do we may want to elevate some coreTileLayer into the canonical set of "building block" layers (Layer, CompositeLayer, ...)

That may not be a bad idea...

modules/extensions/src/terrain/terrain-pass.ts

modules/extensions/src/terrain/terrain-picking-pass.ts

docs/api-reference/extensions/terrain-extension.md

felixpalmer · 2023-02-02T10:48:35Z

modules/extensions/src/terrain/terrain-cover.ts

+
+const MAX_SIZE = 2048;
+
+// TODO - import from loaders when Tileset2D is split out


+1 on moving Tileset2D/TileHeader into core

modules/extensions/src/terrain/terrain-cover.ts

felixpalmer · 2023-02-02T12:04:22Z

modules/extensions/src/terrain/terrain-effect.ts

+  useInPicking = true;
+
+  /** true if inside a picking pass */
+  private isPicking: boolean = false;


Given that useInPicking is set on Effect, would it not be better to move isPicking there also and then have it set externally rather than inferring from the Pass?

Technically this flag is not a property of the effect, but a property of the current pass. What we need is an isPicking in PreRenderOptions, just like in LayerFilterContext. I'm not making that change here because it's going to be a much more extensive API change. All these bits of information about the current render pipeline are scattered and slightly different when passed to each effect method, layer method, pass method, and user callbacks. We need to do a thorough review of the Effect interface in v9.0, now that we have a lot more use cases to consider than when it was first designed.

Given that we are very close to the 8.9 release, I propose that we don't rush to another big API overhaul prematurely. At least this is a self-contained module and will be easy to refactor when we're ready.

modules/extensions/src/terrain/terrain-effect.ts

felixpalmer · 2023-02-02T12:16:17Z

modules/extensions/src/terrain/terrain-effect.ts

+      ...opts,
+      pass: 'terrain-height-map',
+      viewports: [viewport],
+      layers: terrainLayers,


Another worry about updating the viewport is that slight changes in the viewport will result in a different rasterized heightmap. Thus offset features will wobble as the camera is moved. An extreme example is a thin column terrain layer offseting points. Sometimes the sample will hit and other times miss

Screen.Recording.2023-02-02.at.13.14.22.mov

Perhaps the heightmap could be rendered into a viewport defined in the coordinate space of the terrain layer?

It was a hard decision to make. There is only one height map for all layers. If we index the height information by common space coordinates instead of clipspace coordinates, then for high-pitch scenarios, the resolution closer to the camera will be very bad. I can try construct a viewport from the pitch, bearing and the common bounds, which could be more stable. The shadow module suffers from the similar problem, and honestly it's never completely solved.

It would be great if at least when panning we could avoid the glitches. I know it is a hard problem to solve, one approach I've used in the past was to quantize the viewport calculations (e.g. round to the nearest degree/zoom) so that small changes don't cause a heightmap re-render. While this doesn't completely solve the issue it at least means there are fewer jumps.

I also have the same issue in the CollideExtension where fading isn't completely smooth due to sampling precision. I've considered writing the visibilities into a intermediate buffer/texture and averaging over frames to smooth out the visibility value, but it is too much to fit into the current work, but perhaps something to consider as a future improvement.

If we index the height information by common space coordinates instead of clipspace coordinates, then for high-pitch scenarios, the resolution closer to the camera will be very bad.

Superficially, this seems quite similar to the problems that arise for perspective shadow maps. The good news is that there are a wide range of techniques to address those.

Rendering the shadowmap in three zones (top thirds, middle third, bottom third), rendering by object distance, logarithmic shadowmaps etc. It is not hard to find good articles.

If my hunch is right, we might even be able to share such code with an improved shadow effect?

Now this implementation seems to have some nice advantages, it should have good performance characteristics for static layers, however for heavily animated layers or very dense scenes where the draw call count becomes very high with this method, it is not impossible that a more "traditional", non-tiled approach could be useful

Thank you both for the suggestions. I'm interested in exploring this idea further in a future PR, especially as a reusable strategy in multiple effects. As it is now the height map texture is not bound to any viewport type (common space -> clip space projection is handled by its own shader, not the standard project module) which means we could potentially use regions of this texture differently to provide multi-tier resolution.

This PR is already huge as is. I acknowledge that various functionalities could still be improved, but I prefer that we land a good skeleton first then improve it incrementally.

felixpalmer · 2023-02-02T12:18:42Z

modules/extensions/src/terrain/terrain-effect.ts

+          ...opts,
+          layers: drapeLayers,
+          moduleParameters: {
+            dummyHeightMap: this.dummyHeightMap,


We are using this dummyXXXXMap pattern in a number of effects. Would it be worth having a single global dummy map to avoid having to duplicate resources?

@ibgreen Maybe this is a luma question - is it reasonable to add something like Texture.getDefaultTexture(device)? These "dummy" textures are never read in the shader, just to make sure that all samplers are supplied for the model to render.

It is indeed annoying that one cannot detect in the shader if a texture or sampler was actually supplied. Additional boolean uniforms tend to be used for this.

I suspect there is a perf advantage to the boolean uniform approach as it avoids the sample/memory read but the default texture idea is not bad for cases where this is not a perf bottleneck.

Supplying a default 1x1 texture would not be a problem. However we may need to check for WebGPU, where the setup is more rigid. No point in adding an interface in 8.9 that we have to retire in 9.0

modules/extensions/src/terrain/terrain-effect.ts

Fix case for terrain op only rename enum fix shader compilatione errors Fix layer culling when rendering to terrain cover Fix TS errors lint

modules/extensions/src/common/projection-utils.ts

modules/extensions/src/index.ts

modules/extensions/src/terrain/height-map.ts

ibgreen · 2023-02-13T11:47:30Z

modules/extensions/src/terrain/shader-module.ts

+  SKIP: 5
+};
+
+const TERRAIN_MODE_CONSTANTS = Object.keys(TERRAIN_MODE)


If we were changing them with some frequency that could make some sense. But our projection system feels quite complicated now, The saved effort in not having to change these during an update to this system must be really small.

modules/extensions/src/terrain/terrain-cover.ts

ibgreen · 2023-02-13T12:02:36Z

modules/extensions/src/terrain/terrain-effect.ts

+      ...opts,
+      pass: 'terrain-height-map',
+      viewports: [viewport],
+      layers: terrainLayers,


If we index the height information by common space coordinates instead of clipspace coordinates, then for high-pitch scenarios, the resolution closer to the camera will be very bad.

Superficially, this seems quite similar to the problems that arise for perspective shadow maps. The good news is that there are a wide range of techniques to address those.

Rendering the shadowmap in three zones (top thirds, middle third, bottom third), rendering by object distance, logarithmic shadowmaps etc. It is not hard to find good articles.

If my hunch is right, we might even be able to share such code with an improved shadow effect?

Now this implementation seems to have some nice advantages, it should have good performance characteristics for static layers, however for heavily animated layers or very dense scenes where the draw call count becomes very high with this method, it is not impossible that a more "traditional", non-tiled approach could be useful

ibgreen · 2023-02-13T12:08:44Z

modules/extensions/src/terrain/terrain-effect.ts

+          ...opts,
+          layers: drapeLayers,
+          moduleParameters: {
+            dummyHeightMap: this.dummyHeightMap,


It is indeed annoying that one cannot detect in the shader if a texture or sampler was actually supplied. Additional boolean uniforms tend to be used for this.

I suspect there is a perf advantage to the boolean uniform approach as it avoids the sample/memory read but the default texture idea is not bad for cases where this is not a perf bottleneck.

Supplying a default 1x1 texture would not be a problem. However we may need to check for WebGPU, where the setup is more rigid. No point in adding an interface in 8.9 that we have to retire in 9.0

modules/extensions/src/terrain/terrain-pass.ts

ibgreen · 2023-02-13T12:15:43Z

modules/mesh-layers/src/simple-mesh-layer/simple-mesh-layer.ts

@@ -224,6 +226,32 @@ export default class SimpleMeshLayer<DataT = any, ExtraPropsT = {}> extends Laye
    });
  }

+  getBounds(): [number[], number[]] | null {
+    if (this.props._instanced) {


What is this prop?

This was an experimental prop we introduced when working on tiled geometries. By default this layer interprets mesh.POSITION as meter offsets from the anchor (instancePosition) regardless of the anchor's coordinate system. If _instanced: false then the layer only renders 1 copy of the mesh and mesh.POSITION is interpreted as world space offset (i.e. in LNGLAT mode the vertex lnglat is calculated as instancePosition + meshPosition). It's used by the i3s and terrain use cases. I agree that the prop name does not make much sense to the user, hence the underscore.

Pessimistress requested review from felixpalmer and ibgreen January 28, 2023 23:58

Pessimistress force-pushed the x/terrain-2 branch from a17bad4 to f1ae39b Compare January 29, 2023 01:10

Pessimistress commented Jan 30, 2023

View reviewed changes

Pessimistress force-pushed the x/terrain-2 branch from a19edd8 to 5bafaed Compare January 30, 2023 22:07

Pessimistress force-pushed the x/terrain-1 branch from c48da99 to 92c03e3 Compare January 30, 2023 23:34

Base automatically changed from x/terrain-1 to master January 30, 2023 23:47

Pessimistress force-pushed the x/terrain-2 branch from 5bafaed to 7cc72aa Compare January 30, 2023 23:50

Pessimistress force-pushed the x/terrain-2 branch from 7cc72aa to 5e15705 Compare January 31, 2023 00:14

ibgreen reviewed Jan 31, 2023

View reviewed changes

Pessimistress mentioned this pull request Jan 31, 2023

ComponentState should dereference component on finalize #7612

Merged

felixpalmer requested changes Feb 2, 2023

View reviewed changes

alasarr added this to the 8.9 milestone Feb 2, 2023

Pessimistress force-pushed the x/terrain-2 branch 2 times, most recently from e4d92cd to 1e6a3d4 Compare February 4, 2023 02:20

Pessimistress added 10 commits February 10, 2023 14:31

TerrainExtension

c35bf4d

Add documentation

4961657

misc fixes

722f8c1

Fix case for terrain op only rename enum fix shader compilatione errors Fix layer culling when rendering to terrain cover Fix TS errors lint

Fix eslint warnings

9751c60

address comments

a21d5d2

address comments; improve height map

a72ed75

Minor tweaks

739aaa9

Use public API

2664c55

Redraw height map more aggressively; fix terrain picking

a3ad1c7

Misc fixes

f999da0

Pessimistress force-pushed the x/terrain-2 branch from 881c74e to f999da0 Compare February 10, 2023 22:31

Pessimistress added 2 commits February 10, 2023 14:37

Fix ts error

aed83c5

lint

2a91fd8

ibgreen approved these changes Feb 13, 2023

View reviewed changes

felixpalmer approved these changes Feb 13, 2023

View reviewed changes

Pessimistress added 2 commits February 13, 2023 12:14

address comments

f75da3b

ts fix

8ddedf1

Pessimistress merged commit dbfb31c into master Feb 13, 2023

Pessimistress deleted the x/terrain-2 branch February 13, 2023 20:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TerrainExtension (2/3) #7605

TerrainExtension (2/3) #7605

Pessimistress commented Jan 28, 2023 •

edited

Pessimistress Jan 30, 2023

ibgreen Jan 31, 2023

felixpalmer Feb 2, 2023

Pessimistress Jan 30, 2023

coveralls commented Jan 31, 2023 •

edited

ibgreen left a comment

ibgreen Jan 31, 2023

ibgreen Jan 31, 2023

Pessimistress Jan 31, 2023

felixpalmer Feb 2, 2023

felixpalmer Feb 2, 2023

Pessimistress Feb 2, 2023

felixpalmer Feb 2, 2023

Pessimistress Feb 2, 2023

felixpalmer Feb 3, 2023

ibgreen Feb 13, 2023

Pessimistress Feb 13, 2023

felixpalmer Feb 2, 2023

Pessimistress Feb 2, 2023

ibgreen Feb 13, 2023

ibgreen Feb 13, 2023

ibgreen Feb 13, 2023

ibgreen Feb 13, 2023

ibgreen Feb 13, 2023

Pessimistress Feb 13, 2023 •

edited


		const MAX_SIZE = 2048;

		// TODO - import from loaders when Tileset2D is split out

TerrainExtension (2/3) #7605

TerrainExtension (2/3) #7605

Conversation

Pessimistress commented Jan 28, 2023 • edited

Change List

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

coveralls commented Jan 31, 2023 • edited

ibgreen 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

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

Pessimistress Feb 13, 2023 • edited

Choose a reason for hiding this comment

Pessimistress commented Jan 28, 2023 •

edited

coveralls commented Jan 31, 2023 •

edited

Pessimistress Feb 13, 2023 •

edited