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

Support schemaUri property of EXT_structural_metadata #969

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

azrogers
Copy link
Contributor

As noted in #727, though EXT_structural_metadata can specify a schemaUri that contains the URI of a JSON file to load instead of the embedded schema, we don't currently support this property. This change adds that support.

As part of the change, I gave the generate-classes tool some idea of what a "shared asset" is, so it can now generate a shared asset property directly rather than needing to mess with derived classes like in the case of Image/ImageSpec. The largest change, by lines of code, is that a getSizeBytes method is now generated for every type. This is necessary for the SharedAssetDepot to know when it's "full."

Code needs cleanup, especially the JavaScript code which I don't have a formatter set up for. Also need to test in each of the runtimes - if there's any changes needed, they'll be small, but I haven't checked yet.

Base automatically changed from shared-assets to main October 30, 2024 03:34
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.

@azrogers this looks great! There are just some details in the generated getSizeBytes implementations that need to be worked through.

CesiumGltf/generated/include/CesiumGltf/AccessorSparse.h Outdated Show resolved Hide resolved
CesiumGltf/generated/include/CesiumGltf/AccessorSpec.h Outdated Show resolved Hide resolved
CesiumGltf/generated/include/CesiumGltf/AccessorSpec.h Outdated Show resolved Hide resolved
@azrogers
Copy link
Contributor Author

azrogers commented Nov 7, 2024

@kring Updated the code generation based on your comments, should be good for another look when you get the chance!

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.

Looking good, just a little more getSizeBytes nitpicking! Also, don't forget that all of the implementations of that method need to chain to their base class (usually ExtensibleObject) implementation, too!

CesiumGltf/generated/include/CesiumGltf/Asset.h Outdated Show resolved Hide resolved
CesiumGltf/generated/include/CesiumGltf/Class.h Outdated Show resolved Hide resolved
CesiumUtility/include/CesiumUtility/JsonValue.h Outdated Show resolved Hide resolved
@azrogers
Copy link
Contributor Author

@kring Just letting you know this should be good for another look!

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.

Nearly there! Just one tricky getSizeBytes situation left that I'm going to take a quick look at myself.

CesiumUtility/include/CesiumUtility/ExtensibleObject.h Outdated Show resolved Hide resolved
CesiumUtility/include/CesiumUtility/ExtensibleObject.h Outdated Show resolved Hide resolved
CesiumUtility/include/CesiumUtility/ExtensibleObject.h Outdated Show resolved Hide resolved
CesiumUtility/include/CesiumUtility/ExtensibleObject.h Outdated Show resolved Hide resolved
CesiumUtility/include/CesiumUtility/ExtensibleObject.h Outdated Show resolved Hide resolved
CesiumUtility/include/CesiumUtility/JsonValue.h Outdated Show resolved Hide resolved
@kring kring added this to the December 2024 Release milestone Nov 14, 2024
@azrogers
Copy link
Contributor Author

@kring Updated the doc comments and added const to those for(auto& ... loops!

@kring
Copy link
Member

kring commented Nov 15, 2024

All changes look good! But I tried regenerating the 3D Tiles (npm run generate-3d-tiles) and quantized-mesh (npm run generate-quantized-mesh-terrain) classes, and the generated code has compiler errors in both cases.

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