remove defaultValue() and create DefaultValue namespace #12252

dave-b-b · 2024-10-13T23:04:15Z

Description

I started over because I could not get the tests to pass
removed defaultValue() in 'easy' to spot references
fetched most up to date copy of cesium, so that my pull request wasn't cluttered

Issue number and link

Issue: 11674
#11674

Testing plan

The following tests failed:
✗ renders a point
✗ renders pnts with point size style

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

- I started over because I could not get the tests to pass - removed defaultValue() in 'easy' to spot references - fetched most up to date copy of cesium, so that my pull request wasn't cluttered

github-actions · 2024-10-13T23:04:35Z

Thank you for the pull request, @dave-b-b!

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

dave-b-b · 2024-10-13T23:05:33Z

@jjspace Can you delete the other commits I had? I started over because I couldn't get the tests to pass. This one is much cleaner. If I can get help on the two tests that are failing, then I can move forward with this one.

jjspace · 2024-10-14T18:15:17Z

@dave-b-b I'm a little confused on the state of this with #12207 still open? should that PR be closed and only plan to merge this one?
All the tests look like they're passing on this branch, which ones are you having issues with?
I also still see a bunch of instances of defaultValue() being used on this branch, are there commits missing? is this ready to review?

dave-b-b · 2024-10-15T00:52:26Z

@jjspace Yes, I could not get the tests to pass on #12207, so I started over. You can delete that pull request because I redid everything here.
I had two tests fail:
✗ renders a point
✗ renders pnts with point size style

They weren't passing on my machine. I'm almost done with this pull request. I'm trying to finish everything right now.

- I've finished making all the changes, but a few tests are failing:

dave-b-b · 2024-10-15T02:31:16Z

@jjspace I finished and submitted the final pull request, but I'm not sure whether I should rebase/merge.

A few of the tests are failing, so I must have messed something up. Can you help with this?

destroys resources after pixel datatype changes
Renderer/FramebufferManager
destroys resources after pixel format changes
Renderer/FramebufferManager
BalloonStyle: creates table from ExtendedData
DataSources/KmlDataSource
Parse Camera and LookAt on features
DataSources/KmlDataSource
recreates resources when HDR changes
Scene/GlobeTranslucencyFramebuffer
white
Scene/PostProcessStageCollection HDR tonemapping Reinhard
red
Scene/PostProcessStageCollection HDR tonemapping Reinhard
green
Scene/PostProcessStageCollection HDR tonemapping Reinhard
blue
Scene/PostProcessStageCollection HDR tonemapping Reinhard
white
Scene/PostProcessStageCollection HDR tonemapping Filmic
red
Scene/PostProcessStageCollection HDR tonemapping Filmic
green
Scene/PostProcessStageCollection HDR tonemapping Filmic
blue
Scene/PostProcessStageCollection HDR tonemapping Filmic
white
Scene/PostProcessStageCollection HDR tonemapping ACES
red
Scene/PostProcessStageCollection HDR tonemapping ACES
green
Scene/PostProcessStageCollection HDR tonemapping ACES
blue
Scene/PostProcessStageCollection HDR tonemapping ACES
white
Scene/PostProcessStageCollection HDR tonemapping PBR Neutral
grey
Scene/PostProcessStageCollection HDR tonemapping PBR Neutral
red
Scene/PostProcessStageCollection HDR tonemapping PBR Neutral
green
Scene/PostProcessStageCollection HDR tonemapping PBR Neutral
blue
Scene/PostProcessStageCollection HDR tonemapping PBR Neutral
renders with HDR when available
Scene/Scene
renders a point
Scene/Vector3DTilePoints
renders pnts with point size style
Scene/Model/Model3DTileContent pnts
changing specularEnvironmentMaps works
Scene/Model/Model imageBasedLighting

- change CesiumMath.toRadians((tilt, 0.0)) to CesiumMath.toRadians(tilt ?? 0.0) - put the parenthesis in the right place to fix the logic in FramebuggerManager lines 143 - 151

jjspace

Did a pass on just the code and spotted a handful of text errors. looks like you might've fixed one already but take a look. I will try and look at the test failures soon if they're still happening.

jjspace · 2024-10-15T21:39:35Z

packages/engine/Source/DataSources/KmlDataSource.js

+        text += `<tr><th>${
+          value.displayName ?? key
+        }</th><td>${(value.value, "")}</td></tr>`;


Suggested change

text += `<tr><th>${

value.displayName ?? key

}</th><td>${(value.value, "")}</td></tr>`;

text += `<tr><th>${

value.displayName ?? key

}</th><td>${value.value ?? ""}</td></tr>`;

jjspace · 2024-10-15T21:40:24Z

packages/engine/Source/DataSources/KmlDataSource.js


-    tilt = CesiumMath.toRadians(defaultValue(tilt, 0.0));
-    heading = CesiumMath.toRadians(defaultValue(heading, 0.0));
+    tilt = CesiumMath.toRadians((tilt, 0.0));


Suggested change

tilt = CesiumMath.toRadians((tilt, 0.0));

tilt = CesiumMath.toRadians(tilt ?? 0.0);

packages/engine/Source/Scene/GltfPipeline/addDefaults.js

packages/engine/Source/Scene/GltfPipeline/parseGlb.js

packages/engine/Source/Scene/GltfPipeline/updateVersion.js

It seems like only the following errors remain - renders a point - renders pnts with point size style

dave-b-b · 2024-10-20T22:17:59Z

@jjspace Everything seems fine except two tests that I can't get to pass.

renders a point
renders pnts with point size style

dave-b-b · 2024-10-21T21:53:40Z

@jjspace Not sure why this test fails in the pipeline, but it passes when I run it on my computer....

jjspace · 2024-10-21T23:16:18Z

@dave-b-b that statistics test failure is a known test failure we've been fighting in CI for a little bit. Don't worry about it #11958

- I started over because I could not get the tests to pass - removed defaultValue() in 'easy' to spot references - fetched most up to date copy of cesium, so that my pull request wasn't cluttered

- I've finished making all the changes, but a few tests are failing:

- change CesiumMath.toRadians((tilt, 0.0)) to CesiumMath.toRadians(tilt ?? 0.0) - put the parenthesis in the right place to fix the logic in FramebuggerManager lines 143 - 151

It seems like only the following errors remain - renders a point - renders pnts with point size style

- updated CHANGES.md - updated CONTRIBUTORS.md - removed defaultValue() rom FramebufferManager.js - commented out tests for defaultValueSpec.js

dave-b-b · 2024-10-23T01:53:23Z

@jjspace I think this is done now. I add something to the change log under 2024-12-01

dave-b-b · 2024-10-24T02:02:37Z

@jjspace Should I update the branch? I don't know what I should do. We already discussed the one test that is failing.

jjspace

@dave-b-b this is looking good. All the tests are passing now and I didn't spot any other weird instances of missing parens or formatting issues. Just had a couple small comments.

It looks like there might have been a bad merge somewhere. It's considering all the #11953 changes and one or two other PR's changes as being part of this PR. Could you take a look at the git history and see what might've happened.

On top of that since this is such a wide reaching change we'll probably want to tag a commit before and after this change to make it easier to merge into other PRs. Similar to what we did for the recent prettier changes. I can help create the tags but we should figure out the other git issue first

jjspace · 2024-10-24T21:18:02Z

packages/engine/Specs/Core/defaultValueSpec.js

-    expect(defaultValue(1, 5)).toEqual(1);
-  });
-});
+// import { defaultValue } from "../../index.js";


This can probably be deleted completely right?

Yeah This can be deleted.

jjspace · 2024-10-24T21:25:37Z

CHANGES.md

+##### Deprecated :hourglass_flowing_sand:
+
+- defaultValue() has been deprecated. All uses have been changed to the nullish coalescing operator (??)


This can go under the 1.123 release since that will be the next one (and I expect we should be able to get this in by then)

@jjspace Is the play to stick with the nullish coalescing operator, or deprecating defaultValue and create DefaultValue? Based on #12238, I understood it is the latter. If that is the case, would it make more sense to first create DefaultValue and then replace defaultValue with DefaultValue in the codebase? I saw Dave mention he may need to redo the majority of the PR, so it may be easier to directly replace defaultValue with DefaultValue. Rather than replace defaultValue with ?? to then replace it with DefaultValue. If I misunderstood anything, could you provide some guidance?

dave-b-b · 2024-10-25T01:18:03Z

@jjspace I didn't realize that rebasing would bring in changes from other pull requests. The only way for me to undo this would be to redo the vast majority of the pull request. Can you help me figure this out?

remove defaultValue()

8aed02e

- I started over because I could not get the tests to pass - removed defaultValue() in 'easy' to spot references - fetched most up to date copy of cesium, so that my pull request wasn't cluttered

second pull request to change to ??

8c5b622

jjspace self-assigned this Oct 14, 2024

finished

df89fc0

- I've finished making all the changes, but a few tests are failing:

fixed some mistakes

aa46629

- change CesiumMath.toRadians((tilt, 0.0)) to CesiumMath.toRadians(tilt ?? 0.0) - put the parenthesis in the right place to fix the logic in FramebuggerManager lines 143 - 151

jjspace reviewed Oct 16, 2024

View reviewed changes

jjspace mentioned this pull request Oct 16, 2024

Replace defaultValue with ?? #12207

Closed

6 tasks

fixed dangling semicolons

f8ecf1a

It seems like only the following errors remain - renders a point - renders pnts with point size style

dave-b-b changed the title ~~remove defaultValue()~~ remove defaultValue() and create DefaultValue namespace Oct 20, 2024

dave-b-b added 7 commits October 22, 2024 21:13

remove defaultValue()

aad0cc6

- I started over because I could not get the tests to pass - removed defaultValue() in 'easy' to spot references - fetched most up to date copy of cesium, so that my pull request wasn't cluttered

second pull request to change to ??

88dd27b

finished

294c5b5

- I've finished making all the changes, but a few tests are failing:

fixed some mistakes

0513e41

- change CesiumMath.toRadians((tilt, 0.0)) to CesiumMath.toRadians(tilt ?? 0.0) - put the parenthesis in the right place to fix the logic in FramebuggerManager lines 143 - 151

fixed dangling semicolons

f8df16b

It seems like only the following errors remain - renders a point - renders pnts with point size style

last minute changes

6280bd5

- updated CHANGES.md - updated CONTRIBUTORS.md - removed defaultValue() rom FramebufferManager.js - commented out tests for defaultValueSpec.js

fixed merge issue

694c6bc

jjspace reviewed Oct 24, 2024

View reviewed changes

dave-b-b added 2 commits November 3, 2024 16:54

Merge branch 'main' into fix-issue-11674

3ee6d36

Merge branch 'main' into fix-issue-11674

97820f5

Merge branch 'main' into fix-issue-11674

d271bf2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove defaultValue() and create DefaultValue namespace #12252

remove defaultValue() and create DefaultValue namespace #12252

dave-b-b commented Oct 13, 2024 •

edited

Loading

github-actions bot commented Oct 13, 2024

dave-b-b commented Oct 13, 2024

jjspace commented Oct 14, 2024

dave-b-b commented Oct 15, 2024

dave-b-b commented Oct 15, 2024

jjspace left a comment

jjspace Oct 15, 2024

jjspace Oct 15, 2024

dave-b-b commented Oct 20, 2024

dave-b-b commented Oct 21, 2024

jjspace commented Oct 21, 2024

dave-b-b commented Oct 23, 2024

dave-b-b commented Oct 24, 2024

jjspace left a comment

jjspace Oct 24, 2024

dave-b-b Oct 25, 2024

jjspace Oct 24, 2024

anpaulan Oct 30, 2024

dave-b-b commented Oct 25, 2024

	tilt = CesiumMath.toRadians((tilt, 0.0));
	tilt = CesiumMath.toRadians(tilt ?? 0.0);

		##### Deprecated :hourglass_flowing_sand:

		- defaultValue() has been deprecated. All uses have been changed to the nullish coalescing operator (??)

remove defaultValue() and create DefaultValue namespace #12252

Are you sure you want to change the base?

remove defaultValue() and create DefaultValue namespace #12252

Conversation

dave-b-b commented Oct 13, 2024 • edited Loading

Description

Issue number and link

Testing plan

Author checklist

github-actions bot commented Oct 13, 2024

dave-b-b commented Oct 13, 2024

jjspace commented Oct 14, 2024

dave-b-b commented Oct 15, 2024

dave-b-b commented Oct 15, 2024

jjspace left a comment

Choose a reason for hiding this comment

jjspace Oct 15, 2024

Choose a reason for hiding this comment

jjspace Oct 15, 2024

Choose a reason for hiding this comment

dave-b-b commented Oct 20, 2024

dave-b-b commented Oct 21, 2024

jjspace commented Oct 21, 2024

dave-b-b commented Oct 23, 2024

dave-b-b commented Oct 24, 2024

jjspace left a comment

Choose a reason for hiding this comment

jjspace Oct 24, 2024

Choose a reason for hiding this comment

dave-b-b Oct 25, 2024

Choose a reason for hiding this comment

jjspace Oct 24, 2024

Choose a reason for hiding this comment

anpaulan Oct 30, 2024

Choose a reason for hiding this comment

dave-b-b commented Oct 25, 2024

dave-b-b commented Oct 13, 2024 •

edited

Loading