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

Batching skins and custom shader chunks #1233

Closed
wants to merge 0 commits into from

Conversation

Glidias
Copy link
Contributor

@Glidias Glidias commented Jun 8, 2018

All changes in batching.js file....


Other changes to graphics/material files:

  • Custom shader chunk support for fog codes. (program-lib.js, standard.js)
  • Custom shader chunks support in (basic.js)

I confirm I have signed the Contributor License Agreement.

if (options.chunks) {
var customChunks = {};
var newP;
var p;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please update this PR to have 4 spaces instead of tabs?

@Glidias
Copy link
Contributor Author

Glidias commented Jun 9, 2018

After the discussion on whether basic.js should include support for custom chunks, i decided to revert back to to original and instead just rely on standard.js the main choice of material to use. Essentially, with the right settings, standard.js can easily emulate any unlit material, right? So, basic.js is just for legacy purposes it seems.

Regarding batching, when is clone(batch, meshInstances) method being used? I don't see any immediate case of it being used within the batch.js codebase itself. Regarding inverse bind poses from skin's rigs, from what I remembered, they are precomputed beforehand and remain immutable right? ie.They aren't modified during rendering so it should be safe to re-use the exact same reference instances of inverse bind poses from the same rig, without deep-cloning them, right?

@malDuffin
Copy link

malDuffin commented Jun 17, 2018 via email

material.chunks.transformVS = this.transformVS;
material.chunks.skinTexVS = this.skinTexVS;
material.chunks.skinConstVS = this.skinConstVS;
material.chunks.transformVS = this.boneLimit + "#define DYNAMICBATCH\n" + (material.chunks.transformVS ? material.chunks.transformVS : pc.shaderChunks.transformVS);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep in mind that shaders with DYNAMICBATCH define implement skinning with just one bone:
https://github.com/playcanvas/engine/blob/master/src/graphics/program-lib/chunks/transform.vert#L17

If you want proper batched skinning, you need to make shaders use SKIN define instead of it.

Copy link
Contributor Author

@Glidias Glidias Jun 19, 2018

Choose a reason for hiding this comment

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

Hmmm... like:

material.chunks.transformVS = this.boneLimit + "#define "+(!isSkin ? "DYNAMICBATCH" : "SKIN")+"\n" + (material.chunks.transformVS ? material.chunks.transformVS : pc.shaderChunks.transformVS);

Is this correct? Looks like it's not just that.... I think you mean with various blend weights across possibly multiple bones, right? As the supplied inputs must also include the relavant weights if needed. Hmm.... Looks like the previous animation tests didn't blend the bones and i didn't noticed it. Gotta see how to get this to work proper when i hav the time though.. It seems with teh SKIN define, it has problems accessing x,y,z,w properties it seems? That's weird. Maybe it's because it ALWAYS assumes component size of 4 always?

Oh wait...so that's Playcanvas skinning convention? It always uploads bone component size of 4 always for skins, regardless of the actual model setup by the artist? Isn't that slightly wasteful for VRAM for some skins that don't require blending of all 4 components? (i might be wrong, i think component size of 1 still takes up all 4 registers?). But i guess this simplifies the shader setup though, otherwise need differnet vertex program for different skin blend amounts. Or, if i'm not wrong, eg. the y,z,w properties will default to zero if only component size 1 skin is used...

Copy link
Collaborator

@guycalledfrank guycalledfrank Jun 19, 2018

Choose a reason for hiding this comment

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

I think all shader-based game engines I ever saw or reverse engineered used 4 bones per-vertex, because it's usually enough, and you don't really want to to compile/switch more shaders just because some objects use less. And 4 bone indices fit neatly into 4 bytes, and you can't have less for a vertex attribute. So PlayCanvas too used 4 bones per vert from the beginning. All imported FBX models are set up to use 4.
Dynamic batching is basically a specialized skinning mode that only uses 1 bone and no weights. But if you want to batch skins, then you'll need to switch shaders to a "normal" skinning mode with 4 bones.

Copy link
Contributor Author

@Glidias Glidias Jun 19, 2018

Choose a reason for hiding this comment

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

I meant what if the skin had less than 4 blends, etc. But anyway...yea, it would mean having to create a differnet program setup for a trivial thing just to shave off a bit of bytes seems pointless.

Ok, i did up some changes, working fine now, you can preview the results in:

https://playcanv.as/p/3QlSEgHF/

Aha, now the animations look much more fluid/real now. Previously, there wasn't any blending. Lol, didn't realised.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Pretty cool! I hope what happened to character's leg is expected though :D

image

Copy link
Contributor Author

@Glidias Glidias Jun 19, 2018

Choose a reason for hiding this comment

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

Ignore that, urm...was trying to test if IK was possible so i can align it with terrain slopes..... If you got any advice on how to get it to work, perhaps you can help. I just need basic IK solver to solve knee to ankle based off ankle position (y offset), ...also prefably solve ankle base to foot tip alignment along slope for sole base. Perhaps, the rig must be set up with certain conventions to help make the process simpler.

Copy link
Contributor Author

@Glidias Glidias left a comment

Choose a reason for hiding this comment

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

Skin blending fox for batching: The changes up above, some 3 lines changed so it uses original skinning routine to keep the original blendings.

Copy link
Contributor Author

@Glidias Glidias left a comment

Choose a reason for hiding this comment

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

(deleted duplicate)

@guycalledfrank
Copy link
Collaborator

BTW, I recommend testing if the combined mesh AABB is still correct for batched skins and doesn't waste performance.

Right now batched mesh AABB is a combination of underlying mesh instances' AABBs:
https://github.com/playcanvas/engine/blob/master/src/scene/batching.js#L967

And skinned underlying instances will try to use their transformed matrices to generate full mesh AABB:
https://github.com/playcanvas/engine/blob/master/src/scene/mesh.js#L335

Matrices are calculated for all draw calls before rendering:
https://github.com/playcanvas/engine/blob/master/src/scene/forward-renderer.js#L1034

But given batching removes original models from the list of draw calls... perhaps they'll stop updating. That means you probably need to patch BatchManager.prototype.update() to generate the AABB using the matrices you build in SkinInstanceBatched.

Here is a simple AABB visualization script: https://playcanvas.com/project/342895/overview/skinbounds

@Glidias
Copy link
Contributor Author

Glidias commented Jun 19, 2018

It should be the same as regular meshes ( which also has a ".skinInstance" ) though I'm not too sure for playcanvas if skinned mesh (ie. the one skinned with animations) conservatively uses the largest possible bounding box based off all it's animations. ( I assume it should?? Oh ...it looks like it doesn't ...). Taking this further I might handle culling manually on my own via meshInstance.cull= false, or/and since the crowd entities are moving about in gamespace, i might use my custom conservatively expanded/padded bounding boxes management to avoid updating per frame meshInstance._updateAabb =false, especially when batch counts are large.

Okay, wait, it seems that the batch's meshInstance._updateAabb is set to false in batching.js. Which means the bounding boxes of the batch's meshInstance.aabb getter won't update unless you manually call batch manager's update() function. This makes sense because it uses combined bounding boxes of all the entities to determine bounding box of the entities instead of the default prototype implemetnation. .

Important tip to avoid unnecessary calculations: When calling .aabb on the mesh instance, you should remember to call it once only since it's a getter (ie. only whenever needed to lazy-update), and then use ._aabb subsequently. Eg. from playcanvas engine code forward renderer:

        meshPos = meshInstance.aabb.center;
        if (meshInstance._aabb._radiusVer !== meshInstance._aabbVer) {
            meshInstance._aabb._radius = meshInstance._aabb.halfExtents.length();
            meshInstance._aabb._radiusVer = meshInstance._aabbVer;
        }

Notice that .aabb getter is called once only to ensure the aabb is updated if there are any skinning/dynamic-batch position changes. Then, subsequent references to aabb may simply use ._aabb raw property instead.

@Glidias
Copy link
Contributor Author

Glidias commented Aug 17, 2018

Okay, i'd proably need to update/redo this pull request to only pull from a proper branch with ONLY 1 file (batching.js) change. but anyway, to continue the discussion, here are my findings so far from my test projects (see links below at end):

To recap, contrary to what guycalledfrank mentioned, Playcanvas engine DOES update the batched skins, and you don't need to call update() manually. (However, I do know that this generalised approach may not be very performant so I've personally considered ways of circumventing this dynamic batch bounding boxes updates if performance is a concern...such as manually force-flagging batch.dynamic= false/batch.meshInstance.cull=false to prevent Playcanvas from updating it, while i roll in custom app-specific batched bounding box management/culling management schemes.) Apparently, Playcanvas' meshInstances also has an option to set _updateAabbFunc , which perhaps you could use to roll in your own custom AABB updates to better suited for your game if needed be. (eg. instead of updating the skin's bounding boxes based off vertex animation per frame, simply use a large-enough bounding box by default and then simply update bounding box's .center to match the entity's position, for example..)

It appears that somehow skinned meshInstances must remain within the scene's layer composition (ie. remain within the draw call array list!) in order to have their individual bounding boxes update correctly per frame. Otherwise, the bounding boxes appear shorter than usual (even though they still morph/adjust themselves with the animations). Why? Something to do with skinned meshes having skin binding transform updates that only apply within renderComposition of forward-renderer.js?

The project describing the above problem can be found in:
https://playcanvas.com/project/571467/overview/skinbounds-batching-test-problem

Okay, so the real (suggested/to-be-discussed ) fix would be under this project:
https://playcanvas.com/project/571469/overview/skinbounds-batching-testings

Basically, I added provision for original skinned meshes in batching.js to have their updateMatrices() be called as well. This will ensure .aabb calculaion for skinned meshes wlll have the correct matrices (i think) to properly expand the bounding box per frame. Additionally, i included various flags at the moment to forego this autoamtic per-frame calculation depending on advanced usage situation. For example, if the user deliberately sets the entire Batch's global meshInstance to cull=false or visible=false , then it's assumed the user doesn't intend to cull the batch at all via the default Playcanvas approach of combined bounding boxes) as it won't be visible or culling isn't necessary, so why bother updating the bounding boxes for that frame, since it's only used for culling for rendering? Setting any of these flags to false will therefore forego the need to update the original mesh instances' bounding boxes via Playcanvas, and thus not require the need to update matrices of original skinned meshes.

 SkinInstancesBatched.prototype = {

    updateMatrices: function (rootNode) {
        var v;
        var vp;
        
        if (this.meshInstance.cull && this.meshInstance.visible) {
            var origMeshInstances = this.origMeshInstances;
            for (v=0; v< origMeshInstances.length; v++) {
                vp = origMeshInstances[v];
                vp.skinInstance.updateMatrices(vp.node);
            }
        }
        
        
		var _invMatrix = SkinInstancesBatched.invMatrix;
	   _invMatrix.copy(rootNode.getWorldTransform()).invert();
	   
       var i =0;
	   var inverseBindPoses = this.inverseBindPoses;
	   for (v = 0; v <inverseBindPoses.length; v++) {
		   vp = inverseBindPoses[v];
		  
		   for (var b=0; b< vp.boneCount; b++) {
			    this.matrices[i].mul2(_invMatrix, this.bones[i].getWorldTransform()); // world space -> rootNode space
				this.matrices[i].mul2(this.matrices[i], vp.pose[b]); // rootNode space -> bind space
                i++
		   }
	   }
    },

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.

4 participants