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.

Sign up for GitHub

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

Build an ISO compliant string - 11507 #12155

Closed
wants to merge 8 commits into from
Closed

Build an ISO compliant string - 11507 #12155

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
48bbf8b
add a fix so that the output is iso compliant
Alforoan Aug 26, 2024
36fdc6c
update changes.md and contributors.md
Alforoan Aug 26, 2024
893dc6f
Merge branch 'main' into jay_issue_11507
Alforoan Aug 26, 2024
5314819
Merge branch 'main' into jay_issue_11507
Alforoan Sep 20, 2024
e89af80
change line in changes.md
Alforoan Sep 20, 2024
30bacc8
add a contributor back
Alforoan Sep 23, 2024
6ac8ad8
remove default precision
Alforoan Sep 23, 2024
5bdb97d
moved descrption to under the latest release
Alforoan Sep 23, 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
3 changes: 0 additions & 3 deletions .husky/pre-commit
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1 @@
#!/bin/sh
. "$(dirname "$0")/_/husky.sh"

npx lint-staged
1 change: 1 addition & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ This is an npm-only release to extra source maps included in 1.121
- Fixed environment map transform for image-based lighting. [#12091](https://github.com/CesiumGS/cesium/pull/12091)
- Updated geometric self-shadowing function to improve direct lighting on models using physically-based rendering. [#12063](https://github.com/CesiumGS/cesium/pull/12063)
- Prevent Bing Imagery API format issues from throwing errors [#12094](https://github.com/CesiumGS/cesium/pull/12094)
- Fix JulianDate to always generate valid ISO strings for fractional milliseconds [#11507](https://github.com/CesiumGS/cesium/issues/11507)
jjspace marked this conversation as resolved.
Show resolved Hide resolved

### 1.119 - 2024-07-01

Expand Down
2 changes: 1 addition & 1 deletion CONTRIBUTORS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,7 @@ See [CONTRIBUTING.md](CONTRIBUTING.md) for details on how to contribute to Cesiu
- [Vladislav Yunev](https://github.com/YunVlad)
- [Levi Montgomery](https://github.com/Levi-Montgomery)
- [Brandon Berisford](https://github.com/BeyondBelief96)
- [Jay Yong](https://github.com/Alforoan)
- [Lawrence Owen](https://github.com/ljowen)
- [Adam Wirth](https://github.com/adamwirth)
jjspace marked this conversation as resolved.
Show resolved Hide resolved
- [Javier Sanchez](https://github.com/jvrjsanchez)
- [Jérôme Fayot](https://github.com/jfayot)
50 changes: 13 additions & 37 deletions packages/engine/Source/Core/JulianDate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -783,40 +783,16 @@ JulianDate.toIso8601 = function (julianDate, precision) {

let millisecondStr;

if (!defined(precision) && millisecond !== 0) {
//Forces milliseconds into a number with at least 3 digits to whatever the default toString() precision is.
millisecondStr = (millisecond * 0.01).toString().replace(".", "");
return `${year.toString().padStart(4, "0")}-${month
.toString()
.padStart(2, "0")}-${day
.toString()
.padStart(2, "0")}T${hour
.toString()
.padStart(2, "0")}:${minute
.toString()
.padStart(2, "0")}:${second
.toString()
.padStart(2, "0")}.${millisecondStr}Z`;
}

//Precision is either 0 or milliseconds is 0 with undefined precision, in either case, leave off milliseconds entirely
if (!defined(precision) || precision === 0) {
return `${year.toString().padStart(4, "0")}-${month
.toString()
.padStart(2, "0")}-${day
.toString()
.padStart(2, "0")}T${hour
.toString()
.padStart(2, "0")}:${minute
.toString()
.padStart(2, "0")}:${second.toString().padStart(2, "0")}Z`;
}

//Forces milliseconds into a number with at least 3 digits to whatever the specified precision is.
millisecondStr = (millisecond * 0.01)
.toFixed(precision)
.replace(".", "")
.slice(0, precision);
if (millisecond !== 0 || defined(precision)) {
if (!defined(precision)) {
precision = 7; // Default precision if not provided
}
jjspace marked this conversation as resolved.
Show resolved Hide resolved
// Forces milliseconds into a number with the specified precision, without scientific notation
const fractionalSeconds = (second + millisecond * 0.001).toFixed(precision);
const parts = fractionalSeconds.split('.');
millisecondStr = parts.length > 1 ? parts[1] : '';
Comment on lines +788 to +790
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you expand on why you're adding the seconds here then splitting on the .? I think there may potentially be other, better, ways to get the decimal part of a number if that's the desire.

Also the original issue called out that toFixed does not necessarily eliminate the issue of getting scientific notation when converting a number to string if the precision is right (see the docs). Does this addition avoid that somehow? or is that not accounted for?

Copy link
Author

Choose a reason for hiding this comment

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

I am splitting on . to separate the fractional part (milliseconds) from the whole seconds.
I am not getting scientific notations for very small numbers.

Copy link
Contributor

Choose a reason for hiding this comment

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

My initial thought is that splitting on . is a bad way to manipulate numbers but looking into it a bit this seems like a fine way to do it. However, I'm still unclear about why you're adding the seconds to it first if you just strip them off.
It's possible there's a more simple solution that does include combining the seconds and milliseconds that looks something like this (n * 0.001 + 45).toFixed(precision).padStart(2 + precision + 1, '0'). Just pay special attention to the padding for the seconds.

Also it looks like we have constants for some of these "magic numbers", please swap out 0.001 with TimeConstants.SECONDS_PER_MILLISECOND as we use elsewhere in this code.

}

return `${year.toString().padStart(4, "0")}-${month
.toString()
.padStart(2, "0")}-${day
Expand All @@ -825,9 +801,9 @@ JulianDate.toIso8601 = function (julianDate, precision) {
.toString()
.padStart(2, "0")}:${minute
.toString()
.padStart(2, "0")}:${second
.toString()
.padStart(2, "0")}.${millisecondStr}Z`;
.padStart(2, "0")}:${Math.floor(second).toString().padStart(2, "0")}${
millisecondStr ? `.${millisecondStr}` : ''
}Z`;
};

/**
Expand Down
Loading