Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose CesiumGltfPrimitiveComponent.h to public #715

Open
wants to merge 6 commits into
base: ue4-main
Choose a base branch
from

Conversation

mmoedinhas
Copy link

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

@cesium-concierge
Copy link

Thanks for the pull request @mmoedinhas!

  • ❌ Missing CLA.
  • 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:

@mmoedinhas
Copy link
Author

I've sent the CLA

@shehzan10
Copy link
Member

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.

@mmoedinhas
Copy link
Author

mmoedinhas commented Jan 5, 2022

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.

@shehzan10
Copy link
Member

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.

@kring
Copy link
Member

kring commented Jan 14, 2022

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!

@mmoedinhas
Copy link
Author

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.

Copy link
Member

@kring kring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the changes @mmoedinhas! Looks pretty good, just a couple of suggested changes.

@@ -12,6 +12,9 @@ FCesiumMetadataPrimitive::FCesiumMetadataPrimitive(
const CesiumGltf::ExtensionMeshPrimitiveExtFeatureMetadata&
primitiveMetadata) {

_model = model;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be set using a C++ initializer list, i.e.:

FCesiumMetadataPrimitive::FCesiumMetadataPrimitive(
    const CesiumGltf::Model& model,
    const CesiumGltf::MeshPrimitive& primitive,
    const CesiumGltf::ExtensionModelExtFeatureMetadata& metadata,
    const CesiumGltf::ExtensionMeshPrimitiveExtFeatureMetadata&
        primitiveMetadata) : _model(model), _meshPrimitive(primitive) {

private:
TArray<FCesiumMetadataFeatureTable> _featureTables;
VertexIDAccessorType _vertexIDAccessor;

CesiumGltf::Model _model;
CesiumGltf::MeshPrimitive _meshPrimitive;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holding the CesiumGltf::Model by value means that the entire model - including all its data buffers - will be copied into the FCesiumMetdataPrimitive at construction, which is going to be really expensive. A better alternative is to hold (const) pointers to these objects.

* Use C++ initializer list
* Hold a const pointer instead of a copied value
@mmoedinhas
Copy link
Author

mmoedinhas commented Jan 18, 2022

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
Copy link
Author

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?

@cesium-concierge
Copy link

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.

1 similar comment
@cesium-concierge
Copy link

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.

@kring kring deleted the branch CesiumGS:ue4-main April 1, 2022 02:48
@kring kring closed this Apr 1, 2022
@kring kring reopened this Apr 1, 2022
@kring kring changed the base branch from main to ue4-main April 1, 2022 03:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants