Fix #11940 : add VerticalExaggeration option to models #12141

timeichfeld-msa · 2024-08-20T17:50:08Z

Description

added allowVerticalExaggeration configuration to models:
This is currently defaulted to false.

Models should not exaggerate with the terrain unless specified to do so.

Issue number and link

Turn off vertical exaggeration for some Models in the Scene Fixes #11940

Testing plan

in a local Local Sandcastle instance.
Set allowVerticalExaggeration = true and run Cesium

tested with reatlime + changes and static + changes.
Set allowVerticalExaggeration = false and run Cesium
tested with reatlime + changes and static + changes.

Run the following code.



const viewer = new Cesium.Viewer("cesiumContainer", {
  timeline: false,
  animation: false,
  terrainProvider: await Cesium.createWorldBathymetryAsync({
    requestVertexNormals: true,
  }),
});

viewer.baseLayerPicker.viewModel.selectedImagery =
  viewer.baseLayerPicker.viewModel.imageryProviderViewModels[11];

const scene = viewer.scene;

scene.verticalExaggeration = 1.1;

//-----------------------------------------------------------------------------------------
function createModel(url, height) {
  viewer.entities.removeAll();

    const positionexaggerate = Cesium.Cartesian3.fromDegrees(
    -123.0746000,
    44.0505000,
    height
  );
  
  const position = Cesium.Cartesian3.fromDegrees(
    -123.0744619,
    44.0503706,
    height
  );
  
  const positionlook = Cesium.Cartesian3.fromDegrees(
    -123.0744619,
    44.0503706,
    10000
  );
  
  const heading = Cesium.Math.toRadians(135);
  const pitch = 0;
  const roll = 0;
  const hpr = new Cesium.HeadingPitchRoll(heading, pitch, roll);
  const orientation = Cesium.Transforms.headingPitchRollQuaternion(
    position,
    hpr
  );

  const item = {
      uri: url,
      minimumPixelSize: 128,
      maximumScale: 20000,
     allowVerticalExaggeration: false,
    };

  
  const entity = viewer.entities.add({
    name: "Exaggerated (false)",
    position: position,
    orientation: orientation,
    model: item,
  });
  
//allowVerticalExaggeration is true by default
  const itemexaggerate = {
      uri: url,
      minimumPixelSize: 128,
      maximumScale: 20000,
      //allowVerticalExaggeration: true,
  };
  
    const entityexaggerate = viewer.entities.add({
    name: "Exaggerated (true)",
    position: positionexaggerate,
    orientation: orientation,
    model: itemexaggerate,
  });
  
  
  
    //viewer.trackedEntity = entityexaggerate;
  viewer.scene.camera.flyTo({
    destination: positionlook
  });
  
  viewer.trackedEntity = entity;
    
}

const options = [
  {
    text: "Milk Truck",
    onselect: function () {
      createModel(
        "../../SampleData/models/CesiumMilkTruck/CesiumMilkTruck.glb",
        0
      );
    },
  },
    {
    text: "Ground Vehicle",
    onselect: function () {
      createModel(
        "../../SampleData/models/GroundVehicle/GroundVehicle.glb",
        0
      );
    },
  },
  {
    text: "Aircraft",
    onselect: function () {
      createModel(
        "../../SampleData/models/CesiumAir/Cesium_Air.glb",
        5000.0
      );
    },
  },
  {
    text: "Drone",
    onselect: function () {
      createModel(
        "../../SampleData/models/CesiumDrone/CesiumDrone.glb",
        150.0
      );
    },
  },
  {
    text: "Hot Air Balloon",
    onselect: function () {
      createModel(
        "../../SampleData/models/CesiumBalloon/CesiumBalloon.glb",
        1000.0
      );
    },
  },
  
  {
    text: "Skinned Character",
    onselect: function () {
      createModel(
        "../../SampleData/models/CesiumMan/Cesium_Man.glb",
        0
      );
    },
  },
  {
    text: "Unlit Box",
    onselect: function () {
      createModel(
        "../../SampleData/models/BoxUnlit/BoxUnlit.gltf",
        10.0
      );
    },
  },
  {
    text: "Draco Compressed Model",
    onselect: function () {
      createModel(
        "../../SampleData/models/DracoCompressed/CesiumMilkTruck.gltf",
        0
      );
    },
  },
  {
    text: "KTX2 Compressed Balloon",
    onselect: function () {
      if (!Cesium.FeatureDetection.supportsBasis(viewer.scene)) {
        window.alert(
          "This browser does not support Basis Universal compressed textures"
        );
      }
      createModel(
        "../../SampleData/models/CesiumBalloonKTX2/CesiumBalloonKTX2.glb",
        1000.0
      );
    },
  },
  {
    text: "Instanced Box",
    onselect: function () {
      createModel(
        "../../SampleData/models/BoxInstanced/BoxInstanced.gltf",
        15
      );
    },
  },
];

Sandcastle.addToolbarMenu(options);

Author checklist

I have submitted a Contributor License Agreement
I have added my name to CONTRIBUTORS.md
I have updated CHANGES.md with a short summary of my change
I have added or updated unit tests to ensure consistent code coverage
I have updated the inline documentation, and included code examples where relevant
I have performed a self-review of my code

…been rendered on the map, verticalExaggeration != 1.0 and allowVerticalExaggeration was set to false, it would still exaggerate. Capture state and reset model.

…ually implemented. Fixed accordingly.

github-actions · 2024-08-20T17:50:30Z

Thank you for the pull request, @timeichfeld-msa! Welcome to the Cesium community!

In order for us to review your PR, please complete the following steps:

Send in a Contributor License Agreement (CLA)
Add yourself to the contributors file

Review Pull Request Guidelines to make sure your PR gets accepted quickly.

ggetz · 2024-08-21T19:54:30Z

Hi @timeichfeld-msa, thanks for the PR! I can confirm we now have a CLA on file for you.

We will review this PR shortly!

@jjhembd Can you please add your thoughts?

jjhembd · 2024-08-21T20:18:24Z

Hi @timeichfeld-msa, many thanks for this contribution! We have had several users asking for it.

I just have one comment about the default: I think it would be better for allowVerticalExaggeration to default to true. Two reasons for this:

Exaggeration of buildings, vehicles, and UI elements are important visual clues that the terrain is not realistic. Many geoscientists have objected to showing terrain with vertical exaggeration, without obvious indications that it is exaggerated. See this post from a NASA scientist and this paper from a geologist. If a user really does need to show exaggerated terrain with un-exaggerated Models, then they can set allowVerticalExaggeration = false (and use a scale bar or other tool to communicate the exaggeration).
The exaggeration in Model is how we exaggerate 3D Tilesets, including tilesets that are used as a substitute for terrain. See for example the Google Photorealistic 3D Tiles. These should be exaggerated by default.

timeichfeld-msa · 2024-08-22T18:19:41Z

Hi Jeshurun,

Understood, makes sense. I can make those changes.
I am on vacation until next week, I can submit the default update when I return :)

Please let me know about the protobufjs/dist/minimal/protobuf.js error and guidance if I need to take a look.

Thanks much,
-Timothy

ggetz · 2024-08-22T18:25:33Z

@timeichfeld-msa The protobufjs error is being addressed in #12144.

The only action needed on your end will be to merge in the main branch when you return. Thanks!

…xaggeration

timeichfeld-msa · 2024-08-29T13:33:59Z

@jjhembd, @ggetz
Hello, the PR with the above changes (default to true) + updates for Changes/Contributors is ready to go.
Please let me know if there is anything else needed.

Thasnk much!
-Timothy

jjhembd

Thanks @timeichfeld-msa! I added a few quick observations for now. I can look more carefully next week.

The tests are showing a few failures. Can you please check these? See the Testing Guide for details on how to run the tests on your machine.

jjhembd · 2024-08-29T19:41:48Z

packages/engine/Source/Scene/Model/Model.js

@@ -123,6 +123,7 @@ import pickModel from "./pickModel.js";
 * @privateParam {boolean} [options.show=true] Whether or not to render the model.
 * @privateParam {Matrix4} [options.modelMatrix=Matrix4.IDENTITY]  The 4x4 transformation matrix that transforms the model from model to world coordinates.
 * @privateParam {number} [options.scale=1.0] A uniform scale applied to this model.
+ * @privateParam {boolean} [options.allowVerticalExaggeration=false] Allows the model to participate in vertical exaggeration.


I think we will also want this class to default to true

jjhembd · 2024-08-29T19:42:23Z

packages/engine/Source/Scene/Model/Model.js

+    set: function (value) {
+      if (value !== this._allowVerticalExaggeration) {
+        this.resetDrawCommands();
+        //this._verticalExaggerationDirty = true;


I assume we can drop this commented line?

jjhembd · 2024-08-29T19:43:56Z

packages/engine/Source/Scene/Model/ModelRuntimePrimitive.js

@@ -199,7 +199,8 @@ ModelRuntimePrimitive.prototype.configurePipeline = function (frameState) {
  const mode = frameState.mode;
  const use2D =
    mode !== SceneMode.SCENE3D && !frameState.scene3DOnly && model._projectTo2D;
-  const exaggerateTerrain = frameState.verticalExaggeration !== 1.0;
+  const exaggerateTerrain =


Can we rename this variable to exaggerationOn or something like that? The leftover "terrain" naming looks like an oversight from the previous change to verticalExaggeration

jjhembd · 2024-08-29T19:44:06Z

packages/engine/Source/Scene/Model/Model.js

@@ -2748,6 +2791,7 @@ Model.prototype.destroyModelResources = function () {
 * @param {boolean} [options.show=true] Whether or not to render the model.
 * @param {Matrix4} [options.modelMatrix=Matrix4.IDENTITY] The 4x4 transformation matrix that transforms the model from model to world coordinates.
 * @param {number} [options.scale=1.0] A uniform scale applied to this model.
+ * @param {boolean} [options.allowVerticalExaggeration=false] Allows the model to participate in Vertical Exaggeration


Another place to change the default

timeichfeld-msa · 2024-08-29T21:11:38Z

@jjhembd
Hi Jeshurun, I think I need a little help.
I am getting errors running coverage tests:
I cant see where the error originates from the cmd output.

Chrome 128.0.0.0 (Windows 10) ERROR
An error was thrown in afterAll
Unhandled promise rejection: undefined thrown
Unhandled promise rejection: undefined thrown
...

Chrome won't open in the automation.
it is in the cleared list of programs, and I set chrome://flags/#allow-insecure-localhost to enabled.

I made the suggestions above, and eslint appears to have passed.

We can catch up next week.

Thanks much,
-Timothy

-=-=-
More info.
Running all tests in the browser: not sure how to get these in the correct order. Please advise.

…icalExaggeration tests..

ggetz · 2024-09-03T15:56:49Z

Hi @timeichfeld-msa, thanks for the update! I pushed a small change to your branch which changes the naming slightly to align with our existing properties. I also fixed and added some additional specs.

I noticed #11936 and #11974 are still indeed issues, but those is out of the scope of this PR.

@jjhembd Can you do a final review and merge if happy (after the 1.121 release is complete)?

ggetz · 2024-09-09T18:52:24Z

Thanks @timeichfeld-msa!

timeichfeld-msa added 5 commits August 9, 2024 10:29

allow models to participate in scene vertical exaggeration or not

379362d

dont update husky

94ee958

Fixes CesiumGS#11940 : Fix Logic. Animation - if a model had not yet …

91ff9c3

…been rendered on the map, verticalExaggeration != 1.0 and allowVerticalExaggeration was set to false, it would still exaggerate. Capture state and reset model.

Fixes CesiumGS#11940 : Fix Logic. Found where the exaggeration is act…

04eabed

…ually implemented. Fixed accordingly.

Merge branch 'CesiumGS:main' into main

abf6afe

timeichfeld-msa added 4 commits August 20, 2024 13:58

Fixes CesiumGS#11940 : adding changes and contributors

7419df4

Fixes CesiumGS#11940 : adding changes and contributors

8e6adc9

Merge remote-tracking branch 'origin/main'

fef11cf

Fixes CesiumGS#11940 : adding changes and contributors

77bfabd

timeichfeld-msa added 2 commits August 22, 2024 16:15

Fixes CesiumGS#11940 : PR Update - default to true for allowVerticalE…

85523e7

…xaggeration

Merge branch 'main' into main

82a2725

jjhembd reviewed Aug 29, 2024

View reviewed changes

timeichfeld-msa and others added 3 commits August 29, 2024 17:13

Fixes CesiumGS#11940 : missed a couple defaults + code cleanup.

76f1cad

Fixes CesiumGS#11940 : missed a couple defaults + code cleanup - vert…

d09ceb8

…icalExaggeration tests..

Update property naming and documentation

3ddfaa0

ggetz mentioned this pull request Sep 3, 2024

Unable to zoom in after setting the relative height of vertical exaggeration #11974

Open

ggetz added 3 commits September 3, 2024 11:40

Merge branch 'main' into timeichfeld-msa/main

4918730

Merge remote-tracking branch 'origin/main' into timeichfeld-msa/main

fbbbe1d

Spec updates, move to 1.122 release

3341fa6

Merge branch 'main' into main

c0fa214

ggetz merged commit 2cf09cb into CesiumGS:main Sep 9, 2024
4 checks passed

ggetz mentioned this pull request Oct 18, 2024

Setting verticalExaggeration greater than 1 affects 3D models #12256

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #11940 : add VerticalExaggeration option to models #12141

Fix #11940 : add VerticalExaggeration option to models #12141

timeichfeld-msa commented Aug 20, 2024 •

edited

Loading

github-actions bot commented Aug 20, 2024

ggetz commented Aug 21, 2024

jjhembd commented Aug 21, 2024

timeichfeld-msa commented Aug 22, 2024

ggetz commented Aug 22, 2024

timeichfeld-msa commented Aug 29, 2024

jjhembd left a comment

jjhembd Aug 29, 2024

timeichfeld-msa Aug 29, 2024

jjhembd Aug 29, 2024

timeichfeld-msa Aug 29, 2024

jjhembd Aug 29, 2024

timeichfeld-msa Aug 29, 2024

jjhembd Aug 29, 2024

timeichfeld-msa Aug 29, 2024

timeichfeld-msa commented Aug 29, 2024 •

edited

Loading

ggetz commented Sep 3, 2024

ggetz commented Sep 9, 2024

Fix #11940 : add VerticalExaggeration option to models #12141

Fix #11940 : add VerticalExaggeration option to models #12141

Conversation

timeichfeld-msa commented Aug 20, 2024 • edited Loading

Description

Issue number and link

Testing plan

Run the following code.

Author checklist

github-actions bot commented Aug 20, 2024

ggetz commented Aug 21, 2024

jjhembd commented Aug 21, 2024

timeichfeld-msa commented Aug 22, 2024

ggetz commented Aug 22, 2024

timeichfeld-msa commented Aug 29, 2024

jjhembd 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

timeichfeld-msa commented Aug 29, 2024 • edited Loading

@jjhembd Hi Jeshurun, I think I need a little help. I am getting errors running coverage tests: I cant see where the error originates from the cmd output.

ggetz commented Sep 3, 2024

ggetz commented Sep 9, 2024

timeichfeld-msa commented Aug 20, 2024 •

edited

Loading

timeichfeld-msa commented Aug 29, 2024 •

edited

Loading

@jjhembd
Hi Jeshurun, I think I need a little help.
I am getting errors running coverage tests:
I cant see where the error originates from the cmd output.