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

Make time parameter optional for Property #12099

Merged
merged 4 commits into from
Aug 2, 2024
Merged

Conversation

anne-gropler
Copy link
Contributor

@anne-gropler anne-gropler commented Jul 30, 2024

Description

The callback in a CallbackProperty could do anything, even ignoring the time parameter.

Users in typescript strict mode always have to pass a time parameter, even if their callback doesn't use it, and are forced to write something like .getValue(undefined!).

This pull requests makes the time paramter optional for all Propertys, so that users can write .getValue() without problems. As suggested in the linked issue comments, a default for time (i.e. JulianDate.now()) is used for compatibility. This default was added for all properties, because the abstract Property could otherwise not make the time parameter optional.

Issue number and link

Closes #12009

Testing plan

Added tests that check that JulianDate.now() is called when time is undefined.

Built Cesium and linked it in another typescript app (strict mode) and checked that the time parameter is indeed optional now.

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

Copy link

Thank you for the pull request, @anne-gropler!

✅ We can confirm we have a CLA on file for you.

@anne-gropler anne-gropler force-pushed the main branch 8 times, most recently from 5c717f1 to b0e5757 Compare July 31, 2024 07:27
@anne-gropler anne-gropler marked this pull request as ready for review July 31, 2024 07:44
@anne-gropler anne-gropler changed the title Make time parameter optional for CallbackProperty Make time parameter optional for Property Jul 31, 2024
Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @anne-gropler! I have a two comments which apply to each of the files changed.

* @param {object} [result] The object to store the value into, if omitted, a new instance is created and returned.
* @returns {object} The modified result parameter or a new instance if the result parameter was not supplied or is unsupported.
*/
CallbackProperty.prototype.getValue = function (time, result) {
if (!defined(time)) {
time = JulianDate.now();
Copy link
Contributor

Choose a reason for hiding this comment

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

This will allocate a new object each time getValue is called without a time parameter. To avoid a performance hit, we should use a scratch variable.

Copy link
Contributor Author

@anne-gropler anne-gropler Aug 1, 2024

Choose a reason for hiding this comment

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

Do you mean something like this?

let timeScratch = new JulianDate();

/**
 * Gets the value of the property.
 *
 * @param {JulianDate} [time=JulianDate.now()] The time for which to retrieve the value. If omitted, the current system time is used.
 * @param {object} [result] The object to store the value into, if omitted, a new instance is created and returned.
 * @returns {object} The modified result parameter or a new instance if the result parameter was not supplied or is unsupported.
 */
CallbackProperty.prototype.getValue = function (time, result) {
    if (!defined(time)) {
    timeScratch = JulianDate.now();
  }

  return this._callback(defined(time) ? time : timeScratch, result);
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Close! The JulianDate.now function is designed to take a result parameter. Instead of creating a new JulianDate object, it'll be copied into the existing scratch variable:

const timeScratch = new JulianDate();

/**
 * Gets the value of the property.
 *
 * @param {JulianDate} [time=JulianDate.now()] The time for which to retrieve the value. If omitted, the current system time is used.
 * @param {object} [result] The object to store the value into, if omitted, a new instance is created and returned.
 * @returns {object} The modified result parameter or a new instance if the result parameter was not supplied or is unsupported.
 */
CallbackProperty.prototype.getValue = function (time, result) {
    if (!defined(time)) {
    time = JulianDate.now(timeScratch);
  }

  return this._callback(time);
};

Does that make sense?

Copy link
Contributor Author

@anne-gropler anne-gropler Aug 1, 2024

Choose a reason for hiding this comment

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

Ohh, right! This does make more sense!

I adapted the code. Thank you for your patience!

@@ -49,11 +50,14 @@ Object.defineProperties(CallbackProperty.prototype, {
/**
* Gets the value of the property.
*
* @param {JulianDate} time The time for which to retrieve the value.
* @param {JulianDate} [time=now] The time for which to retrieve the value. If omitted, now is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest:

Suggested change
* @param {JulianDate} [time=now] The time for which to retrieve the value. If omitted, now is used.
* @param {JulianDate} [time=JulianDate.now()] The time for which to retrieve the value. If omitted, the current system time is used.

just to be a bit more precise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@anne-gropler anne-gropler force-pushed the main branch 5 times, most recently from 6fc345b to 05d8ac8 Compare August 1, 2024 13:54
Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

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

Awesome, thanks @anne-gropler!

CHANGES.md Outdated
@@ -9,6 +9,7 @@
- Added `Transforms.computeIcrfToMoonFixedMatrix` and `Transforms.computeMoonFixedToIcrfMatrix` to compute the transformations between the Moon's fixed frame and ICRF at a given time.
- Added `Transforms.computeIcrfToCentralBodyFixedMatrix` to specific the default ICRF to fixed frame transformation to use internally, including for lighting calculations.
- Added SplitDirection property for display PointPrimitive and Billboard relative to the `Scene.splitPosition`. [#11982](https://github.com/CesiumGS/cesium/pull/11982)
- Made the `time` parameter optional for `Property`, using `JulianDate.now()` as default. [#12099](https://github.com/CesiumGS/cesium/pull/12099)
Copy link
Contributor

Choose a reason for hiding this comment

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

One last thing: Please move this under a new section.

### 1.121 - 2024-09-01

Copy link
Contributor Author

Choose a reason for hiding this comment

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

new month, new section :) done!

@ggetz
Copy link
Contributor

ggetz commented Aug 2, 2024

Perfect, thanks for the PR @anne-gropler!

@ggetz ggetz merged commit 0a69f67 into CesiumGS:main Aug 2, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

time param of CallbackProperty.Callback should be optional
2 participants