cesium-unreal icon indicating copy to clipboard operation
cesium-unreal copied to clipboard

Expose CesiumGltfPrimitiveComponent.h to public

Open mmoedinhas opened this issue 3 years ago • 11 comments

I'm opening this pull request to ask for this component to be exposed

mmoedinhas avatar Jan 05 '22 17:01 mmoedinhas

Thanks for the pull request @mmoedinhas!

  • :x: Missing CLA.
  • :grey_question: CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

cesium-concierge avatar Jan 05 '22 17:01 cesium-concierge

I've sent the CLA

mmoedinhas avatar Jan 05 '22 17:01 mmoedinhas

Thanks @mmoedinhas for opening this pull request.

Can you please share how this component being made public helps and what goals it will accomplish that are currently not possible? I wonder if those can be accomplished by exposing the right APIs through the glTF classes.

shehzan10 avatar Jan 05 '22 18:01 shehzan10

Thank you for the quick response!

In our project, we're raycasting the tileset to check whether the tile under the camera contains water or terrain. The raycast hits this component in the tileset actor, which has a CesiumGltf::Model in it with the information that we want. Is there any other way to approach this problem? Before trying the raycast we were doing a BFS through all the Tiles for the one nearest the camera. That works, but sometimes it has buggy results. We were trying a more straighforward approach to this problem by using a raycast, but were blocked by the component being private.

mmoedinhas avatar Jan 05 '22 19:01 mmoedinhas

Thanks @mmoedinhas for elaborating. I think @kring is best positioned to either accept this or suggest alternatives. Kevin is currently on leave and will be back next week.

If this PR represents any urgency on your end, please feel free to chime in here.

shehzan10 avatar Jan 10 '22 17:01 shehzan10

Thanks for the pull request @mmoedinhas! Your use-case makes sense, but I'm a little wary of exposing UCesiumGltfPrimitiveComponent, because it's an implementation detail that is fairly likely to change in the future.

There's already very similar (but not quite adequate) functionality in CesiumMetadataUtilityBlueprintLibrary. The GetPrimitiveMetadata function gets the set of feature metadata corresponding to a particular UPrimitiveComponent (e.g. that you get back from a line trace). That function returns FCesiumMetadataPrimitive, which is constructed with the glTF model and primitive; exactly what you need! But they're not exposed publicly. So I think a reasonable approach here is to change that: make it possible to obtain the model and primitive from a FCesiumMetadataPrimitive instance. That should solve your use-case without exposing implementation details.

If you're up for implementing that, great! If not, feel free to keep using the change here as a workaround and we'll write an issue to implement something like this ourselves (but I can't promise when).

Thanks again!

kring avatar Jan 14 '22 06:01 kring

Thank you for your answers.

I followed your advice and ended up using FCesiumMetadataPrimitive, with a few modifications that I pushed here in case you would like to merge them.

mmoedinhas avatar Jan 17 '22 15:01 mmoedinhas

I've applied your code review suggested changes.

Unfortunately this is not working 100% for our use case and I can't figure out why. I thought this was working at first, but now I notice that the metadata that is returned with GetPrimitiveMetadata is always empty... Which is weird since the pModel and pMeshPrimitive have values. I've attached a screenshot that shows this. image

I'm thinking of doing a workaround, which is adding a GetPrimitiveModel to UCesiumMetadataUtilityBlueprintLibrary and stop using the FCesiumMetadataPrimitive at all. That way I'll get the model "directly". I'll push the change here too, but if you don't want it, just say and I'll revert it.

Thank you again for your time

mmoedinhas avatar Jan 18 '22 12:01 mmoedinhas

I just noticed that CesiumGltf::Model is not exposed to blueprints, which makes my suggested workaround of creating a GetPrimitiveModel in UCesiumMetadataUtilityBlueprintLibrary not make much sense, as the function wouldn't be able to be called from blueprints. Do you think there are other alternatives?

mmoedinhas avatar Jan 18 '22 12:01 mmoedinhas

Thanks again for your contribution @mmoedinhas!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

cesium-concierge avatar Feb 18 '22 03:02 cesium-concierge

Thanks again for your contribution @mmoedinhas!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

cesium-concierge avatar Mar 20 '22 02:03 cesium-concierge