Instance feature rendering #1565
Conversation
Add feature ids from feature sets as PerInstanceSMCustomData to instanced static meshes. Construct the material graph that exposes these feature IDs and does a lookup of property values, as for non-instanced metadata. This isn't working yet; it appears that some flag is not set in the overall Cesium material.
This didn't make any difference to displaying materials on instances, but it's probably a good idea to do things this way.
This was caused by missing code to populate the |
There was a problem hiding this comment.
Since diffing materials isn't really a thing, can you summarize what you changed in this file?
There was a problem hiding this comment.
I added a PerInstanceCustomData node to it, not connected to anything. I've now added a comment to it. Do you think its addition needs to be mentioned anywhere else?
| @@ -2877,6 +2877,39 @@ static void SetMetadataParameterValues_DEPRECATED( | |||
| PRAGMA_ENABLE_DEPRECATION_WARNINGS | |||
| #pragma endregion | |||
|
|
|||
| namespace { | |||
| void addInstanceFeatureIds(UCesiumGltfInstancedComponent* pInstancedComponent) { | |||
There was a problem hiding this comment.
It looks like this function will add at least a little memory usage for every instance, right? And it's only needed if we're actually styling based on instance feature IDs? Can we avoid adding it when it's not needed?
There was a problem hiding this comment.
It adds storage for the feature IDs if there are any, otherwise I don't think there's any additional memory usage. Isn't this the same situation with regular feature IDs stored in textures?
There was a problem hiding this comment.
No, I think this is a lot different from the feature ID texture case. With attribute and texture feature IDs, space to hold a set of feature IDs is only allocated if that feature ID set is used by a the CesiumFeaturesMetadata component. Step through encodePrimitiveFeaturesAnyThreadPart in CesiumEncodedFeaturesMetadata.cpp in the Metadata level in the Samples, and you'll see that encodeFeatureIdTexture is called. However, if you delete the CesiumFeaturesMetadata component from the "San Francisco Ferry Building" tileset, or if you remove the feature IDs from it, and then refresh the tileset, you'll see that that function is no longer called. The texture is only created if it's needed.
For instances, though, it appears that the "custom data" for the instance IDs is added, even if the tileset doesn't even have a CesiumFeaturesMetadata component. This means that all tilesets with instance IDs will use more memory than necessary (either on the CPU or GPU or both, I'm not sure where custom data values are stored) when those instance IDs aren't used at all.
There was a problem hiding this comment.
No, I think this is a lot different from the feature ID texture case. With attribute and texture feature IDs, space to hold a set of feature IDs is only allocated if that feature ID set is used by a the CesiumFeaturesMetadata component. Step through
encodePrimitiveFeaturesAnyThreadPartinCesiumEncodedFeaturesMetadata.cppin the Metadata level in the Samples, and you'll see thatencodeFeatureIdTextureis called. However, if you delete the CesiumFeaturesMetadata component from the "San Francisco Ferry Building" tileset, or if you remove the feature IDs from it, and then refresh the tileset, you'll see that that function is no longer called. The texture is only created if it's needed.For instances, though, it appears that the "custom data" for the instance IDs is added, even if the tileset doesn't even have a CesiumFeaturesMetadata component. This means that all tilesets with instance IDs will use more memory than necessary (either on the CPU or GPU or both, I'm not sure where custom data values are stored) when those instance IDs aren't used at all.
I understand now what you're saying; I wasn't thinking of the possibility of not loading model data that's part of the glTF. If I understand correctly, there's a big difference between the feature ID approach with normal models vs GPU instances: in the former, the feature IDs are stored in textures that are part of a material created during the encoding procedure, whereas instance feature IDs are stored in PerInstanceSMCustomData, which is like a vertex attribute with a per-instance value. If the instance feature IDs are not stored when the static mesh is first created, then they would need to be modified during encoding...
The extra memory used is essentially one float per instance -- more accurately one float per feature ID set, though it seems rare to have more than one -- or about 10%. On the GPU. Is this a blocker for you?
There was a problem hiding this comment.
If the instance feature IDs are not stored when the static mesh is first created, then they would need to be modified during encoding...
Adding or modifying CesiumEncodedFeaturesMetadata will refresh the tileset. So this shouldn't really be an issue. The code doesn't know how to add a feature ID texture after the tile is created either, but it doesn't matter.
The extra memory used is essentially one float per instance -- more accurately one float per feature ID set, though it seems rare to have more than one -- or about 10%.
I think it's well worth a bit of effort to reduce memory usage by 10%. Obviously there's a level of effort where it wouldn't be worth it, but this sounds like it should be pretty straightforward (unless there's some subtlety to it that isn't obvious).
There was a problem hiding this comment.
I went ahead and added the logic to avoid populating the custom data for instance feature ID sets that aren't used in d63f692. I appreciate your review on it when you get a chance, but I'm going to go ahead and merge this so that we can ship it on January 2!
M_CesiumBaseMaterial now has a vestigial PerInstanceCustomData node whose sole function is to force some magic bit to be set, thus allowing a shader that uses instance data to be instantiated.
…instance-feature-rendering
| #if ENGINE_VERSION_5_3_OR_HIGHER | ||
| pInstancedComponent->SetNumCustomDataFloats(featureSetCount); | ||
| #else | ||
| pInstancedComponent->NumCustomDataFloats = featureSetCount; | ||
| #endif |
There was a problem hiding this comment.
Looks like the M_CesiumBaseMaterial has been saved with UE 5.3 in this branch. That's fine, because our next release will be 5.3+ only. But given that that is the case, this #if is unnecessary.
| #if ENGINE_VERSION_5_3_OR_HIGHER | |
| pInstancedComponent->SetNumCustomDataFloats(featureSetCount); | |
| #else | |
| pInstancedComponent->NumCustomDataFloats = featureSetCount; | |
| #endif | |
| pInstancedComponent->SetNumCustomDataFloats(featureSetCount); |
|
@timoore this looks great, just that one concern about unnecessary memory usage. I was able to make a simple material that makes features red if their
|
|
Thanks @kring, I will review this today. |
|
oh ok, thanks Tim! I was just going to merge as soon as CI passes, but if you have a chance to review today that's even better. Definitely no need to interrupt your holiday for it, though! |
|
@kring this looks fine to me. |
This depends on #1537, so merge it first.
Add functionality to discover feature IDs in instances with the EXT_instance_features extension in a tileset, in the same way that we do for non-instanced feature IDs. Autogenerate a material which the user can edit to render instance properties.
The autogenerated material does not have a connection between the CesiumGetFeatureIdsFromInstance material function and the property accessor node, and I'm not sure why...