Skip to content

Update GltfWriter to use autogenerated files#416

Merged
kring merged 17 commits intomainfrom
gltf-writer
Jan 21, 2022
Merged

Update GltfWriter to use autogenerated files#416
kring merged 17 commits intomainfrom
gltf-writer

Conversation

@lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Dec 17, 2021

This is a follow up to #412 that replaces the current GltfWriter with an autogenerated GltfWriter.

The new writer takes a very hands-off approach. It serializes the CesiumGltf struct as-is without any special handling for external files or data uris. It's expected that the user would manage this on their own before calling writeGltf or writeGlb. This was to avoid injecting custom code in the autogenerated code or copying or modifying the CesiumGltf struct that's passed in. The full doc is below.

  /**
   * @brief Serializes the provided model into a glTF JSON byte vector.
   *
   * @details Ignores internal data such as {@link CesiumGltf::BufferCesium}
   * and {@link CesiumGltf::ImageCesium} when serializing the glTF. Internal
   * data must either be converted to data uris or saved as external files. The
   * buffer.uri and image.uri fields must be set accordingly prior to calling
   * this function.
   *
   * @param model The model.
   * @return The result of writing the glTF.
   */
  GltfWriterResult writeGltf(const CesiumGltf::Model& model) const;

  /**
   * @brief Serializes the provided model into a glb byte vector.
   *
   * @details The first buffer object implicitly refers to the GLB binary chunk
   * and should not have a uri. Ignores internal data such as
   * {@link CesiumGltf::BufferCesium} and {@link CesiumGltf::ImageCesium}.
   *
   * @param model The model.
   * @param bufferData The buffer data to store in the GLB binary chunk.
   * @return The result of writing the glb.
   */
  GltfWriterResult writeGlb(
      const CesiumGltf::Model& model,
      const std::vector<std::byte>& bufferData) const;

@lilleyse lilleyse requested a review from kring December 17, 2021 23:00
#include <vector>

// OPAQUE is defined in wingdi.h
#undef OPAQUE
Copy link
Contributor

Choose a reason for hiding this comment

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

this turns out to be quite cumbersome to use the glTF library in O3DE too. I think we should rename the enum to something else (Maybe add prefix gltf to enum)

Copy link
Contributor

Choose a reason for hiding this comment

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

or maybe use Opaque as a name with the first letter capitalized (Not entirely fool-proofed for what window does, but seems to be good enough)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it's unfortunate. I'm not happy with undef'ing things in Cesium Native so maybe renaming is a better choice. We already do that already for properties that conflict with reserved cpp words, like "class" becomes "clasProperty". Maybe "OPAQUE" could become "OPAQUE_ENUM".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the code

@lilleyse
Copy link
Contributor Author

Pushed another commit 108c90b so the enums in EXT_meshopt_compression are processed correctly (here). It should also support the older style enums in AGI_articulations (here) and KHR_lights_punctual (here) though I haven't tried those.

@lilleyse lilleyse changed the base branch from main to misc-generator December 27, 2021 20:17
@lilleyse
Copy link
Contributor Author

All the code generator and cleanup changes are now in #418 so make sure to review that one first.

Base automatically changed from misc-generator to main January 21, 2022 03:07
@kring kring merged commit de933c2 into main Jan 21, 2022
@kring kring deleted the gltf-writer branch January 21, 2022 03:18
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.

3 participants