Skip to content

Detangle some material generation logic from CesiumFeaturesMetadataComponent #1628

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

Merged
merged 16 commits into from
Apr 29, 2025

Conversation

j9liu
Copy link
Contributor

@j9liu j9liu commented Mar 6, 2025

A lot of the boilerplate in CesiumFeaturesMetadataComponent is necessarily entangled, but there are some parts that can be moved out and made reusable. I also made some changes to file names in accordance with our style guide.

This PR does the following:

  • Create GeneratedMaterialUtility namespace
  • Move some common functions, constants, etc. into the new namespace
  • Update CesiumFeaturesMetadataComponent.cpp for the changes
  • Drops the Cesium from EncodedFeaturesMetadata, since it isn't public API
  • Drops the Cesium from EncodedMetadataConversions, since it isn't public API

@j9liu j9liu marked this pull request as draft March 6, 2025 18:53
@j9liu j9liu marked this pull request as ready for review March 6, 2025 18:56
@kring kring added this to the May 2025 Release milestone Apr 1, 2025
@j9liu
Copy link
Contributor Author

j9liu commented Apr 8, 2025

This should be ready for a review, provided CI passes this time ^_^

@j9liu
Copy link
Contributor Author

j9liu commented Apr 9, 2025

CI is now passing thanks to #1649 🙌

@kring
Copy link
Member

kring commented Apr 22, 2025

I noticed a strange problem. I'm not sure if it's new in this branch or not. This happens in main, too.

  1. Create a new empty level in the Samples project.
  2. And Cesium SunSky, Cesium World Terrain + Bing Maps Aerial, and Cesium OSM Buildings using the Cesium panel.
  3. Click the Cesium OSM Buildings Actor in the Outliner and add a CesiumFeaturesMetadata component. It will be selected automatically.
  4. Click the Auto Fill button.
  5. Open Model Metadata -> Property Tables -> Index [ 0 ] -> Properties and scroll down until you find cesium#longitude. Expand it and then expand Property Details inside it. Notice that the "Component Type" is Int 8. It should be Float64.
    image
  6. Delete all of the properties by clicking the trash can icon near Model Metadata -> Property Tables -> Index [ 0 ] -> Properties.
  7. Press Auto Fill again. The property type of cesium#longitude will still be Int 8. 🐛
  8. Delete the properties again, as in step (6).
  9. Click the main Cesium OSM Buildings Actor at the top of the Details panel.
  10. Click Refresh Tileset
  11. Click the CesiumFeaturesMetadata component again.
  12. Click Auto Fill
  13. Expand cesium#longitude and notice that the property type is now correctly Float 64. It seems that refreshing the tileset after the metadata component is added fixes something.
    image

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.

Looks good to me! Just minor comments that might not even require any action.

@@ -25,7 +26,7 @@

using namespace CesiumTextureUtility;

namespace CesiumEncodedFeaturesMetadata {
namespace EncodedFeaturesMetadata {
Copy link
Member

Choose a reason for hiding this comment

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

This is probably fine, but we do need to keep sight of the fact that even private types pollute a global namespace in Unreal builds. In shipping builds, every symbol - public or private, across all plugins/modules - get dumped into one executable. So if we have a class name that conflicts with one in some other plugin, users may get linker errors when they combine the two plugins.

I'm remembering now that there's a line in our style guide saying private types shouldn't be prefixed with Cesium. That reflects our common practice, but I think it may not have been a well-considered choice. It's fine as long as conflicts are unlikely.

I think if I were starting over with Cesium for Unreal today, I'd probably put everything in a CesiumForUnreal namespace (except maybe UObjects? I'm not sure) in order to avoid the issue entirely.

But anyway, the chance of conflict in this particular case is really low, so I'm fine with the rename. I just wanted to mention it so we don't go renaming all the private types.

@j9liu
Copy link
Contributor Author

j9liu commented Apr 23, 2025

Thanks @kring for the review! I was able to reproduce the bug you found.

Stepping through the debugger -- it looks like some tiles actually have a "valid" cesium#longitude property that is an Int8 scalar. But then other tiles have the property as a Float64. The "Auto Fill" system makes a blanket assumption that metadata properties are uniformly defined across the tileset, and that assumption falls apart for something like OSM Buildings.

I'd still like to step through and see how those Int8 properties are being initialized -- it might be a bug in cesium-native, for instance -- but I think this is just demonstrating the fundamental flaw in the "Auto Fill" system, because it simply picks up the first type definition it finds.

@kring
Copy link
Member

kring commented Apr 29, 2025

Thanks @j9liu!

@kring kring merged commit d553fb1 into main Apr 29, 2025
23 checks passed
@kring kring deleted the more-metadata-reorg branch April 29, 2025 04:35
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.

2 participants