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

groups/drawcalls/offsets not working for non MeshFaceMaterial on 72dev #7057

Closed
edankwan opened this issue Aug 27, 2015 · 27 comments
Closed

Comments

@edankwan
Copy link

If the mesh material is not MeshFaceMaterial, it doesn't render separately with groups. Which means I can't render parts of the whole geometry.

https://github.com/mrdoob/three.js/blob/dev/src/renderers/WebGLRenderer.js#L1358-L1399

@edankwan
Copy link
Author

Sorry about that, I guess I will just use MeshFaceMaterial when I need to do so...

@WestLangley WestLangley reopened this Aug 27, 2015
@WestLangley
Copy link
Collaborator

Reopened.

I would hope this would be fixed.

@edankwan
Copy link
Author

@WestLangley Yes, I understand that but I guess it is more important to remove those legacy support. Like offsets/drawcalls/drawCalls.. so messy at the moment in the source codes.

For now the simple workaround is to "follow their standards" like:

    mesh = new THREE.Mesh(
        new THREE.PlaneBufferGeometry(100, 100),
        new THREE.MeshFaceMaterial([
            new THREE.MeshBasicMaterial({color: 0xff0000})
        ])
    );
    mesh.geometry.groups = [{ start: 0, count: 3, materialIndex: 0 }];
    stage3d.scene.add(mesh);

@WestLangley
Copy link
Collaborator

Yes, @mrdoob made a similar suggestion in #6982 (comment) when commenting on my animated line example.

He also acknowledged that the workaround was "weird". I agree.

@mrdoob
Copy link
Owner

mrdoob commented Sep 2, 2015

Yeah. I'm indeed aware of this, but I just can't see a better solution...

Basically, this approach aligns Geometry and BufferGeometry behaviour.

@arose
Copy link
Contributor

arose commented Sep 4, 2015

What about (at least temporarily) adding something like this

} else if ( geometry.groups.length > 0 ) {

    var groups = geometry.groups;

    for ( var i = 0, l = groups.length; i < l; i ++ ) {

        pushRenderItem( object, geometry, material, _vector3.z, groups[ i ] );

    }

at https://github.com/mrdoob/three.js/blob/dev/src/renderers/WebGLRenderer.js#L1363? That way, you don't have to wrap all your materials in THREE.MultiMaterial.

@mrdoob
Copy link
Owner

mrdoob commented Sep 4, 2015

Looks good to me! 😊

@WestLangley
Copy link
Collaborator

Thanks @arose @mrdoob !

@mrdoob
Copy link
Owner

mrdoob commented Sep 4, 2015

Meh... This broke wireframe.
Or rather... made evident that the wireframe solution doesn't work with groups/drawcalls 😕

@mrdoob
Copy link
Owner

mrdoob commented Sep 4, 2015

Part of the problem is that Geometry -> BufferGeometry converts materialIndex to drawcalls/groups. So, with the last change, rendering a cube with a single material results in 6 drawcalls (one per face) instead of just one...

The wireframe problem is related but I hadn't noticed until now...

@mrdoob
Copy link
Owner

mrdoob commented Sep 4, 2015

What about removing drawcalls/groups altogether and do something along these lines:

var geometry = new THREE.SphereBufferGeometry();
var geometries = [
    new THREE.SubGeometry( geometry, materialIndex ).setRange( start, count ),
    new THREE.SubGeometry( geometry, materialIndex ).setRange( start, count )
];
var mesh = new THREE.Mesh( geometries, new THREE.MultiMaterial( [ material1, material2 ] ) );

So, instead of hiding the drawcalls/groups stuff inside buffergeometry we take it out of it and then we can reuse a single geometry across different objects type with different start/counts, etc

Maybe we could even add start and count directly to BufferGeometry and allow passing other geometries as references, so this would work:

var geometry = new THREE.SphereBufferGeometry();
var geometries = [
    new THREE.BufferGeometry( geometry, materialIndex ).setRange( start, count ),
    new THREE.BufferGeometry( geometry, materialIndex ).setRange( start, count )
];
var mesh = new THREE.Mesh( geometries, new THREE.MultiMaterial( [ material1, material2 ] ) );

And this... which I know @WestLangley likes.

var geometry = new THREE.SphereBufferGeometry().setRange( start, count );
var mesh = new THREE.Mesh( geometry, new THREE.MeshBasicMaterial() );

Maybe a more objectified version would be this:

var geometry = new THREE.SphereBufferGeometry();
var geometries = new THREE.GroupGeometry();
geometries.add( geometry, materialIndex, start, count );
geometries.add( geometry, materialIndex, start, count );
var mesh = new THREE.Mesh( geometries, new THREE.MultiMaterial( [ material1, material2 ] ) );

Just when I thought that we were done with this... 😁

The good news is that now that WebGLRenderer has been cleaned up, implementing these things will be pretty easy, in the past we couldn't even reuse a geometry with different materials.

@mrdoob
Copy link
Owner

mrdoob commented Sep 6, 2015

I think this is what I'm going to implement...

Defining a range to draw:

var geometry = new THREE.SphereBufferGeometry();
geometry.setRange( 0, 100 ); // start, count
var mesh = new THREE.Mesh( geometries, new THREE.MeshBasicMaterial() );

When using MultiMaterial, the geometry range is ignore and groups is used instead:

var geometry = new THREE.SphereBufferGeometry();
geometry.addGroup( new THREE.GeometryGroup( 0, 100, 0 ) ); // start, count, materialIndex
geometry.addGroup( new THREE.GeometryGroup( 100, 100, 1 ) );
var mesh = new THREE.Mesh( geometries, new THREE.MultiMaterial( [ material1, material2 ] ) );

I think this solves all the cases I've seen people mentioning. Any better names than setRange()?

I'm also wondering whether we should simplify raycast() and remove all the drawcalls/groups code and only raycast full geometries.

@mrdoob
Copy link
Owner

mrdoob commented Sep 6, 2015

/cc @benaadams @bhouston @tschw

@benaadams
Copy link
Contributor

Would this allow sorting by program for the draws in WebGLRenderer?

@mrdoob
Copy link
Owner

mrdoob commented Sep 6, 2015

Well, we can do that already I think. But what this allows is to be able to load an old Geometry that used MeshFaceMaterial and be able to render in a single drawcall when using a single material, but also render different parts of it with different material. And, at the same time, the new setRange in BufferGeometry allows webgl_buffergeometry_drawcalls to not have to use MultiMaterial (plus it's more intuitive than having to add a GeometryGroup).

@benaadams
Copy link
Contributor

Well, we can do that already I think.

I mean you'll want to sort at the sub geometry level across the scene; rather than the mesh level; otherwise you will be flip flopping programs which is the most expensive gpu state change

@mrdoob
Copy link
Owner

mrdoob commented Sep 6, 2015

Yeah, we do that already 😊
Well, we're sorting by material, not by program yet.

@arose
Copy link
Contributor

arose commented Sep 6, 2015

I like geometry.setRange (may setDrawRange?).

Related: renderInstances (WebGL[Indexed]BufferRenderer) ignores start/length. Is this just not implemented or is there another reason?

I am not too sure about the materialIndex in geometry.addGroup as it is not a property of an geometry, or is it? Also, it would limit the re-usability of the geometry in different meshes. Maybe it is better to add the groups to the mesh, as both relate to geometry and material.

May be create the Mesh as you proposed and then add the mapping between draw ranges and materials to the mesh via an addGroup (addDrawGroup?) method. There could also be an optional parameter in the mesh constructor. Hmm, this raises the question what the default mapping should be when no groups are added.

@edankwan
Copy link
Author

edankwan commented Sep 6, 2015

I agree, Group/drawcalls should belong to the mesh rather than the geometry. It is more flexible to reuse the geometry data

@WestLangley
Copy link
Collaborator

I have made my case multiple times before in other threads, so I will keep it short.

Materials have nothing to do with geometry. It is the responsibility of the mesh/line/pointCloud to define the mapping between geometry and materials. That means groups/drawcalls does not belong in geometry, it belongs inmesh/line/pointCloud. It also means that materialIndex does not belong in geometry. The fact that previously we did it that way for mesh faces is unfortunate.

Note, we had offsets in BufferGeometry, but that was for a different purpose - a purpose we no longer support, apparenty. So there is no need for offsets in geometry now.

I would also use the term drawcalls, because that is what they represent.

@edankwan
Copy link
Author

edankwan commented Sep 6, 2015

Exactly and I agree that the drawcalls or drawCalls was the right name. groups is just too generic and unclear on its purpose.

@arose
Copy link
Contributor

arose commented Sep 6, 2015

@WestLangley Do you agree that for purposes like the webgl_buffergeometry_drawcalls example it is good to set the draw range of a geometry (within the geometry object)? After all, in such cases the data outside the draw range contains only garbage but the attribute buffers are reused to avoid repeated allocation.

@WestLangley
Copy link
Collaborator

@arose In that example, all the position/color data is pushed to the GPU every frame.

linesMesh.geometry.attributes.position.needsUpdate = true;
linesMesh.geometry.attributes.color.needsUpdate = true;
pointCloud.geometry.attributes.position.needsUpdate = true;

It is the API that repopulates the data every frame, so I see no reason why drawcalls should not be uniforms of the renderable object.

@arose
Copy link
Contributor

arose commented Sep 6, 2015

@WestLangley right, data is pushed there every frame, but the number of positions drawn is also changed every frame:

linesMesh.geometry.groups[ 0 ].count = numConnected * 2;

So the attribute data is updated, but not the full data is drawn. This allows the variable number of connections drawn in the demo per frame.

@WestLangley
Copy link
Collaborator

@arose I am aware of that. You can achieve the same even if groups/drawcalls is a property of Line, instead of the geometry itself.

@arose
Copy link
Contributor

arose commented Sep 7, 2015

@WestLangley sorry, didn't mean to imply you weren't aware of that. I guess having groups/drawcalls only as a property of the renderable Mesh/Line objects keeps the code simpler. My perspective was to see Geometry as allocated memory (specifically its attributes) on the GPU and then setting via offset/length what of that memory actually contains any geometry. Anyway, I know agree that having groups/drawcallsin Mesh/Line is sufficient.

Some thoughts on a possible API

var geometry = new THREE.SphereBufferGeometry();
var material1 = new THREE.MeshBasicMaterial();
var material2 = new THREE.MeshBasicMaterial();

// establish a one-to-one mapping between geometry and material, draw full geometry
var mesh = new THREE.Mesh( geometry, material1 );
// set a draw range, clear to remove default drawcall
mesh.clearDrawcalls();
mesh.addDrawcall( 0, 100 );  // offset, length [materialIndex defaults to zero]

// do we need something like MultiMaterial or is a simple list sufficient?
var multiMaterial = new THREE.MultiMaterial( [ material1, material2 ] );

// when given a MultiMaterial, the mesh would render nothing initially...
var mesh = new THREE.Mesh( geometry, multiMaterial );
// ... but would require adding the mappings
mesh.addDrawcall( 0, 100, 0 );  // offset, length, materialIndex
mesh.addDrawcall( 100, 100, 1 );

Here is another, more abstract approach, avoiding the notion of drawcalls altogether.

var geometryView1 = new THREE.GeometryView( geometry, 0, 100 );  // geometry, offset, length
var geometryView2 = new THREE.GeometryView( geometry, 100, 100 );
var mesh1 = new THREE.Mesh( geometryView1, material1 );
var mesh2 = new THREE.Mesh( geometryView2, material2 );
geometryView1.setRange( 200, 100 );  // change the draw range
mesh2.material = material1;  // change the material

There could be helper function to create these meshes, put them in an enclosing THREE.Group and add them to a scene. It would bloat the scene graph a bit, not sure if that would hurt. However, it should allow for a very straightforward implementation. A GeometryView object could be passed around in the WebGLRenderer like a normal Geometry and boundbox and wireframe index could also be added per GeometryView object. The offset/length property need only be accessed for the draw call.

@mrdoob
Copy link
Owner

mrdoob commented Sep 7, 2015

Here is another, more abstract approach, avoiding the notion of drawcalls altogether.

var geometryView1 = new THREE.GeometryView( geometry, 0, 100 );  // geometry, offset, length
var geometryView2 = new THREE.GeometryView( geometry, 100, 100 );
var mesh1 = new THREE.Mesh( geometryView1, material1 );
var mesh2 = new THREE.Mesh( geometryView2, material2 );
geometryView1.setRange( 200, 100 );  // change the draw range
mesh2.material = material1;  // change the material

I like this. I think something like this follows the design of the API.

My problem with drawcall is that is too tied to OpenGL/WebGL. The user is forced to understand what a draw call is. I think something like this intuitively abstracts the concept while still being approachable.

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

No branches or pull requests

5 participants