Instance feature metadata #1537
Conversation
There was a problem hiding this comment.
@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:
- By face -> vertex -> feature ID attribute lookup
- By texture coordinate -> feature ID texture lookup
- 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.
| const TSharedPtr<FCesiumInstanceFeatures> pInstanceFeatures = | ||
| pInstancedComponent->pInstanceFeatures; | ||
| if (!pInstanceFeatures) { | ||
| return TMap<FString, FCesiumMetadataValue>(); | ||
| } |
There was a problem hiding this comment.
This check looks unnecessary. And also redundant because GetInstanceFeatures does the same thing.
There was a problem hiding this comment.
My thinking was that this would be a cheap test-and-out if the hit result was not on an instance.
|
I haven't looked deeply at this PR, but with the way that I also don't see something that disallows I feel like it makes more sense if we use "Instance ID" for |
|
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" ? |
|
@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. |
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 However, there is also the possibility for multiple feature ID sets in 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
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 |
|
Ok, I see. That's a good point. We actually already have this problem, though, as it's currently implemented. I think we either need to (1) accept this, and apply it everywhere, meaning that there's just one 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 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. |
|
@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. |
|
It turns out that |
There was a problem hiding this comment.
@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.
| if (!IsValid(pInstancedComponent)) { | ||
| return std::nullopt; | ||
| } | ||
| const TSharedPtr<FCesiumInstanceFeatures> pInstanceFeatures = | ||
| pInstancedComponent->pInstanceFeatures; | ||
| if (!pInstanceFeatures) { | ||
| return TMap<FString, FCesiumMetadataValue>(); | ||
| } |
There was a problem hiding this comment.
See @kring 's comment of course, but if we're keeping these lines then these two if statements need to return the same thing. The point is that getting the instance metadata values fails because ultimately the component doesn't contain (proper) instance features. So the behavior should be consistent either way.
There was a problem hiding this comment.
This was not clear and at least deserved a comment. The intent was to return std::nullopt if the primitive was not an instanced static mesh and therefore cause GetPropertyTableValuesFromHit() to use feature IDs from a primitive attribute. I was thinking that if the primitive was instanced and didn't have instance feature IDs, then it couldn't have feature IDs at all. But given our discussion above, that's not clearly the case.
I thought this was the source of a bug. Not sure about this now, but it seems good to be explicit about what we mean for the move.
This is needed because UCesiumFeatureIdSetBlueprintLibrary::GetAsFeatureIDAttribute() returns an error for instance attributes. I guess that's reasonable behavior.
Build the functionality into the more general version that works on all primitive types.
timoore
force-pushed
the
instance-feature-metadata
branch
from
fd4e0f9
to
4f5e3ea
Compare
Changed the function and its helper getInstancePropertyTableValues() to have less duplicated code. Also, it is now possible for instances to fall back to feature IDs that might be stored in the primitives of an instance.
Support for accessing metadata in GPU instances in picking hit results.