Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #11940 : add VerticalExaggeration option to models #12141

Merged
merged 18 commits into from
Sep 9, 2024
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
379362d
allow models to participate in scene vertical exaggeration or not
timeichfeld-msa Aug 9, 2024
94ee958
dont update husky
timeichfeld-msa Aug 9, 2024
91ff9c3
Fixes #11940 : Fix Logic. Animation - if a model had not yet been ren…
timeichfeld-msa Aug 11, 2024
04eabed
Fixes #11940 : Fix Logic. Found where the exaggeration is actually im…
timeichfeld-msa Aug 11, 2024
abf6afe
Merge branch 'CesiumGS:main' into main
timeichfeld-msa Aug 12, 2024
7419df4
Fixes #11940 : adding changes and contributors
timeichfeld-msa Aug 20, 2024
8e6adc9
Fixes #11940 : adding changes and contributors
timeichfeld-msa Aug 20, 2024
fef11cf
Merge remote-tracking branch 'origin/main'
timeichfeld-msa Aug 20, 2024
77bfabd
Fixes #11940 : adding changes and contributors
timeichfeld-msa Aug 20, 2024
85523e7
Fixes #11940 : PR Update - default to true for allowVerticalExaggeration
timeichfeld-msa Aug 22, 2024
82a2725
Merge branch 'main' into main
timeichfeld-msa Aug 29, 2024
76f1cad
Fixes #11940 : missed a couple defaults + code cleanup.
timeichfeld-msa Aug 29, 2024
d09ceb8
Fixes #11940 : missed a couple defaults + code cleanup - verticalExag…
timeichfeld-msa Aug 29, 2024
3ddfaa0
Update property naming and documentation
ggetz Sep 3, 2024
4918730
Merge branch 'main' into timeichfeld-msa/main
ggetz Sep 3, 2024
fbbbe1d
Merge remote-tracking branch 'origin/main' into timeichfeld-msa/main
ggetz Sep 3, 2024
3341fa6
Spec updates, move to 1.122 release
ggetz Sep 3, 2024
c0fa214
Merge branch 'main' into main
ggetz Sep 9, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

##### Additions :tada:


- added allowVerticalExaggeration configuration to models [#12141](https://github.com/CesiumGS/cesium/pull/12141)
- Enable MSAA by default with 4 samples. To turn MSAA off set `scene.msaaSamples = 1` [#12158](https://github.com/CesiumGS/cesium/pull/12158)
- Made the `time` parameter optional for `Property`, using `JulianDate.now()` as default. [#12099](https://github.com/CesiumGS/cesium/pull/12099)
- Exposes `ScreenSpaceCameraController.zoomFactor` to allow adjusting the zoom factor (speed). [#9145](https://github.com/CesiumGS/cesium/pull/9145)
Expand Down
2 changes: 2 additions & 0 deletions CONTRIBUTORS.md
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,8 @@ See [CONTRIBUTING.md](CONTRIBUTING.md) for details on how to contribute to Cesiu
- [T2 Software](http://t2.com.tr/)
- [Hüseyin ATEŞ](https://github.com/ateshuseyin)
- [İbrahim Furkan Aygar](https://github.com/furkanaygar)
- [MSA]
- [Timothy Eichfeld](https://github.com/timeichfeld-msa)
- [EMapGis](http://emapgis.com)
- [IKangXu](https://github.com/IKangXu)
- [EMapGIS](https://github.com/EMapGIS)
Expand Down
21 changes: 21 additions & 0 deletions packages/engine/Source/DataSources/ModelGraphics.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ function createArticulationStagePropertyBag(value) {
* @property {Property | boolean} [show=true] A boolean Property specifying the visibility of the model.
* @property {Property | string | Resource} [uri] A string or Resource Property specifying the URI of the glTF asset.
* @property {Property | number} [scale=1.0] A numeric Property specifying a uniform linear scale.
* @property {Property | boolean} [allowVerticalExaggeration=true] A boolean Property specifying to allow participation in scene Vertical Exaggeration.
* @property {Property | number} [minimumPixelSize=0.0] A numeric Property specifying the approximate minimum pixel size of the model regardless of zoom.
* @property {Property | number} [maximumScale] The maximum scale size of a model. An upper limit for minimumPixelSize.
* @property {Property | boolean} [incrementallyLoadTextures=true] Determine if textures may continue to stream in after the model is loaded.
Expand Down Expand Up @@ -70,6 +71,10 @@ function ModelGraphics(options) {
this._uriSubscription = undefined;
this._scale = undefined;
this._scaleSubscription = undefined;
this._verticalExaggerationOn = undefined;
this._verticalExaggerationOnSubscription = undefined;
this._allowVerticalExaggeration = undefined;
this._allowVerticalExaggerationSubscription = undefined;
this._minimumPixelSize = undefined;
this._minimumPixelSizeSubscription = undefined;
this._maximumScale = undefined;
Expand Down Expand Up @@ -150,6 +155,17 @@ Object.defineProperties(ModelGraphics.prototype, {
*/
scale: createPropertyDescriptor("scale"),

/**
* Gets or sets the boolean Property specifying the model to
* participate in scene Vertical Exaggeration.
* @memberof ModelGraphics.prototype
* @type {Property|undefined}
* @default false
*/
allowVerticalExaggeration: createPropertyDescriptor(
"allowVerticalExaggeration"
),

/**
* Gets or sets the numeric Property specifying the approximate minimum
* pixel size of the model regardless of zoom. This can be used to ensure that
Expand Down Expand Up @@ -333,6 +349,7 @@ ModelGraphics.prototype.clone = function (result) {
result.show = this.show;
result.uri = this.uri;
result.scale = this.scale;
result.allowVerticalExaggeration = this.allowVerticalExaggeration;
result.minimumPixelSize = this.minimumPixelSize;
result.maximumScale = this.maximumScale;
result.incrementallyLoadTextures = this.incrementallyLoadTextures;
Expand Down Expand Up @@ -370,6 +387,10 @@ ModelGraphics.prototype.merge = function (source) {
this.show = defaultValue(this.show, source.show);
this.uri = defaultValue(this.uri, source.uri);
this.scale = defaultValue(this.scale, source.scale);
this.allowVerticalExaggeration = defaultValue(
this.allowVerticalExaggeration,
source.allowVerticalExaggeration
);
this.minimumPixelSize = defaultValue(
this.minimumPixelSize,
source.minimumPixelSize
Expand Down
8 changes: 8 additions & 0 deletions packages/engine/Source/DataSources/ModelVisualizer.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import Property from "./Property.js";
import Cartographic from "../Core/Cartographic.js";

const defaultScale = 1.0;
const defaultAllowVerticalExaggeration = true;
const defaultMinimumPixelSize = 0.0;
const defaultIncrementallyLoadTextures = true;
const defaultClampAnimations = true;
Expand Down Expand Up @@ -198,6 +199,13 @@ ModelVisualizer.prototype.update = function (time) {
time,
defaultScale
);

model.allowVerticalExaggeration = Property.getValueOrDefault(
modelGraphics._allowVerticalExaggeration,
time,
defaultAllowVerticalExaggeration
);

model.minimumPixelSize = Property.getValueOrDefault(
modelGraphics._minimumPixelSize,
time,
Expand Down
55 changes: 50 additions & 5 deletions packages/engine/Source/Scene/Model/Model.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

complete.

* @privateParam {number} [options.minimumPixelSize=0.0] The approximate minimum pixel size of the model regardless of zoom.
* @privateParam {number} [options.maximumScale] The maximum scale size of a model. An upper limit for minimumPixelSize.
* @privateParam {object} [options.id] A user-defined object to return when the model is picked with {@link Scene#pick}.
Expand Down Expand Up @@ -161,7 +162,7 @@ import pickModel from "./pickModel.js";
* @privateParam {string|number} [options.instanceFeatureIdLabel="instanceFeatureId_0"] Label of the instance feature ID set used for picking and styling. If instanceFeatureIdLabel is set to an integer N, it is converted to the string "instanceFeatureId_N" automatically. If both per-primitive and per-instance feature IDs are present, the instance feature IDs take priority.
* @privateParam {object} [options.pointCloudShading] Options for constructing a {@link PointCloudShading} object to control point attenuation based on geometric error and lighting.
* @privateParam {ClassificationType} [options.classificationType] Determines whether terrain, 3D Tiles or both will be classified by this model. This cannot be set after the model has loaded.

*
*
* @see Model.fromGltfAsync
*
Expand Down Expand Up @@ -340,6 +341,10 @@ function Model(options) {
this._heightDirty = this._heightReference !== HeightReference.NONE;
this._removeUpdateHeightCallback = undefined;

this._allowVerticalExaggeration = defaultValue(
options.allowVerticalExaggeration,
false
);
this._verticalExaggerationOn = false;

this._clampedModelMatrix = undefined; // For use with height reference
Expand Down Expand Up @@ -1339,6 +1344,39 @@ Object.defineProperties(Model.prototype, {
},
},

/**
* Enable Models to participate in scene verticalExaggeration.
*
* @memberof Model.prototype
*
* @type {boolean}
*/
allowVerticalExaggeration: {
get: function () {
return this._allowVerticalExaggeration;
},
set: function (value) {
if (value !== this._allowVerticalExaggeration) {
this.resetDrawCommands();
//this._verticalExaggerationDirty = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we can drop this commented line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

}
this._allowVerticalExaggeration = value;
},
},

/**
* State of VerticalExaggeration (true = enabled, false = disabled).
*
* @memberof Model.prototype
*
* @type {boolean}
*/
verticalExaggerationOn: {
get: function () {
return this._verticalExaggerationOn;
},
},

/**
* The light color when shading the model. When <code>undefined</code> the scene's light color is used instead.
* <p>
Expand Down Expand Up @@ -2062,10 +2100,15 @@ function updateFog(model, frameState) {
}

function updateVerticalExaggeration(model, frameState) {
const verticalExaggerationNeeded = frameState.verticalExaggeration !== 1.0;
if (model._verticalExaggerationOn !== verticalExaggerationNeeded) {
model.resetDrawCommands();
model._verticalExaggerationOn = verticalExaggerationNeeded;
if (model.allowVerticalExaggeration) {
const verticalExaggerationNeeded = frameState.verticalExaggeration !== 1.0;
if (model.verticalExaggerationOn !== verticalExaggerationNeeded) {
model.resetDrawCommands();
model._verticalExaggerationOn = verticalExaggerationNeeded;
}
} else if (model.verticalExaggerationOn) {
model.resetDrawCommands(); //if verticalExaggeration was on, reset.
model._verticalExaggerationOn = false;
}
}

Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another place to change the default

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

complete

* @param {number} [options.minimumPixelSize=0.0] The approximate minimum pixel size of the model regardless of zoom.
* @param {number} [options.maximumScale] The maximum scale size of a model. An upper limit for minimumPixelSize.
* @param {object} [options.id] A user-defined object to return when the model is picked with {@link Scene#pick}.
Expand Down Expand Up @@ -3116,6 +3160,7 @@ function makeModelOptions(loader, modelType, options) {
show: options.show,
modelMatrix: options.modelMatrix,
scale: options.scale,
allowVerticalExaggeration: options.allowVerticalExaggeration,
minimumPixelSize: options.minimumPixelSize,
maximumScale: options.maximumScale,
id: options.id,
Expand Down
3 changes: 2 additions & 1 deletion packages/engine/Source/Scene/Model/ModelRuntimePrimitive.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

complete

frameState.verticalExaggeration !== 1.0 && model.verticalExaggerationOn;

const hasMorphTargets =
defined(primitive.morphTargets) && primitive.morphTargets.length > 0;
Expand Down
2 changes: 2 additions & 0 deletions packages/engine/Specs/Scene/Model/ModelSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -517,6 +517,8 @@ describe(
expect(model.id).toBeUndefined();
expect(model.allowPicking).toEqual(true);

expect(model.allowVerticalExaggeration).toEqual(true);

expect(model.activeAnimations).toBeDefined();
expect(model.clampAnimations).toEqual(true);

Expand Down
Loading