Instance feature metadata

[timoore]

Support for accessing metadata in GPU instances in picking hit results.

[kring]

@timoore I tried this out with the instanced version of everybody's favorite model, and it works well. In fact, the existing Metadata level in the Samples can now query instanced models without any changes, which is nice!

However, I also found that that commonality with non-instanced metadata pretty much ends there. As soon as you want to use any of the lower-level, more powerful metadata access Blueprint APIs, we're stick with completely different paths for instanced and non-instanced models.

Now there are certainly details here that I don't understand, but I'm reasonably sure that this doesn't need to be so. For example, Get Feature ID From Hit looks to me like a concept that works (and is identical) across all types of models. So why do we now have two functions to do this? (confusingly, they have the same name, too)

Well, the immediate reason is that one needs "Primitive Features" and the other needs "Instance Features". But why are those separate? Both are just containers for sets of feature IDs that are resolved in different ways:

1. By face -> vertex -> feature ID attribute lookup
2. By texture coordinate -> feature ID texture lookup
3. And now! By instance ID -> feature ID lookup

In fact, it seems reasonable that a single "hit" could return feature IDs from all three. There could be metadata attached to the instance, the vertex, and the texture.

What do you think, is that achievable?

So that's my main feedback on this PR. I also have some additional small comments below, based on only a partial review of the code so far.

[kring]

This check looks unnecessary. And also redundant because GetInstanceFeatures does the same thing.

[timoore]

My thinking was that this would be a cheap test-and-out if the hit result was not on an instance.

[j9liu]

I haven't looked deeply at this PR, but with the way that EXT_instance_features exists on a node versus EXT_mesh_features existing on a primitive, I feel like the separation between the two makes sense.

I also don't see something that disallows EXT_instance_features from instancing a mesh with EXT_mesh_features. I feel like you would need to differentiate between the two. Maybe you have two separate building instances, and you'll want their different instance IDs. But what if you want to get the feature ID of the window within an instance? Then it becomes confusing whether the returned feature ID is from EXT_instance_features or EXT_mesh_features

I feel like it makes more sense if we use "Instance ID" for EXT_instance_features, if that's acceptable terminology. Or if not "Instance ID", then very carefully differentiating the feature IDs of the instances versus the feature IDs in the mesh. Using "Instance ID" as shorthand makes most sense for me, and you could rename the Get Feature ID From Hit with CesiumInstanceFeatures to Get Instance ID From Hit. Does that seem reasonable?

[j9liu]

Hm now that I think about it, "Instance ID" is not an accurate descriptor / is confusing for cases where you're assigning multiple instances the same feature ID. So maybe we have to use terminology like "Instance Feature ID" vs. "Mesh Feature ID" ?

[kring]

@j9liu I'm not as well versed in the metadata system as you are, so I very well could be missing something. But I don't understand why an "Instance Feature ID" is fundamentally different from our two other feature IDs we already have: the vertex attribute feature ID, and the texture feature ID. I guess they come from different extensions, but I don't think users know or care about that. They just want to be able to say: what are the metadata properties associated with the thing I just clicked on? And right now, if you want typed metadata (not just strings), that requires coding up (in Blueprint, say) two completely different paths to access the two types of feature IDs.

Obviously, if that's unavoidable, then we'll just have to live with it. But it doesn't look unavoidable. As I understand it, a feature ID - no matter which of the three sources provided it - is fundamentally just an index into a property table.

[j9liu]

But I don't understand why an "Instance Feature ID" is fundamentally different from our two other feature IDs we already have: the vertex attribute feature ID, and the texture feature ID. I guess they come from different extensions, but I don't think users know or care about that. They just want to be able to say: what are the metadata properties associated with the thing I just clicked on?

As I understand it, a feature ID - no matter which of the three sources provided it - is fundamentally just an index into a property table.

I agree that functionality wise, they're pretty much the same, and distinctions between them are going to be confusing. And I'm not sure if EXT_instance_features + EXT_mesh_features is going to come up frequently enough to be of concern. (@lilleyse do you have any opinion?) I agree with trying to make it as easy as possible to get metadata from a tileset.

However, there is also the possibility for multiple feature ID sets in EXT_mesh_features alone. And, there is the possibility for EXT_instance_features to have multiple feature ID sets. You can see the example here: https://github.com/CesiumGS/glTF/tree/3d-tiles-next/extensions/2.0/Vendor/EXT_instance_features#feature-id-by-gpu-instance These multiple feature ID sets are stored in a featureIds array on each extension respectively.

Since there are multiple assignments of feature ID sets, we'd want to have some way for the user to query the one they actually want. So if my memory serves me right, the Get Feature ID From Hit function has a Set Index parameter that indexes into the array in EXT_mesh_features. That's how you could explicitly pick from the feature ID sets to query. But if we were to combine Blueprints for EXT_instance_features and EXT_mesh_features, that index might be ambiguous. Is that a fair thing to do?

Obviously, if that's unavoidable, then we'll just have to live with it. But it doesn't look unavoidable.

It's not unavoidable, but unfortunately there is a level of granularity to these extensions, and abstracting that away can lead to ambiguous behavior. So we just have to be extra careful about documenting the assumptions we make

[lilleyse]

I agree that functionality wise, they're pretty much the same, and distinctions between them are going to be confusing. And I'm not sure if EXT_instance_features + EXT_mesh_features is going to come up frequently enough to be of concern. (@lilleyse do you have any opinion?) I agree with trying to make it as easy as possible to get metadata from a tileset.

EXT_instance_features + EXT_mesh_features is unlikely to show up much in practice. I think it would be fine to assume that EXT_instance_features takes precedence over EXT_mesh_features if they both exist.

[kring]

But if we were to combine Blueprints for EXT_instance_features and EXT_mesh_features, that index might be ambiguous.

Ok, I see. That's a good point. We actually already have this problem, though, as it's currently implemented. UCesiumMetadataPickingBlueprintLibrary::GetPropertyTableValuesFromHit takes a FeatureIDSetIndex. If there's instance metadata, that index is an index into EXT_instance_features. If there's not, then it's an index into EXT_mesh_features.

I think we either need to (1) accept this, and apply it everywhere, meaning that there's just one Get Feature ID From Hit and it resolves the index in the same way.

OR (2) we need to declare this unacceptable and have a separate "namespace" for these two sets of feature ID indices. And if we go this way, I think that Get Feature ID From Hit should take an enumeration specifying which set of feature IDs to prefer when both exist, rather than have two entirely separate (but confusingly similar-looking in Blueprint) versions of this function.

It's also possible to start with (1) today, and add (2) in a later version of Cesium for Unreal. But currently this branch has the two separate functions, and I don't think we should ship it that way.

Another approach (which doesn't have to replace any of the options already mentioned) is to make it easy to enumerate the feature ID sets, wherever they come from, and then query by reference to one. In other words, avoid the need for feature ID set indices completely.

[j9liu]

@kring Fair enough, I didn't notice that -- I've been arguing at a high level and haven't looked at the code 😅

I think (1) makes sense today. (2) sounds nice for future proofing, and so does the last approach! But I agree that we should just do 1 now and defer the others for later.

[timoore]

It turns out that UCesiumFeatureIdSetBlueprintLibrary::GetFeatureIDFromHit is already generic for instance features. The instance-only version was an earlier effort that I should have removed. @kring, are there other instance-only blueprint functions that you think should be combined with the existing feature functions to be fully generic?

[j9liu]

@timoore I did a code-only pass for code design, clarity, and quality. We're on Thanksgiving break this week, so if this is supposed to be in by next week's release I ask that you address feedback ASAP so this can make it in!

Also please address @kring 's comments too, I'm seeing that some of them have not been resolved. If you have any questions or want to discuss something please don't hesitate to ask.

[timoore]

I've removed the FCesiumInstanceFeatures collection type, which I think resolves the complaints about redundancy and makes this PR a lot more compact. I've left UCesiumFeatureIdSetBlueprintLibrary::GetAsFeatureIDInstanceAttribute(); I guess I don't understand how it's cousin GetAsFeatureIDAttribute() is used today in blueprint functions and whether or not it's important for that function to only return attributes from primitives.

[kring]

Thanks for making all the changes @timoore! I have a few comments below, but just minor stuff because this looks really good to me overall. It would probably be good to get @j9liu's eyes on it too, though!

[kring]

Thanks @timoore, this looks good to me! @j9liu I'll let you hit merge since you reviewed it previously and also have a deeper understanding of the metadata system than I do.

[kring]

"Hit merge" and also add more comments if necessary, of course!

[j9liu]

@kring For sure, I'll take a look later today / tomorrow!

[j9liu]

I just have a few tweaks, but I'm going to push a commit myself to speed things up.

I'm not 100% happy about CesiumPrimitiveFeatures representing both primitive and instance IDs. Not because we need redundancy with CesiumInstanceFeatures, but I think the name CesiumPrimitiveFeatures is now inaccurate.

On the one hand, the node with EXT_mesh_gpu_instancing gets converted into an instanced mesh primitive under the hood, so it is technically still a primitive. On the other hand, I named CesiumPrimitiveFeatures after the fact that the EXT_mesh_features extension is on the glTF primitive. This scheme carries over to EXT_structural_metadata, where we have CesiumModelMetadata and CesiumPrimitiveMetadata to denote the difference.

But not sure what a happy replacement would be. Maybe CesiumGltfFeatures? Not saying this needs to be resolved -- it's out of scope for this PR. Maybe I'm just abnormally nitpicky about names 🤷

I still notice something that should be addressed, but I will make my own follow-up PR to fix it.

[j9liu]

@timoore I should have pointed this out earlier, but if you could add unit tests for your changes in a follow-up PR, that would be great.

[j9liu]

Merging after the Apple builds. I haven't played around with this in action but I assume @kring tested this enough when he reviewed 😛