fixes issue #12356 performance drop due to EnvironmentMap

[jfayot]

Description

This is a proposal to fix / workaround performance drop due to EnvironmentMap.

Issue number and link

Fixes #12356

Testing plan

• Updated Cesium3DTilesetVisualizerSpec and ModelVizualizerSpec
• Run following Sandcastle.

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

[github-actions]

Thank you for the pull request, @jfayot!

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

[ggetz]

Thanks for the PR @jfayot! Instead of a specific enableEnvironmentMap property, what do you think of allowing the constructor options, similar to what we do for Model.js?

Also, given that entities' positions are usually more dynamic than static models or tilesets, I'm thinking for entities specifically, we make the threshold for updating environment maps much higher or disable them entirely. That way, the environment map generation only runs once on load.

[jfayot]

Thanks for the review, @ggetz !

what do you think of allowing the constructor options, similar to what we do for Model.js?

As far as you consider DynamicEnvironmentMapManager as a high-level interface, yes, this can be added to Entity's constructor options. I didn't go that way because, in my understanding, it was rather a low-level interface associated with primitives.

for entities specifically, we make the threshold for updating environment maps much higher

I think that finding a threshold that fits for all use cases will be difficult ! It's probably better to disable the environment map by default for entities and let the user decide the appropriate options.

[jfayot]

@ggetz I've added environmentMapOptions to Entity constructor. Tell me if this is what you had in mind...

[jfayot]

Hi @ggetz , I hope you're doing well !
Do you think there's a chance to get this PR in 1.125 ?
I haven't seen much activity on the repo since 2 weeks. Should I get worried 😬 ...

[ggetz]

Hi @jfayot! Apologies on the delayed responses– Many have been out for the holidays.

Work done in #12370 appears to address the performance issue demonstrated in #12356. Can you confirm?

However, the additional failsafe in this PR is also beneficial IMO. We'll take a pass on this and hopefully get it into today's release. Thank you!

[jfayot]

Hi @ggetz ! Happy new year to you and the Cesium team !

Yes I can confirm that your PR #12370 fixes the issue: back to 60fps when moving entities more than 1000m.

[ggetz]

I think that finding a threshold that fits for all use cases will be difficult ! It's probably better to disable the environment map by default for entities and let the user decide the appropriate options.

I was thinking we don't entirely disable the environment map, but set the update timing to Infinity so that it's only generated once. If you agree, I can push the update to this branch.

[jfayot]

I think that finding a threshold that fits for all use cases will be difficult ! It's probably better to disable the environment map by default for entities and let the user decide the appropriate options.

I was thinking we don't entirely disable the environment map, but set the update timing to Infinity so that it's only generated once. If you agree, I can push the update to this branch.

Sure, that's fine for me!

[ggetz]

@jfayot If these updates work for you, I think this should be ready to go!

[jfayot]

@jfayot If these updates work for you, I think this should be ready to go!

It works for me @ggetz ! Thanks for the update.

[ggetz]

Thank you @jfayot!