Entity Shadows

[lilleyse]

For #2594
Continuation of #3928 now that the shadows branch is gone.

I added castShadows and receiveShadows properties to all the applicable entity types. It supports changing these properties at runtime. Batching based on shadows is handled in GeometryVisualizer. Each batch, besides the dynamic batch, is now an array of 4 batches (cast+receive, cast, receive, none). This was the cleanest solution for now, but could get unwieldy if we continue to add more bucket types. I tried and gave up on a unified solution for batching so that we wouldn't need StaticOutlineGeometryBatch, StaticGeometryColorBatch, and in StaticGeometryPerMaterialBatch. Maybe this is worth revisiting in a future rewrite.

We talked about potentially using just shadows instead of castShadows and receiveShadows.

• Deprecate castShadows and receiveShadows
• Performance testing

[lilleyse]

Entities now have a single shadows property. I am seeing some problems with the checkered entity in the Sandcastle demo, but otherwise this is ready for review.

[pjcozzi]

We talked about potentially using just shadows instead of castShadows and receiveShadows.

I am all for less code in the higher-level layers, but are we sure about this? If so, why didn't we just do this in the Primitive API?

Separate cast/receive are standard and support all use cases.

The other option is to make this an enum.

[mramato]

Yeah, we talked offline and Dan mentioned some use cases where you would want both. An enum definitely sounds like the right call (perhaps that should have been done at the Primitive layer as well?)

[pjcozzi]

perhaps that should have been done at the Primitive layer as well?

It will be easy to deprecate if we go this route.

[shunter]

Given the uncertainty about the properties in this PR I am going to omit the CZML model shadow property merged with #3856 until we have an API that won't change.

[lilleyse]

An enum doesn't really seem intuitive from the user's perspective.

I'm ok with reverting back to castShadows and receiveShadows.

[mramato]

An enum doesn't really seem intuitive from the user's perspective.

Why not, it makes the most sense to me and is what we do elsewhere in Cesium. How is this different?

[lilleyse]

It just seems overly complicated for the entity api, remembering the enum names is harder than just setting some booleans.

[mramato]

They would not be names, we would have a new enum type. We already do this everywhere else in the Entity API (HorizontalOrigin, VerticalOrigin, etc..) no one has ever complained.

[pjcozzi]

I am fine with the enum or without it I suppose; it just seems like a lot more plumbing without it.

I agree the enum names are a bit awkward, but could be simple enough:

ENABLED
DISABLED // same as being undefined
CAST_ONLY
RECEIVE_ONLY

@lilleyse perhaps see what is most common in game engines.

[lilleyse]

Ok, I'll go with the enum approach. Hopefully today or tomorrow I'll have time to get this ready before the release.

[pjcozzi]

@lilleyse I see you pushed three new commits. Is this ready or should it wait until 1.24?

[lilleyse]

This is ready for review at least. I'm still seeing issues with the checkered box though which I hope to fix soon.

[pjcozzi]

Let's finish this for 1.24 since 1.23 is on Friday.

[lilleyse]

This is ready for review, though the Sandcastle demo may crash in certain cases until #4099 is merged.

Given the uncertainty about the properties in this PR I am going to omit the CZML model shadow property merged with #3856 until we have an API that won't change.

@shunter Are there any additional czml changes that I need to make besides what's already here?

[mramato]

Can we generate some performance numbers that compares both load time of vienna.geojson and memory usage after load with master"? It doesn't have to be super rigorous, but the Entity visualization architecture is already long overdue for an overhaul and I just want to make sure we didn't make things substantially worse here.

[lilleyse]

Sure I can do that.

[pjcozzi]

What is the plan here? Will this make 1.24 on Monday?

[lilleyse]

Would it be possible to squeeze this into the release, or should we just wait for the next? The last major change is to add back the castShadows and releaseShadows for certain types just for deprecation purposes.

[pjcozzi]

You would have to code this now, right? If so, let's not hold up the release (but go ahead and still make the change now so we can get it into master early for the 1.25 release).

[lilleyse]

Ok

[lilleyse]

@mramato where can I find vienna.geojson?

[mramato]

vienna.geojson.txt

[lilleyse]

In terms of performance:

• The load time is roughly the same, about 7 seconds each (I didn't get super accurate, just used a stopwatch).
• The memory difference is around 12MB (the Chrome process used 742MB then 754MB). This is hard to get an accurate reading for by the time you change branches, rebuild, etc, but 12MB was the biggest difference I saw.

[lilleyse]

I added back the deprecated properties and updated CHANGES.md. This is ready for a look now.

[pjcozzi]

@mramato please merge when ready.

[mramato]

Merge in master and this is ready. Thanks @lilleyse!

[mramato]

In terms of performance:

Thanks, I think if there were any major issues, they would have been obvious.

[lilleyse]

Did anything change lately with trackedEntity? After merging the view starts out pretty far away in the Shadows demo:

[lilleyse]

This is also noticeable in the 3D Models demo. @tfili the commit is 934828e when something changed.

The correct start view changed to the one below:

[mramato]

No that I know of, but I can take a quick look. Does this happen in master?

[lilleyse]

Yeah it's also in master.

[mramato]

Yeah it's also in master.

OK, then I'll merge this and we can look at this later. Can you write up a separate issue so that we don't forget? Thanks!

[lilleyse]

OK, then I'll merge this and we can look at this later. Can you write up a separate issue so that we don't forget? Thanks!

Sure