Hardware Instancing fixes / sample (#1846) #1856

mvaligursky · 2020-02-05T09:36:44Z

No description provided.

- fixes related to device.draw trying to render without instancing when single instance is requested, and also handling of 0 instancing by skipping rendering - hardware-instancing.html - simple engine test to render few cubes using instancing

mvaligursky · 2020-02-05T09:38:06Z

I have more complex Editor sample for hardware instancing as well.

mvaligursky · 2020-02-05T10:07:38Z

Note - I’ve just noticed the issue with culling - bounding box of original entity/mesh is used for culling. I need to disable culling on instanced objects.

willeastcott · 2020-02-05T11:42:29Z

examples/graphics/hardware-instancing.html

+
+        var pos = new pc.Vec3();
+        var rot = new pc.Quat();
+        var scl = pc.Vec3.ONE.clone().scale(0.1);


Just do var scl = new pc.Vec3(0.1, 0.1, 0.1);. Simpler, faster, less characters! 😄

willeastcott · 2020-02-05T11:43:01Z

examples/graphics/hardware-instancing.html

+        var box = new pc.Entity();
+        box.addComponent("model", {
+            material: material,
+            type: "box",


Remove trailing comma.

willeastcott · 2020-02-05T11:45:12Z

examples/graphics/hardware-instancing.html

+        material.onUpdateShader = function(options) {
+            options.useInstancing = true;
+            return options;
+        };


I gotta say - this is a bit ugly from an API use perspective. For a casual reader, it's not obvious what's going on here. Is there not an option to either do:

var material = new pc.StandardMaterial(); material.instancing = true;

Or, even better, to set the useInstancing option internally since we can see the mesh instance that the material is assigned to has a pc.InstanceData object assigned to it?

willeastcott · 2020-02-05T11:55:38Z

src/graphics/vertex-format.js

+     * @description Returns description of vertex format which is used by {@link pc.VertexFormat} to
+     * construct a vertex format used for hardware geometry instancing. This represents 16 float values
+     * which represent a {@link pc.Mat4}
+     */


I think the naming of getInstancedStreamDefaultVertexFormat is nice and clear, but overly verbose. Why don't you create an instanced vertex format (type: pc.VertexFormat) on pc.GraphicsDevice creation maybe, and make it accessible at pc.VertexFormat.defaultInstancingFormat (use Object.defineProperty with just a get function to make it read-only)? Also, notice how I'm matching defaultInstancingFormat with naming of pc.InstancingData.

willeastcott · 2020-02-05T11:59:24Z

src/scene/mesh.js

            }
+        },
+
+        setVertexBuffer: function(vb) {


I'd probably recommend just having pc.InstancingData#vertexBuffer as a property (with getter/setter). Maybe then we could get rid of the update function. On a set, just also do what's in update? We'll need to also make pc.InstancingData public API.

- simplified engine interface - disabled culling for instanced meshes - improved documentation

…per for it

willeastcott · 2020-02-07T12:30:26Z

examples/graphics/hardware-instancing.html

+        var vertexBuffer = new pc.VertexBuffer(this.app.graphicsDevice, pc.VertexFormat.defaultInstancingFormat, instanceCount, pc.BUFFER_STATIC, matrices);
+
+        // initialise instancing using the vertex buffer on meshInstance of the created box
+        var boxMeshInst = box.model.model.meshInstances[0];    


Actually, you can access the meshInstances array on the pc.ModelComponent, so you can do the slightly simpler:

var boxMeshInst = box.model.meshInstances[0];

willeastcott · 2020-02-07T12:35:58Z

src/scene/mesh.js

+      * Pass null to turn off hardware instancing.
+      */
+    Object.assign(MeshInstance.prototype, {
+        setInstancing: function (vertexBuffer) {


Can you define this in the previous Object.assign(MeshInstance.prototype, { block?

willeastcott · 2020-02-07T12:36:34Z

src/scene/mesh.js

+     * @type {number}
+     * @description Number of instances when using hardware instancing to render the mesh.
+     */
+    Object.defineProperty(MeshInstance.prototype, 'instancingCount', {


Keep all pc.MeshInstance API together. 😄 Move up?

guycalledfrank · 2020-02-07T13:08:22Z

Just saying - if I had time, I would implement instancing on top of Batch Groups, because it is related and also easy to grasp/use. Currently we have static (attach all meshes) and dynamic (attach and skin all meshes) batching. Now let's say we mark a group as "instanced" - engine would go over meshes, find cloned meshInstances, generate static per-instance vertex buffer, and that's it. We can also add some method to update this buffer.

mvaligursky · 2020-02-07T13:15:06Z

agreed, it's something I'm considering adding as well. This PR is basic functionality / examples allowing users to use it themselves, and we should integrate it with batching.

Even though I'd probably like to have batching dynamic .. current one has limitations - especially if you need to preserve sorting.

guycalledfrank · 2020-02-07T13:29:51Z

I tried to add completely dynamic instancing before, which would scan meshInstances to be rendered and do a single instanced draw instead - it wasn't worth it, scanning through lots of objects and updating/creating buffers at runtime was too costly. This time I'd recommend generating preferably static intstancing groups and cull them with a combined AABB.

Updating buffer data to keep correct sorting can be an option - just don't recreate buffers.

ellthompson · 2020-02-07T14:23:35Z

examples/graphics/hardware-instancing.html

+
+        app.scene.ambientLight = new pc.Color(0.2, 0.2, 0.2);
+
+        // Create an Entity with a point light component and a sphere model component.


Doesn't look like there's a sphere model component used here, is this an outdated comment?

willeastcott · 2020-02-07T15:22:21Z

src/graphics/device.js

+    /**
+     * @readonly
+     * @name pc.GraphicsDevice#extInstancing
+     * @type {boolean}


Actually, this isn't necessarily a boolean. For WebGL 2 it's a boolean. For WebGL 1, it's an object (see MDN). This should probably be tidied up in the engine code at some point. BTW, we don't document the other pc.GraphicsDevice#extXYZ properties. Maybe we could introduce something like:

this.supportsInstancing = !!this.extInstancing;

There's already precedent for that with supportsMsaa and supportsStencil.

* master: (31 commits) WebXR Input Sources (playcanvas#1873) Simplified constants definition in Math classes (playcanvas#1876) Improve batching to handle 8-bit and 32-bit index buffers (playcanvas#1872) Add particle start frame example to browser [FIX] Update WebXR examples to use latest enums (playcanvas#1870) [FIX] Add XRWebGLLayer to externs for Closure compiler (playcanvas#1869) update paths in the particle system start frame example (playcanvas#1868) add a animation start frame variable to the particle system which def… (playcanvas#1864) point cloud example using engine directly, and custom shader changing size and color of points (playcanvas#1867) WebXR Support (playcanvas#1834) Hardware Instancing fixes / sample (playcanvas#1846) (playcanvas#1856) [VERSION] v1.25.0-dev [RELEASE] v1.24.7 Recompiled basis, now works on IE (playcanvas#1865) Small mesh cleanup (playcanvas#1866) [FIX] Revert memory-leak change (playcanvas#1862) [FIX] Flag model as immutable when it's added to a ModelComponent (playcanvas#1861) [FIX] Fix calculation of deprecated pc.MouseEvent#wheel (playcanvas#1859) [DOCS] Adjust Vec3.normalize docs to reflect behavior better (playcanvas#1849) pc.BatchManager.markGroupDirty is now public (playcanvas#1848) ...

mvaligursky requested a review from a team February 5, 2020 09:37

willeastcott reviewed Feb 5, 2020

View reviewed changes

willeastcott requested a review from slimbuck February 5, 2020 12:01

willeastcott assigned mvaligursky Feb 5, 2020

willeastcott added the enhancement label Feb 5, 2020

mvaligursky added 2 commits February 6, 2020 16:17

update to hardware instancing

437e99b

- simplified engine interface - disabled culling for instanced meshes - improved documentation

update API - using vertex buffer directly instead of providing a wrap…

0d41e85

…per for it

willeastcott reviewed Feb 7, 2020

View reviewed changes

willeastcott requested a review from ellthompson February 7, 2020 12:40

tiny changes to keep properties together

258aff1

ellthompson reviewed Feb 7, 2020

View reviewed changes

fixed comment in example

d05e8a0

willeastcott reviewed Feb 7, 2020

View reviewed changes

exposed supportsInstancing instead of extInstancing

68769ff

willeastcott approved these changes Feb 7, 2020

View reviewed changes

Merge branch 'master' into mvaligursky-hardware-instancing

1897ffb

mvaligursky merged commit 5f7c501 into master Feb 7, 2020

mvaligursky deleted the mvaligursky-hardware-instancing branch February 7, 2020 17:14

mvaligursky mentioned this pull request Feb 11, 2020

Instancing broken #900

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hardware Instancing fixes / sample (#1846) #1856

Hardware Instancing fixes / sample (#1846) #1856

Uh oh!

mvaligursky commented Feb 5, 2020

mvaligursky commented Feb 5, 2020

mvaligursky commented Feb 5, 2020

willeastcott Feb 5, 2020

willeastcott Feb 5, 2020

willeastcott Feb 5, 2020

willeastcott Feb 5, 2020

willeastcott Feb 5, 2020

willeastcott Feb 7, 2020

willeastcott Feb 7, 2020

willeastcott Feb 7, 2020 •

edited

Loading

guycalledfrank commented Feb 7, 2020

mvaligursky commented Feb 7, 2020

guycalledfrank commented Feb 7, 2020 •

edited

Loading

ellthompson Feb 7, 2020 •

edited

Loading

willeastcott Feb 7, 2020 •

edited

Loading

Labels

5 participants


		app.scene.ambientLight = new pc.Color(0.2, 0.2, 0.2);

		// Create an Entity with a point light component and a sphere model component.

Hardware Instancing fixes / sample (#1846) #1856

Hardware Instancing fixes / sample (#1846) #1856

Uh oh!

Conversation

mvaligursky commented Feb 5, 2020

mvaligursky commented Feb 5, 2020

mvaligursky commented Feb 5, 2020

willeastcott Feb 5, 2020

Choose a reason for hiding this comment

willeastcott Feb 5, 2020

Choose a reason for hiding this comment

willeastcott Feb 5, 2020

Choose a reason for hiding this comment

willeastcott Feb 5, 2020

Choose a reason for hiding this comment

willeastcott Feb 5, 2020

Choose a reason for hiding this comment

willeastcott Feb 7, 2020

Choose a reason for hiding this comment

willeastcott Feb 7, 2020

Choose a reason for hiding this comment

willeastcott Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

guycalledfrank commented Feb 7, 2020

mvaligursky commented Feb 7, 2020

guycalledfrank commented Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ellthompson Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

willeastcott Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Labels

5 participants

willeastcott Feb 7, 2020 •

edited

Loading

guycalledfrank commented Feb 7, 2020 •

edited

Loading

ellthompson Feb 7, 2020 •

edited

Loading

willeastcott Feb 7, 2020 •

edited

Loading