-
Notifications
You must be signed in to change notification settings - Fork 296
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
base: ue4-main
Are you sure you want to change the base?
Expose CesiumGltfPrimitiveComponent.h to public #715
Conversation
Thanks for the pull request @mmoedinhas!
Reviewers, don't forget to make sure that:
|
I've sent the CLA |
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. |
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? |
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. |
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 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! |
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. |
There was a problem hiding this 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; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
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. |
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 |
1 similar comment
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 |
I'm opening this pull request to ask for this component to be exposed