feat(tile-converter): support attributes data from textures #2511
Conversation
mspivak-actionengine
requested review from
belom88 and
ibgreen
and removed request for
belom88
|
@ibgreen Hi Ib, could you please review the PR? |
| @@ -50,12 +50,13 @@ export class GLTFScenegraph { | |||
| byteLength: number; | |||
|
|
|||
| // TODO - why is this not GLTFWithBuffers - what happens to images? | |||
| constructor(gltf?: {json: GLTF; buffers?: any[]}) { | |||
| constructor(gltf?: {json: GLTF; buffers?: any[]; images?: any[]}) { | |||
There was a problem hiding this comment.
| constructor(gltf?: {json: GLTF; buffers?: any[]; images?: any[]}) { | |
| constructor(gltf?: GLTFWithBuffers) { |
See comment above
There was a problem hiding this comment.
Declined. The problem is that GLTFWithBuffers has REQUIRED field "buffers", which results in necessity to change many calls of the constructor, where "gltf" doesn't have "buffers" assigned.
modules/gltf/src/lib/extensions/deprecated/EXT_feature_metadata.ts
Outdated
Show resolved
Hide resolved
modules/gltf/src/lib/extensions/deprecated/EXT_feature_metadata.ts
Outdated
Show resolved
Hide resolved
| * @param n | ||
| * @param m | ||
| */ | ||
| function emod(n: number): number { |
There was a problem hiding this comment.
@ibgreen it looks like candidate to move to @math.gl . Probably we already have this function implemented in some place?
modules/gltf/src/lib/extensions/deprecated/EXT_feature_metadata.ts
Outdated
Show resolved
Hide resolved
| promises.push(promise); | ||
|
|
||
| // Parallelize image loading and buffer loading/extension decoding | ||
| await Promise.all(promises); |
There was a problem hiding this comment.
We are removing Promise.all, that is potential performance change. The reason is we need images loaded during decodeExtensions.
@ibgreen what do you think?
modules/tile-converter/test/i3s-converter/helpers/batch-ids-extensions.spec.js
Show resolved
Hide resolved
| @@ -38,6 +38,32 @@ setLoaderOptions({ | |||
| _worker: 'test' | |||
| }); | |||
|
|
|||
| test('tile-converter - Converters#texure attributes', async (t) => { | |||
There was a problem hiding this comment.
The idea was to remove this test because the testing the whole converter takes a lot of time and diminishes CI speed
| @@ -1083,6 +1084,8 @@ type FeatureTexture = { | |||
| extras?: any; | |||
| [key: string]: any; | |||
| }; | |||
| export type {FeatureTexture as EXT_feature_metadata_feature_texture}; | |||
There was a problem hiding this comment.
- Can we have these exports at the bottom together with the other exports?
- Can we minimize the arbitrary renaming of things? - why do these need to be renamed where as other extension exports do not?
I will check an finish other comments in a separate PR |
dsavinov-actionengine
added
the
ActionEngine
No description provided.