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

Baked vertex animation textures #11317

Merged
merged 42 commits into from
Nov 4, 2021
Merged

Baked vertex animation textures #11317

merged 42 commits into from
Nov 4, 2021

Conversation

brunobg
Copy link
Contributor

@brunobg brunobg commented Oct 20, 2021

This PR implements Vertex Animation Textures, according to the discussion at https://forum.babylonjs.com/t/vertex-animation-texture-module-implementation/24576.

Suggestion of how to provide proper tests are welcome.

Code fragment to test this in the playground:

let baker = null, mesh = null;
    BABYLON.SceneLoader.ImportMeshAsync(
        "",
        "https://raw.githubusercontent.com/RaggarDK/Baby/baby/",
        "arr.babylon",
        scene,
        undefined
    ).then((importResult) => {
        mesh = importResult.meshes[0];
        baker = new BABYLON.VertexAnimationBaker(scene, mesh);
        return baker.bakeVertexData([{from: 1, to: 20, name: "aaa"}]);
    }).then((vertexData) => {
        const vertexTexture = baker.textureFromBakedVertexData(vertexData);
        mesh.material.bakedVertexAnimationMap.texture = vertexTexture;
        mesh.material.bakedVertexAnimationMap.animationParameters = new BABYLON.Vector4(
            0,
            20,
            0, 
            30
        );

        scene.registerBeforeRender(() => {
            mesh.material.bakedVertexAnimationMap.time += 1/60;
        });
    });

Specific points for code reviewers:

  • check if I implemented time correctly.
  • check IMaterialBakedVertexAnimationDefines. Does anybody see a define that we need? I don't see one.
  • IMaterialBakedVertexAnimationDefines.setAnimationParameters: I have no idea how to handle this. It's actually meant to be a set of parameters in an instanceBuffer, so I don't even think it belongs to that class. But I want to provide a helper function so it isn't an opaque call like mesh.instancedBuffers.bakedVertexAnimationSettings = new Vector4(...).
    • Should it be part of Mesh?
    • Is there a way to make it work for non-instance meshes as well? One may want to use this for a pure mesh.
    • Same for SPS, any way to make this work properly with SPS, or would instancedBuffers work for particles?
    • same for thinInstanceSetBuffer.

Things left to implement, still open (feedback welcome):

  • change PBR material like StandardMaterial
  • Test this: Maybe this._mesh.skeleton.prepare() will work instead of doing a full this._scene.render() call.
  • automatic tests (suggestions here, please)
  • In the instanced case, the animation data will be provided by the bakedVertexAnimationSettings instance buffer of the mesh (that the user will have to create themselves - maybe with the help of the baker class). => see what the baker misc class can do to help with this.
  • add entries to the inspector:
    • inspector/src/components/actionTabs/tabs/propertyGrids/materials/pbrMaterialPropertyGridComponent.tsx
    • inspector/src/components/actionTabs/tabs/propertyGrids/materials/standardMaterialPropertyGridComponent.tsx
  • some way to serialize textures to files
  • some way to load textures from files
  • documentation with a playground example
  • credit @raggar
  • should I add #include<bakedVertexAnimation> and #include<bakedVertexAnimationDeclaration> in every .fx file that has #include<bonesVertex> and #include<bonesDeclaration>?

Open questions:

  • should this be integrated into the animation system? Perhaps Mesh.beginAnimation() or Scene.beginAnimation() could use VATs when they are available? If so VATs should be called by the animation system. If this is the case I'll need some guidelines as to how this should be done.

@sebavan sebavan requested a review from Popov72 October 20, 2021 21:07
@sebavan sebavan marked this pull request as draft October 20, 2021 21:09
Copy link
Member

@sebavan sebavan left a comment

Choose a reason for hiding this comment

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

This looks pretty promising.

The main left over topic you could discuss with @Popov72 is how to retrieve the setup and how to export it :-)

(@brunobg, you can disregard this comment) @Popov72 / @RaananW I am always wondering if you have any ideas in order to decouple this fully from the material ? would be great to have a kind of more plugin way of doing it for both size and perfs ?

dist/preview release/what&#39;s new.md Outdated Show resolved Hide resolved
src/Misc/bakeVertexAnimation.ts Outdated Show resolved Hide resolved
src/Misc/bakeVertexAnimation.ts Outdated Show resolved Hide resolved
src/Shaders/ShadersInclude/bakedVertexAnimation.fx Outdated Show resolved Hide resolved
src/Shaders/ShadersInclude/bakedVertexAnimation.fx Outdated Show resolved Hide resolved
@brunobg
Copy link
Contributor Author

brunobg commented Oct 21, 2021

This looks pretty promising.

Thanks! There is still a lot to do but I think it will look good in the end, and thank you very much for all the help you're giving me.

The main left over topic you could discuss with @Popov72 is how to retrieve the setup and how to export it :-)

Yes, some ideas here would be helpful!

(@brunobg, you can disregard this comment) @Popov72 / @RaananW I am always wondering if you have any ideas in order to decouple this fully from the material ? would be great to have a kind of more plugin way of doing it for both size and perfs ?

May I not disregard it? :D When I was first thinking about how I could write this as a module the thought of monkey patching the material shader code came to me. Perhaps you could make the base glsl material shaders, like default.vertex.fx, more flexible. Perhaps making MaterialDefines more like CustomMaterial. This would make the material compilation slightly slower, but there would be an added benefit that all materials could be easily customized.

Effects that currently require postprocessing now could be done in one pass by patching the material. I can see a lot of cases for that. My own: I currently render caustics in postprocessing, which is a RTT. If I could do something like material.appendToLastFragmentPhase(myCausticsShaderCode) I could do everything in one pass.

@brunobg
Copy link
Contributor Author

brunobg commented Oct 21, 2021

@sebavan thanks for the review, but could you give me a quite guideline on how to write a test for this? Some existing test that I could use as a guideline? I miss being able to test this code while writing it.

Copy link
Contributor

@Popov72 Popov72 left a comment

Choose a reason for hiding this comment

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

Great work! Let's make this happen!

src/Shaders/ShadersInclude/bakedVertexAnimation.fx Outdated Show resolved Hide resolved
src/Shaders/ShadersInclude/bakedVertexAnimation.fx Outdated Show resolved Hide resolved
src/Shaders/ShadersInclude/bakedVertexAnimation.fx Outdated Show resolved Hide resolved
src/Shaders/default.vertex.fx Outdated Show resolved Hide resolved
@Popov72
Copy link
Contributor

Popov72 commented Oct 21, 2021

@sebavan thanks for the review, but could you give me a quite guideline on how to write a test for this? Some existing test that I could use as a guideline? I miss being able to test this code while writing it.

We don't usually write unit tests (even if we have a number of them), but we generally prefer to write visual validation tests, to make sure the output is not corrupted when adding some new code to the codebase. We can deal with this at the very end when everything is working.

@Popov72
Copy link
Contributor

Popov72 commented Oct 21, 2021

check if I implemented time correctly.

You should simply update the time property and the bindForSubMesh method will do the ubo update:

    mesh.material.bakedVertexAnimationMap.time += timeElapsed;

@sebavan
Copy link
Member

sebavan commented Oct 21, 2021

May I not disregard it? :D When I was first thinking about how I could write this as a module the thought of monkey patching the material shader code came to me. Perhaps you could make the base glsl material shaders, like default.vertex.fx, more flexible. Perhaps making MaterialDefines more like CustomMaterial. This would make the material compilation slightly slower, but there would be an added benefit that all materials could be easily customized.

Effects that currently require postprocessing now could be done in one pass by patching the material. I can see a lot of cases for that. My own: I currently render caustics in postprocessing, which is a RTT. If I could do something like material.appendToLastFragmentPhase(myCausticsShaderCode) I could do everything in one pass.

Ok I love where this is going and I am totally in with the full more customizable approach with this being fully side loadable !!!

We just need to find the right pattern approach for it. @RaananW and @Popov72 any ideas ?

@Popov72
Copy link
Contributor

Popov72 commented Oct 22, 2021

I think it's possible to setup a mechanism on the js side to have material plugins, as our material extensions are all having the same structure:

  • they inject some additional defines in the material define list
  • they have a number of additional properties
  • they implement the same set of methods: isReadyForSubMesh, prepareDefines, bindForSubMesh, hasTexture, etc
  • [UPDATE] they add a number of additional uniforms in the material ubo

However, I can't see how we can devise a generic way of injecting shader code in our standard/PBR materials... Depending on the material extension, the code must not be injected in the same location, and sometimes it is quite interlaced with the existing code (see the PBR extensions for eg)...

So, for our internal purposes I think we can setup this material plugin scheme because we will directly update the shader files, but Babylon.js users won't be able to write their own extensions that way, except by overwriting BABYLON.ShaderStore.ShadersStore["defaultVertexShader"] / BABYLON.ShaderStore.ShadersStore["defaultPixelShader"] (for standard material) by their own updated shader code.

@sebavan
Copy link
Member

sebavan commented Oct 22, 2021

I totally agree but it might already be a great start for us ? and it is pretty similar to custom materials :-)

At least it would prevent growing our features inorganically and reduce the default size of the shaders/code to parse in case your app never rely on some of the features.

@brunobg
Copy link
Contributor Author

brunobg commented Oct 22, 2021

However, I can't see how we can devise a generic way of injecting shader code in our standard/PBR materials... Depending on the material extension, the code must not be injected in the same location, and sometimes it is quite interlaced with the existing code (see the PBR extensions for eg)...

I see. But perhaps you could have a few specific points where code could be injected? For example, right before the end of the frament shader on gl_FragColor = color; we could have a insertion point for 3rd party code. While most internal code is entangled, several effects from external code could easily be added with this simple approach.

As a user I'd love this feature, and I'd like something pretty much like CustomMaterial, except instead of strings for each code fragment I'd prefer a string[], which could let me compose effects more easily.

This is out of scope for this PR (and perhaps could be moved into an issue?), but as a user I'd love this feature. If an agreement is reached I can try to tackle it once this PR is accepted.

@brunobg
Copy link
Contributor Author

brunobg commented Oct 22, 2021

@Popov72 @sebavan Code review with almost all requested changes is up.

Adding #ifndef BAKED_VERTEX_ANIMATION_TEXTURE to bonesDeclaration broke things, however, because matricesIndices and matricesWeights are not available anymore. Any tips to solve this?

@sebavan
Copy link
Member

sebavan commented Oct 22, 2021

This is out of scope for this PR (and perhaps could be moved into an issue?), but as a user I'd love this feature. If an agreement is reached I can try to tackle it once this PR is accepted.

Actually I do not think it should be Out of Scope of this PR as the system here is pretty good but pretty intrusive in the current material code and I guess we should be able to benefit from it to create the basics of the system :-)

@brunobg
Copy link
Contributor Author

brunobg commented Oct 22, 2021

@Popov72 @sebavan Code review with almost all requested changes is up.

Adding #ifndef BAKED_VERTEX_ANIMATION_TEXTURE to bonesDeclaration broke things, however, because matricesIndices and matricesWeights are not available anymore. Any tips to solve this?

Yeah, I'm a bit lost here. I think we need something like

        } else if (mesh.useBones && defines["BAKED_VERTEX_ANIMATION_TEXTURE"] && mesh.skeleton) {
            defines["NUM_BONE_INFLUENCERS"] = mesh.numBoneInfluencers;
        }

at MaterialHelper.PrepareDefinesForBones(), but I think defines["BAKED_VERTEX_ANIMATION_TEXTURE"] isn't set yet at this point.

@Popov72
Copy link
Contributor

Popov72 commented Oct 22, 2021

Adding #ifndef BAKED_VERTEX_ANIMATION_TEXTURE to bonesDeclaration broke things, however, because matricesIndices and matricesWeights are not available anymore. Any tips to solve this?

bonesDeclaration.fx should be something like this instead:

#if NUM_BONE_INFLUENCERS > 0
	attribute vec4 matricesIndices;
	attribute vec4 matricesWeights;
	#if NUM_BONE_INFLUENCERS > 4
		attribute vec4 matricesIndicesExtra;
		attribute vec4 matricesWeightsExtra;
	#endif

    #ifndef BAKED_VERTEX_ANIMATION_TEXTURE
        #ifdef BONETEXTURE
            uniform sampler2D boneSampler;
            uniform float boneTextureWidth;
        #else
            uniform mat4 mBones[BonesPerMesh];
            #ifdef BONES_VELOCITY_ENABLED
                uniform mat4 mPreviousBones[BonesPerMesh];
            #endif
        #endif

        #ifdef BONETEXTURE
            #define inline
            mat4 readMatrixFromRawSampler(sampler2D smp, float index)
            {
                float offset = index  * 4.0;	
                float dx = 1.0 / boneTextureWidth;

                vec4 m0 = texture2D(smp, vec2(dx * (offset + 0.5), 0.));
                vec4 m1 = texture2D(smp, vec2(dx * (offset + 1.5), 0.));
                vec4 m2 = texture2D(smp, vec2(dx * (offset + 2.5), 0.));
                vec4 m3 = texture2D(smp, vec2(dx * (offset + 3.5), 0.));

                return mat4(m0, m1, m2, m3);
            }
        #endif
    #endif
#endif

@Popov72
Copy link
Contributor

Popov72 commented Oct 22, 2021

Actually I do not think it should be Out of Scope of this PR as the system here is pretty good but pretty intrusive in the current material code and I guess we should be able to benefit from it to create the basics of the system :-)

I would also vote to do it in another PR to avoid over-complicating this PR. And once the current PR is done, we may have a better view of what we need to do to implement the plugin system.

src/Materials/standardMaterial.ts Outdated Show resolved Hide resolved
src/Shaders/ShadersInclude/bakedVertexAnimation.fx Outdated Show resolved Hide resolved
@brunobg
Copy link
Contributor Author

brunobg commented Oct 23, 2021

Ok, things are almost working. But for some reason the bakedVertexAnimationTextureSizeInverted.x (but not y) and bakedVertexAnimationTime uniforms are not correctly passed to the shader (if I replace them for hardcoded values, things work). Any idea of what I may have done wrong that could mess this up? It's probably some stupid typo but I've checked the code over and over again and missed it. If this is solved things should be stable and I'll port the changes to the PBR material.

I added some sanity checks too, but I miss two of them:

  • checking if the texture is a float32 texture on assignment (but I couldn't find a way to check this)
  • check if the final texture size is too big (but this is system dependent, so I'm not sure it makes sense to add this one in this code, and should be left to the developers to check themselves)

@Popov72
Copy link
Contributor

Popov72 commented Oct 23, 2021

But for some reason the bakedVertexAnimationTextureSizeInverted.x (but not y) and bakedVertexAnimationTime uniforms are not correctly passed to the shader (if I replace them for hardcoded values, things work)

See my comment, you must add the uniforms in the Material structure of the shader in the same order than in the js code.

I added some sanity checks too, but I miss two of them:

We don't check the inputs passed by the users in Babylon.js, for performance reasons. It's the responsibility of the user to pass valid parameters.

=> you can add some comments in the doc to make clear what is expected for the inputs.

@brunobg
Copy link
Contributor Author

brunobg commented Oct 23, 2021

See my comment, you must add the uniforms in the Material structure of the shader in the same order than in the js code.

Thanks. That fixed it and it's working. Basic code to test it in the playground:

    let baker = null, mesh = null;
    BABYLON.SceneLoader.ImportMeshAsync(
        "",
        "https://raw.githubusercontent.com/RaggarDK/Baby/baby/",
        "arr.babylon",
        scene,
        undefined
    ).then((importResult) => {
        mesh = importResult.meshes[0];
        baker = new BABYLON.VertexAnimationBaker(scene, mesh);
        return baker.bakeVertexData([{from: 1, to: 20, name: "My animation"}]);
    }).then((vertexData) => {
        const vertexTexture = baker.textureFromBakedVertexData(vertexData);

        mesh.material.bakedVertexAnimationMap.isEnabled = true;
        mesh.material.bakedVertexAnimationMap.texture = vertexTexture;
        mesh.material.bakedVertexAnimationMap.animationParameters = new BABYLON.Vector4(
            0,
            19,
            0, 
            30
        );

        scene.registerBeforeRender(() => {
            mesh.material.bakedVertexAnimationMap.time += 1/30;
        });
    });

Thanks for all the help! :)

If it looks good for everyone I'll work on the secondary checkboxes on the PR description. Three points I still need some feedback, please:

SPS, any way to make this work properly with SPS, or would the current code work well for particles too?

same for thinInstanceSetBuffer, will it work well?

should this be integrated into the animation system? Perhaps Mesh.beginAnimation() or Scene.beginAnimation() could use VATs when they are available? If so VATs should be called by the animation system. If this is the case I'll need some guidelines as to how this should be done.

@Popov72
Copy link
Contributor

Popov72 commented Oct 24, 2021

I have tested the PR locally and I have some comments:

  • Regarding the baker, using Promises make it slow, it takes several seconds to bake only a few animations (5/6) (also, we can see the animations playing while the processing is running). If someone wants to bake dozens or hundreds of animations as it could happen it's going to take a very long time. I think using a callback as @raggar did is better in this regard. Also, it seems fair to credit @raggar somewhere (maybe in the what's new file) as I think your work is based on his(?). For the record, here's what I came up with by using a callback instead of Promises:
import { AnimationRange } from "../Animations/animationRange";
import { RawTexture } from "../Materials/Textures/rawTexture";
import { Texture } from "../Materials/Textures/texture";
import { Mesh } from "../Meshes/mesh";
import { Scene } from "../scene";

/**
 * Class to bake vertex animation textures.
 */
export class VertexAnimationBaker {
    private _scene: Scene;
    private _mesh: Mesh;
    private _ranges: AnimationRange[];
    private _rangeIndex: number;
    private _frameIndex: number;
    private _textureIndex: number;
    private _vertexData: Float32Array;

    /**
     * Create a new VertexAnimationBaker object which can help baking animations into a texture.
     * @param scene Defines the scene the VAT belongs to
     * @param mesh Defines the mesh the VAT belongs to
     * @param skeleton Defines the skeleton the VAT belongs to
     */
    constructor(scene: Scene, mesh: Mesh) {
        this._scene = scene;
        this._mesh = mesh;
    }

    /**
     * Bakes the animation into the texture. This should be called once, when the
     * scene starts, so the VAT is generated and associated to the mesh.
     * @param ranges Defines the ranges in the animation that will be baked.
     * @returns Float32Array
     */
    public bakeVertexData(ranges: AnimationRange[], callback: (vertexData: Float32Array) => void): void {
        if (!this._mesh.skeleton) {
            throw new Error("No skeleton in this mesh.");
        }
        const boneCount = this._mesh.skeleton.bones.length;

        /** total number of frames in our animations */
        const frameCount = ranges.reduce(
            (previous: number, current: AnimationRange) => previous + current.to - current.from + 1,
            0
        );

        if (isNaN(frameCount)) {
            throw new Error("Invalid animation ranges.");
        }

        // reset our loop data
        const textureSize = (boneCount + 1) * 4 * 4 * frameCount;
        this._vertexData = new Float32Array(textureSize);

        this._scene.stopAnimation(this._mesh);
        this._mesh.skeleton.returnToRest();

        this._rangeIndex = 0;
        this._ranges = ranges;
        this._frameIndex = this._ranges[this._rangeIndex].from;
        this._textureIndex = 0;

        this._executeAnimationFrame(callback);

        // at this point we have the vertex data, so convert it to an actual texture
        // and build a material
        this._scene.stopAnimation(this._mesh);
        this._mesh.skeleton.returnToRest();
    }

    /**
     * Runs an animation frame and stores its vertex data
     *
     * @param vertexData The array to save data to.
     * @param frameIndex Current frame in the skeleton animation to render.
     * @param textureIndex Current index of the texture data.
     * @returns
     */
    private _executeAnimationFrame(callback: (vertexData: Float32Array) => void): void {
        this._scene.beginAnimation(this._mesh.skeleton, this._frameIndex, this._frameIndex, false, 1.0, () => {
            this._mesh.skeleton?.prepare();

            // generate matrices
            const skeletonMatrices = this._mesh.skeleton!.getTransformMatrices(this._mesh);
            this._vertexData.set(skeletonMatrices, (this._textureIndex++) * skeletonMatrices.length);

            this._frameIndex++;

            if (this._frameIndex <= this._ranges[this._rangeIndex].to) {
                this._executeAnimationFrame(callback);
            } else if (++this._rangeIndex < this._ranges.length) {
                this._frameIndex = this._ranges[this._rangeIndex].from;
                this._executeAnimationFrame(callback);
            } else {
                callback(this._vertexData);
            }
        });
    }

    /**
     * Builds a vertex animation texture given the vertexData in an array.
     * @param vertexData The vertex animation data. You can generate it with bakeVertexData().
     * @returns RawTexture
     */
    public textureFromBakedVertexData(vertexData: Float32Array): RawTexture {
        if (!this._mesh.skeleton) {
            throw new Error("No skeleton in this mesh.");
        }
        const boneCount = this._mesh.skeleton.bones.length;

        const texture = RawTexture.CreateRGBATexture(
            vertexData,
            (boneCount + 1) * 4,
            vertexData.length / ((boneCount + 1) * 4 * 4),
            this._scene,
            false,
            false,
            Texture.NEAREST_NEAREST,
            1
        );
        texture.name = "VAT" + this._mesh.skeleton.name;
        return texture;
    }
}
  • You need to use floor instead of ceil in VATFrameNum = ceil(VATFrameNum); else you can reach the frame n+1 when there is n frames in an animation (or more precisely frame n because we start at frame #0 - we should never go over frame n-1). Also, it appears that in Babylon.js, when you loop an animation after the first run (and for all subsequent loops), we wrap to frame #1, not to frame #0. Wrapping to frame #0 is wrong and produce hiccups. See for eg https://playground.babylonjs.com/#M8LMCY#19. Here's how I updated the code in bakedVertexAnimation.fx to deal with that, but feel free to make your own changes on the existing code:
    float VATStartFrame = BVASNAME.x;
    float VATEndFrame = BVASNAME.y;
    float VATOffsetFrame = BVASNAME.z;
    float VATSpeed = BVASNAME.w;

    float totalFrames = VATEndFrame - VATStartFrame + 1.0;
    float time = bakedVertexAnimationTime * VATSpeed / totalFrames;
    float frameCorrection = time < 1.0 ? 0.0 : 1.0;
    float numOfFrames = totalFrames - frameCorrection;
    float VATFrameNum = fract(time) * numOfFrames;
    VATFrameNum = mod(VATFrameNum + VATOffsetFrame, numOfFrames);
    VATFrameNum = floor(VATFrameNum);
    VATFrameNum += VATStartFrame + frameCorrection;
  • There's also a bug in bakedVertexAnimationsDeclaration.fx where:
float frameUV = frame * bakedVertexAnimationTextureSizeInverted.y;

should be:

float frameUV = (frame + 0.5) * bakedVertexAnimationTextureSizeInverted.y;
  • Everything does work with single mesh, instances and thin instances:
  • Regarding SPS I can't tell because I don't really know them but I don't see why it would not work, as we are only dealing with new properties of the material and an additional attribute (when using instances). But you should probably test it to be sure.
  • Regarding integrating VAT into the animation manager I'm not sure, I don't know it well enough but it seems it would be a bit complicated. Maybe we should first have it as a separate thing as it is today and see if there's a need later on? @deltakosh / @sebavan?

@brunobg
Copy link
Contributor Author

brunobg commented Oct 25, 2021

PBR material added, code review. Only a few missing bullets, feedback on them welcome to close this one! Again, many thanks to all your review and feedback.

BTW, should I add #include<bakedVertexAnimation> and #include<bakedVertexAnimationDeclaration> in every .fx file that has #include<bonesVertex> and #include<bonesDeclaration>?

Regarding the baker, using Promises make it slow, it takes several seconds to bake only a few animations (5/6) (also, we can see the animations playing while the processing is running). If someone wants to bake dozens or hundreds of animations as it could happen it's going to take a very long time. I think using a callback as @raggar did is better in this regard. Also, it seems fair to credit @raggar somewhere (maybe in the what's new file) as I think your work is based on his(?). For the record, here's what I came up with by using a callback instead of Promises:

Sorry, but I don't see why Promises would make it slow. The main loop is still async, so it shouldn't freeze the main thread. It's true that baking animations take a while, but any reasonable project will serialize them instead of generating them at run time. this._mesh.skeleton.prepare() worked, but I don't see a way to not render the object during the animation (disabling it wouldn't render it, right?). Either way, it will still take some time. If it didn't we wouldn't need VATs in the first place :)

Maybe I'm missing something, but if you want a callback instead of a promise it's just baker.bakeVertexData([{from: 1, to: 20, name: "My animation"}]).then(callback);.

All other changes and suggestions from the comment were integrated. Thanks for the examples, I'm already using them on the documentation (being written, PR to come soon https://github.com/Corollarium/Documentation/tree/vat).

serialization and loading

I'm not sure what the best format would be here. DDS seems possible, and also .hdr or .exr. I don't have enough experience with them to pick one. I can also provide a pure Float32Array serialization such as the one in the original PoC.

@Popov72
Copy link
Contributor

Popov72 commented Oct 25, 2021

Let's keep the "promise" version as we should definitely generate the texture in a pre-process step and not at runtime.

Regarding the file format, do you know what Blender is using? IIUC, Blender is able to generate this texture, so it may be interesting to use the same format on our end.

I had a chat with @sebavan about the architecture of the PR and we ended up thinking we should maybe implement it more like the morph targets are implemented... That is, not as an extension of the material but as a separate feature. The main point is that we will probably need to use different VAT textures for the same material (for example, the alien head is made of several meshes which are all using the same material but that have different skeletons, so different animations), something not possible in the current implementation: if we want to use different textures we must create different materials.

You can have a look on your side if you want to try to implement it this way. I think all of the code of the PR is still relevant, only that it will be structured differently. Notably, I think a good chunk of it will end up in the Materials/materialHelper.ts class as it is the case for the morph target feature... On my side I will have to look at the morph target / manager source code as I'm not very familiar with it, but I need first to finish my current PR before being able to help on this one.

Sorry for the late notice, but we do think it will make for a better contribution!

BTW, should I add #include and #include in every .fx file that has #include and #include?

Yes please!

Once we are happy with the status of this PR, we will also need to add support for this feature to the node material.

@Popov72
Copy link
Contributor

Popov72 commented Oct 25, 2021

Please hold on, we are still having some internal discussions about this!

I will let you know the outcome once it is clearer on our side.

@brunobg
Copy link
Contributor Author

brunobg commented Oct 25, 2021

Please hold on, we are still having some internal discussions about this!

Don't forget about #11334. Maybe it will help the implementation, and even as an independent feature it's awesome. I'd love to implement and get it ASAP, but it definitely should get a lot of thinking before it's tackled.

@brunobg
Copy link
Contributor Author

brunobg commented Oct 25, 2021

Regarding the file format, do you know what Blender is using? IIUC, Blender is able to generate this texture, so it may be interesting to use the same format on our end.

https://github.com/yumataesu/Blender.VATExporter_4TD/blob/main/vat_exporter.py uses EXR, and so does https://forums.unrealengine.com/t/vertex-animation-script-for-blender-3d-users/85580. So EXR it is! I'll work on this while discussions go on, since it's independent from the architectural choices.

Copy link
Contributor

@Popov72 Popov72 left a comment

Choose a reason for hiding this comment

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

All good for me!

Congrats @brunobg for this PR which is not a small one!

@Popov72 Popov72 marked this pull request as ready for review November 4, 2021 15:26
@@ -18,6 +18,7 @@ import { MaterialDefines } from "../Materials/materialDefines";
import { Light } from "../Lights/light";
import { Skeleton } from "../Bones/skeleton";
import { MorphTargetManager } from "../Morph/morphTargetManager";
import { BakedVertexAnimationManager } from "../BakedVertexAnimation/bakedVertexAnimationManager";
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not import but declare it
declare type BakedVertexAnimationManager = import("../BakedVertexAnimation/bakedVertexAnimationManager").BakedVertexAnimationManager ;

cc @RaananW to make sure it will not be a problem for tree shaking and side effects

Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend to expose an interface actually and use that interface here

Copy link
Contributor

Choose a reason for hiding this comment

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

This is modeled after the morph target manager: if it works for morph it should work for BVAM too(?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Morphtarget is part of what is problematic for treeshaking and side effects (same as skeleton)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an interface. I was not sure what should be there, take a look at let me know if something is missing/too much.

Copy link
Member

Choose a reason for hiding this comment

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

If it is only used as an interface typescript will remove it from the final .js file, so it should be fine.

Copy link
Member

Choose a reason for hiding this comment

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

yup but the declare prevents any maintenance misuse of the imported file :-) The interface is great I agree

@brunobg
Copy link
Contributor Author

brunobg commented Nov 4, 2021

Congrats @brunobg for this PR which is not a small one!

Me? I just sent bad commits that you fixed :)

@Popov72
Copy link
Contributor

Popov72 commented Nov 4, 2021

It seems there's a problem with AbstractMesh which now shows as being completely modified by the PR whereas it should be only a couple of lines. Maybe a problem with the line ending characters?

@RaananW
Copy link
Member

RaananW commented Nov 4, 2021

It seems there's a problem with AbstractMesh which now shows as being completely modified by the PR whereas it should be only a couple of lines. Maybe a problem with the line ending characters?

This sometimes happens if you edit the file using the github file editor. If possible you can revert those changes locally (while keeping the changes uncommited) and solve this to maintain our git history.

Also, if possible - can you revert the formatting changes in the .fx files? we don't have a formatter for those (which will be fixed very soon), but some of them only had indentation on every line. Not needed (and also messes with the git history of this specific file).

@sebavan
Copy link
Member

sebavan commented Nov 4, 2021

@brunobg this looks stuning !!! thanks @Popov72 for the support !!! I ll merge it and deploy as soon as the AbstractMesh file is fixed.

@brunobg
Copy link
Contributor Author

brunobg commented Nov 4, 2021

Sorry about the lined end issues, I should have checked that, but I don't know why VSCode mangled that file. Did a review and reverted the whitespace issues on blank lines (but left a couple on line ends. BTW, the project has an .editorconfig with trim_trailing_whitespace = true, so any future patches will remove those spaces anyway), as well as reverted a couple of files that didn't have changes in the end. I understand that the interface on MeshAbstract is okay the way I did, right, instead of the declare?

Hopefully that's all! Thanks to all of you and I can't wait to see this merged. I'll use it right away.

Copy link
Contributor

@deltakosh deltakosh left a comment

Choose a reason for hiding this comment

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

Excellent job! thanks a lot

@sebavan sebavan merged commit b6c214f into BabylonJS:master Nov 4, 2021
@brunobg
Copy link
Contributor Author

brunobg commented Nov 12, 2021

Any plans to make a new release soon? I could use this right away :)

@Popov72
Copy link
Contributor

Popov72 commented Nov 12, 2021

It's already available on the Playground / preview CDN, this PG does work:
https://playground.babylonjs.com/#14WJWW#12

If you need a NPM package I can't tell, however.

@brunobg
Copy link
Contributor Author

brunobg commented Nov 12, 2021

Ah, yes, I'm looking for the NPM package. I'll be waiting!

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.

None yet

5 participants