-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Adds maximum scale property to models #3164
Conversation
Thanks for this contribution, @cttibbetts! It looks useful. Can you please send in a Contributor License Agreement so we can review this? |
Sent in the CLA. Thank you! |
@@ -31,6 +31,7 @@ define([ | |||
* @param {Property} [options.show=true] A boolean Property specifying the visibility of the model. | |||
* @param {Property} [options.scale=1.0] A numeric Property specifying a uniform linear scale. | |||
* @param {Property} [options.minimumPixelSize=0.0] A numeric Property specifying the approximate minimum pixel size of the model regardless of zoom. | |||
* @param {Property} [options.maximumScale] The maximum scale size of a model. An upper limit for minimumPixelSize. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add this and the changes to Model
to the 1.16 section of CHANGES.md?
This looks good, @cttibbetts! Just a bunch of doc comments. Can you also add unit tests? See ModelGraphicsSpec and ModelSpec. |
I think this is ready now. I have a few questions/disputes in the comments. |
@@ -125,6 +125,7 @@ define([ | |||
model.show = true; | |||
model.scale = Property.getValueOrDefault(modelGraphics._scale, time, defaultScale); | |||
model.minimumPixelSize = Property.getValueOrDefault(modelGraphics._minimumPixelSize, time, defaultMinimumPixelSize); | |||
model.maximumScale = Property.getValueOrDefault(modelGraphics._maximumScale, time, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I didn't catch this before, use getValueOrUndefined
here.
Switched to use getValueOrUndefined. |
@cttibbetts I merged your commits with PR #3167 with just added commit c9e402b for some corner cases that I didn't want to bother you with. Thanks again for this PR, it is a cool feature. |
Adding a maximum scale property to models allows them to scale up (using minimumPixelSize) to a certain point, then stop growing. This allows models that are useful at many zoom levels.
Before, when fully zoomed out, it was possible to have models that covered continents or even the whole world, which is not particularly useful.