feat(gltf): extensions, encoding feature metadata #2972

mspivak-actionengine · 2024-04-18T13:47:53Z

No description provided.

ibgreen · 2024-04-18T13:56:50Z

modules/gltf/src/lib/extensions/EXT_mesh_features.ts

+ */
+export function encodeExtMeshFeatures(
+  scenegraph: GLTFScenegraph,
+  featureIdArray: number[] | TypedArray,


Normally we would create the extension with these fields, and then call encode() on the extension.

ibgreen · 2024-04-18T13:57:04Z

modules/gltf/src/lib/extensions/EXT_mesh_features.ts

+ * @returns primitive updated
+ * @see https://github.com/CesiumGS/glTF/tree/3d-tiles-next/extensions/2.0/Vendor/EXT_mesh_features
+ */
+export function encodeExtMeshFeatures(


The convention in the extension files is that each extension exports decode and encode functions. That way one can work with a list of extensions and call this function.

modules/gltf/src/lib/extensions/EXT_mesh_features.ts

modules/gltf/test/lib/extensions/EXT_mesh_features.spec.ts

belom88

Remove tile-converter changes. We will change the tile-converter in separate RPs

modules/gltf/src/index.ts

belom88 · 2024-08-27T13:42:13Z

modules/gltf/src/lib/encoders/encode-gltf.ts

 }

 function convertBuffersToBase64(gltf, {firstBuffer = 0} = {}) {
-  if (gltf.buffers && gltf.buffers.length > firstBuffer) {
-    throw new Error('encodeGLTF: multiple buffers not yet implemented');
+  if (gltf.buffers && gltf.buffers.length > firstBuffer   1) {


Why this logic has changed? Probably, breaking change

Why the function name is "convertBuffersToBase64"? It doesn't convert anything, just check number of buffers

belom88 · 2024-08-27T13:47:04Z

modules/gltf/src/lib/extensions/EXT_mesh_features.ts

 /**
 * Decodes feature metadata from extension.
 * @param {GLTFScenegraph} scenegraph - Instance of the class for structured access to GLTF data.
 * @param {GLTFLoaderOptions} options - GLTFLoader options.
 */
-function decodeExtMeshFeatures(scenegraph: GLTFScenegraph, options: GLTFLoaderOptions): void {
+export function decodeExtMeshFeatures(


Why is it exported? We don't need it

belom88 · 2024-08-27T13:47:25Z

modules/gltf/src/lib/extensions/EXT_mesh_features.ts

+  Encoding data
+*/
+
+export function encodeExtMeshFeatures(scenegraph: GLTFScenegraph, options: GLTFWriterOptions) {


Don't need to export it

modules/gltf/src/lib/extensions/EXT_mesh_features.ts

belom88 · 2024-08-27T13:57:02Z

modules/gltf/test/lib/extensions/EXT_mesh_features.spec.ts

+  const typedArray = new Uint32Array(featureIds);
+  const tableIndex = 8;
+
+  createExtMeshFeatures(scenegraph, primitive, typedArray, tableIndex);


The extensions should have interfaces "decode" and "encode". createExtMeshFeature is not necessary

As discussed the removing this function results in the necessity to put all input data into GLTF which complicates the implementation.

belom88 · 2024-08-27T13:59:01Z

modules/gltf/test/lib/extensions/EXT_structural_metadata.spec.ts

+
+test('gltf#EXT_structural_metadata - Should encode', async (t) => {
+  const scenegraph = new GLTFScenegraph();
+  const tableIndex = createExtStructuralMetadata(scenegraph, ATTRIBUTES);


Extensions should work through encode and decode. createExtStructuralMetadata is not necessary and have to be included into encode workflow.

As discussed the removing this function results in the necessity to put all input data into GLTF which complicates the implementation.

ibgreen · 2024-08-27T16:41:13Z

@mspivak-actionengine i this ready for review?

mspivak-actionengine · 2024-08-29T11:37:09Z

@mspivak-actionengine i this ready for review?

@ibgreen Please give me some time to fix the Victor's comments, which might change some logic. I'll let you know when ready. Sorry for that.

belom88

I suggest landing EXT_MESH_FEATURES in this PR and separate EXT_STRUCTURAL_METADATA in an another one.

belom88 · 2024-08-30T08:33:26Z

modules/gltf/src/lib/extensions/EXT_mesh_features.ts

+/**
+ * Creates ExtMeshFeatures, creates a featureId containing feature ids provided.
+ * @param scenegraph - Instance of the class for structured access to GLTF data.
+ * @primitive - target primitive instance that will contain the extension


Suggested change

* @primitive - target primitive instance that will contain the extension

* @param primitive - target primitive instance that will contain the extension

belom88 · 2024-08-30T08:35:04Z

modules/gltf/src/lib/extensions/EXT_mesh_features.ts

@@ -75,6  85,8 @@ function processMeshPrimitiveFeatures(
    // Process "Feature ID by Texture Coordinates"
    else if (typeof featureId.texture !== 'undefined' && options?.gltf?.loadImages) {
      featureIdData = getPrimitiveTextureData(scenegraph, featureId.texture, primitive);
+      // Clean up input data
+      featureId.texture = undefined;


Why do we need this for encoding? I'd remove

It's for decoding. I have removed it and changed the encoding part to make sure that the featureId object doesn't contain anything that can come from the original data (round-trip scenario as an example).

belom88 · 2024-08-30T08:41:33Z

modules/gltf/src/lib/extensions/EXT_mesh_features.ts

+    primitive.extensions[EXT_MESH_FEATURES_NAME] = extension;
+  }
+
+  const featureIds: GLTF_EXT_mesh_features_featureId[] = extension.featureIds;


Nit

Suggested change

const featureIds: GLTF_EXT_mesh_features_featureId[] = extension.featureIds;

const { featureIds } = extension;

belom88 · 2024-08-30T08:45:47Z

modules/gltf/src/lib/extensions/EXT_mesh_features.ts

+  // Search for all "_FEATURE_ID_n" attribures in the primitive provided if any.
+  // If there are some, e.g. "_FEATURE_ID_0", "_FEATURE_ID_1",
+  // we will add a new one with the name "_FEATURE_ID_2"
+  const attrs = Object.keys(attributes).filter((item) => item.indexOf(prefix) !== -1);


Nit: we need startsWith logic that is more precise.

Suggested change

const attrs = Object.keys(attributes).filter((item) => item.indexOf(prefix) !== -1);

const attrs = Object.keys(attributes).filter((item) => item.indexOf(prefix) === 0);

belom88 · 2024-08-30T08:53:30Z

modules/gltf/src/lib/extensions/EXT_mesh_features.ts

+export function createExtMeshFeatures(
+  scenegraph: GLTFScenegraph,
+  primitive: GLTFMeshPrimitive,
+  featureIdArray: TypedArray,


Output of decode is NumericArray. Lets consume similar data here. Can we narrow to this type in GLTF_EXT_mesh_features_featureId?

belom88 · 2024-08-30T09:17:17Z

modules/gltf/src/lib/extensions/EXT_mesh_features.ts

+    if (featureId.data) {
+      const {accessorKey, index} = createAccessorKey(primitive.attributes);
+      featureId.attribute = index;
+      const typedArray = new Uint32Array(featureId.data as number[]);


Suggested change

const typedArray = new Uint32Array(featureId.data as number[]);

const typedArray = new Uint32Array(featureId.data as NumericArray);

belom88 · 2024-08-30T09:36:43Z

modules/gltf/src/lib/extensions/EXT_structural_metadata.ts

+ */
+export function createExtStructuralMetadata(
+  scenegraph: GLTFScenegraph,
+  typedFeatureAttributes: TypedFeatureAttribute[],


The extension should work with encode function (similar to EXT_MESH_FEATURES).
We need th create... function but locate it after encode.
featureAttributes is an I3S entities. We need something more generic. Look at the propertyTable which is an output of this extension decode.

It will be addressed in a separate PR for the EXT_structural _metadata.

belom88

JSON.parse(JSON.stringify is a hack. Try to avoid it in tests.

belom88 · 2024-09-02T05:38:59Z

modules/gltf/src/lib/extensions/EXT_mesh_features.ts

+      // Clean up featureId object.
+      // Everything that could come from the original extension in case of round-trip decode/encode operations should be deleted.
+      // We need make sure the featureId object is clean.


What this comment for? You say "clean up" and then add new objects to the gltf. Forgot a real clean up?

belom88 · 2024-09-02T05:47:47Z

modules/gltf/test/lib/extensions/EXT_mesh_features.spec.ts

+
+test('gltf#EXT_mesh_features - Should decode', async (t) => {
+  const options = {gltf: {loadImages: true, loadBuffers: true}};
+  const gltf = JSON.parse(JSON.stringify(GLTF_WITH_EXTENSION));


To avoid affecting test data, the better solution is making a function wrapper:

const getGltfWithExtension = () => ({ buffers: [ { arrayBuffer: [], byteOffset: 0, byteLength: 128 } ], images: [ .... })

Done. Thanks for the idea.

belom88 · 2024-09-02T05:48:48Z

modules/gltf/test/lib/extensions/EXT_mesh_features.spec.ts

+  const gltf = JSON.parse(JSON.stringify(GLTF_WITH_EXTENSION));
+  gltf.buffers[0].arrayBuffer = new Uint8Array(binaryBufferDataAlligned).buffer;
+  await decodeExtensions(gltf, options);
+  // The clone of GLTF_WITH_EXTENSION has been modified


Comment before an empty line is confusing

belom88 · 2024-09-02T05:52:41Z

modules/gltf/test/lib/extensions/EXT_mesh_features.spec.ts

+  encodeExtensions(scenegraph.gltf, {});
+
+  // The clone of GLTF_WITH_EXTENSION has been modified
+  t.deepEqual(JSON.stringify(primitive), JSON.stringify(PRIMITIVE_EXPECTED));


deepEqual should work without JSON.stringify

belom88 · 2024-09-02T05:59:00Z

modules/gltf/test/lib/extensions/EXT_mesh_features.spec.ts

+  // The clone of GLTF_WITH_EXTENSION has been modified
+  const gltfBin = encodeExtensions(gltf, options);
+
+  const json = deleteUndefined(gltfBin.json);


add undefined values to the expected object, probably

modules/gltf/src/gltf-writer.ts

mspivak-actionengine · 2024-09-02T11:47:55Z

@ibgreen Hi Ib. The PR is ready. Could you please review it?

ibgreen

Conditionally approved.

Required

Update docs and release notes

Nice to have

A bit more comments in the code to guide a user who is not familiar with the extension.

modules/gltf/src/lib/extensions/EXT_mesh_features.ts

ibgreen · 2024-09-02T13:42:11Z

modules/gltf/src/lib/extensions/EXT_mesh_features.ts

+    return;
+  }
+  const featureIds: GLTF_EXT_mesh_features_featureId[] = extension.featureIds;
+  featureIds.forEach((featureId, ind) => {


Type out variable names

Suggested change

featureIds.forEach((featureId, ind) => {

featureIds.forEach((featureId, index) => {

ibgreen · 2024-09-02T13:43:52Z

modules/gltf/src/lib/extensions/EXT_mesh_features.ts

+  if (!extension) {
+    return;
+  }
+  const featureIds: GLTF_EXT_mesh_features_featureId[] = extension.featureIds;


Would it be possible to clone the feature ids so that we don't mutate the existing data structure?

Suggested change

const featureIds: GLTF_EXT_mesh_features_featureId[] = extension.featureIds;

const featureIds: GLTF_EXT_mesh_features_featureId[] = {...extension.featureIds};

I'm afraid not. While encoding/decoding extensions we use temporal fields in the structure and change the data accordingly.

ibgreen · 2024-09-02T13:44:20Z

modules/gltf/src/lib/extensions/EXT_mesh_features.ts

+  });
+}
+
+function createAccessorKey(attributes: {}) {


Please add documentation and types to every function

belom88

The logic of scenegraph was:

Create scenegraph;
Make changes;
Call createBinaryChunk in the current scope in order to save buffer changes

The logic of sceengraph now is not obvious. You have to do

  scenegraph.gltf.buffers.push({
        arrayBuffer: typedArray.buffer,
        byteOffset: typedArray.byteOffset,
        byteLength: typedArray.byteLength
      });

that is outside of scenegraph in the extension. Whithout this piece of code your approach won't work.
I'd preserve the logic from our previous inspection to save time. We can support deferred call of createBinaryChunk in a separate PR because we should think a little bit more on it.

belom88 · 2024-09-06T11:02:22Z

modules/gltf/src/gltf-writer.ts

+  const scenegraph = new GLTFScenegraph(gltf);
+  scenegraph.createBinaryChunk();
+  const gltfToEncode = scenegraph.gltf;


We don't need createBinaryChunk here. It's done in the extension already.

belom88 · 2024-09-06T11:03:19Z

modules/gltf/src/lib/extensions/EXT_mesh_features.ts

+// export function encode(gltfData: {json: GLTF}, options: GLTFWriterOptions): void {
+//   const scenegraph = new GLTFScenegraph(gltfData);
+//   encodeExtMeshFeatures(scenegraph, options);
+// }


feat(gltf): extentions, encoding

3a90cc7

mspivak-actionengine requested a review from belom88 April 18, 2024 13:48

ibgreen reviewed Apr 18, 2024

View reviewed changes

belom88 requested changes Apr 22, 2024

View reviewed changes

modules/gltf/src/lib/extensions/EXT_mesh_features.ts Outdated Show resolved Hide resolved

modules/gltf/test/lib/extensions/EXT_mesh_features.spec.ts Outdated Show resolved Hide resolved

mspivak-actionengine added 2 commits May 16, 2024 12:03

encoding extensions

4fcea4e

Merge branch 'master' into ms/ext-mesh-i3s-to-3dtiles

57e3bd7

belom88 requested changes May 16, 2024

View reviewed changes

rollback converter changes

0b3ca36

mspivak-actionengine requested a review from belom88 May 20, 2024 13:34

mspivak-actionengine added 6 commits May 21, 2024 15:59

Use new scenegraph object in encodeGLTFSync

9531f4f

Merge branch 'master' into ms/ext-mesh-i3s-to-3dtiles

698217e

merged master AND added commit fixing undefined options in glb-writer

81ef136

support binary chunk in scenegraph constructor

b76e23c

Merge branch 'master' into ms/ext-mesh-i3s-to-3dtiles

4c45d8c

lint fix

4e2860f

mspivak-actionengine requested a review from ibgreen August 27, 2024 12:49

belom88 requested changes Aug 27, 2024

View reviewed changes

belom88 mentioned this pull request Aug 27, 2024

chore: binary chunks refactoring #3070

Closed

mspivak-actionengine added 2 commits August 29, 2024 16:08

fix dev review issues, fix tests and scengraph recreation issues

f9486c2

remove two unnecessary exports

4ca8797

mspivak-actionengine requested a review from belom88 August 29, 2024 13:36

belom88 requested changes Aug 30, 2024

View reviewed changes

address dev review issues

bfb807a

mspivak-actionengine requested a review from belom88 August 30, 2024 15:34

belom88 requested changes Sep 2, 2024

View reviewed changes

address dev review issues

7c7d87f

ibgreen approved these changes Sep 2, 2024

View reviewed changes

address dev review issues

851ae4d

mspivak-actionengine requested a review from belom88 September 3, 2024 13:58

belom88 requested changes Sep 4, 2024

View reviewed changes

address dev review issues and revert scenegraph changes

3f3b87d

mspivak-actionengine requested a review from belom88 September 4, 2024 15:03

Merge branch 'master' into ms/ext-mesh-i3s-to-3dtiles

674a98b

belom88 requested changes Sep 6, 2024

View reviewed changes

address dev review issues

0950b4d

mspivak-actionengine requested a review from belom88 September 6, 2024 12:10

belom88 approved these changes Sep 6, 2024

View reviewed changes

mspivak-actionengine merged commit f0c1a28 into master Sep 6, 2024
1 check passed

mspivak-actionengine deleted the ms/ext-mesh-i3s-to-3dtiles branch September 6, 2024 13:38

	* @primitive - target primitive instance that will contain the extension
	* @param primitive - target primitive instance that will contain the extension

	const featureIds: GLTF_EXT_mesh_features_featureId[] = extension.featureIds;
	const { featureIds } = extension;

	const attrs = Object.keys(attributes).filter((item) => item.indexOf(prefix) !== -1);
	const attrs = Object.keys(attributes).filter((item) => item.indexOf(prefix) === 0);

	const typedArray = new Uint32Array(featureId.data as number[]);
	const typedArray = new Uint32Array(featureId.data as NumericArray);

	featureIds.forEach((featureId, ind) => {
	featureIds.forEach((featureId, index) => {

feat(gltf): extensions, encoding feature metadata #2972

feat(gltf): extensions, encoding feature metadata #2972

Conversation

mspivak-actionengine commented Apr 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

belom88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibgreen commented Aug 27, 2024 • edited by mspivak-actionengine Loading

mspivak-actionengine commented Aug 29, 2024

belom88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

belom88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mspivak-actionengine commented Sep 2, 2024

ibgreen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

belom88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibgreen commented Aug 27, 2024 •

edited by mspivak-actionengine

Loading