mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
102.81k stars 35.38k forks source link

TypeError: this.attributes.position is undefined #7304

Closed makc closed 9 years ago

makc commented 9 years ago

sometimes I don't need position. I can generate one in vertex shader based on cutom attribute / uniforms. but threejs insists that I do need it. can this be relaxed?

WestLangley commented 9 years ago

What code is throwing that error?

What is your use case?

makc commented 9 years ago

I am trying to convert the code that had loads of sprites to THREE.Points, and they had positions re-set on every render as a function of time. so in theory I only need ex-sprite index and time uniform to make positions in the shader, but threejs forces me to use positions any way. not that big of a deal, I can live with half of data unused there.

mrdoob commented 9 years ago

Would be great if you could provide a jsfiddle so we understand what you're trying to do and, at the same time, have some code ready to be supported.

makc commented 9 years ago

It's not that hard to do, e.g. take your points example and modify the shader to calculate positions. Like this. Now you see that position attribute is no longer needed, but if you comment out the line

geometry.addAttribute( 'position', new THREE.BufferAttribute( positions, 3 ) );

3js plays dead.

makc commented 9 years ago

I guess meshes have the same behavior.

WestLangley commented 9 years ago

@makc Your jsdo.it example is suitable for InstancedBufferGeometry, in which case only a single position coordinate is required.

This would require that THREE.Points supportInstancedBufferGeometry, but that is an easy enhancement to WebGLRenderer.

makc commented 9 years ago

InstancedBufferGeometry

I was not watching the source closely, I only look there when something does not work as expected (which is not that often at all :+1: ) and found THREE.Points example by pure chance (correct me if I am wrong, but that's new in r72, no ?). So, is there an example for InstancedBufferGeometry you're talking about?

WestLangley commented 9 years ago

So, is there an example for InstancedBufferGeometry you're talking about?

Yes, but not for Points. I submitted #7316.

benaadams commented 9 years ago

I can generate one in vertex shader based on cutom attribute / uniforms. but threejs insists that I do need it. can this be relaxed?

Can use THREE.RawShaderMaterial doesn't add anything to the glsl other than what you put there.

makc commented 9 years ago

@benaadams but this is javascript TypeError, not glsl shader compilation error.

benaadams commented 9 years ago

The shader says how to interoperate the data; if you don't use RawShaderMaterial then the shader expects an attribute vec3 position so will "play dead" if you don't provide it.

makc commented 9 years ago

@benaadams not true, the shader also has normal and uv, and is still happy even if I do not provide them.

mrdoob commented 9 years ago

Here's position-less version:

http://jsfiddle.net/1v2Lyftu/

By default, WebGLRenderer looks for position.count. However, if the geometry has groups then it'll use that

geometry.addGroup( 0, particles );

as long as it's using MultiMaterial.

particleSystem = new THREE.Points( geometry, new THREE.MultiMaterial( [ shaderMaterial ] ) );

However, we're still getting an error in the console. That's because we're trying to compute the BoundingSphere. We can either create a fake one or just disable it.

particleSystem.frustumCulled = false;

Just for fun, here's one version where we only upload indices (computing the size in the GPU too):

http://jsfiddle.net/b1u8czcw/

😁

WestLangley commented 9 years ago

@mrdoob

I have tested your non-instanced approach with THREE.Points using InstancedBufferGeometry, and the performance appears to be identical for a million particles. I actually used RawShaderMaterial, but it does not matter.

In both cases, you need to set fustumCulled = false.

Now, if we could just figure out to get rid of the

points = new THREE.Points( geometry, new THREE.MultiMaterial( [ material ] ) ); 

requirement... That is not required in the instanced case.

mrdoob commented 9 years ago

Now, if we could just figure out to get rid of the

points = new THREE.Points( geometry, new THREE.MultiMaterial( [ material ] ) ); 

requirement... That is not required in the instanced case.

We could maybe use... geometry.setDrawRange( 0, particles ).

And then WebGLRenderer could have this:

var count;
if ( geometry.drawRange.count < Infinity ) {
    count = geometry.drawRange.count;
} else {
    count = geometry.position.count;
}

Not super pretty, but that would not require MultiMaterial.

WestLangley commented 9 years ago

Not requiring MultiMaterial is definitely a plus.

The core of the problem, IMHO, is we have both geometry.groups and geometry.drawRange. Sometimes one is honored --- sometimes the other.

mrdoob commented 9 years ago

The core of the problem, IMHO, is we have both geometry.groups and geometry.drawRange. Sometimes one is honored --- sometimes the other.

drawRange is used if groups is empty.

mrdoob commented 9 years ago

position-less and groups-less version using dev.

http://jsfiddle.net/g0v9ofxd/

WestLangley commented 9 years ago

@mrdoob Previously, groups was used only if the material was MultiMaterial, in which case drawRange was ignored. Otherwise, groups was ignored, and drawRange was honored.

I see you have made some changes. Do you mind quickly explaining the new intended behavior? Thanks.

mrdoob commented 9 years ago

Sure thing!

It's still the same behaviour, actually...

  1. If we use MultiMaterial then we render groups with the matching material in MultiMaterial.
  2. Otherwise, we treat drawRange as a group and we render the geometry with its material.
    • If drawRange.count is Infinity (default) we look for index.count or position.count.

The last step is new. Previously we were doing min( index.count || position.count, drawRange.count ). It's similar, we just give priority to drawRange.count now. I did the min() initially to avoid cases were the user would set a .count bigger than the .position.count and that's what created this issue.

mrdoob commented 9 years ago

Actually, with the new logic we can simplify this by setting geometry.drawRange to null actually. That way the same code applies to MultiMaterial's groups and non-MultiMaterial's drawRange.

So the main issue was the code I added to make sure the user wasn't setting a count bigger than position.count 😇

WestLangley commented 9 years ago

We are typing at the same time.... Oh, well, what I wanted to say was: In your last fiddle

geometry.setDrawRange( 0, particles );

is required. That is bothersome to me. The default of infinity should be sufficient.

mrdoob commented 9 years ago

If we don't set geometry.setDrawRange( 0, particles ) then we need to add some code to parse all the attributes and see which one has the biggest count... We need to figure out the count somehow.

mrdoob commented 9 years ago

I think having to write geometry.setDrawRange( 0, particles ); is not too much of a burden for the "rare" case of geometries without position. Plus it also gives you control on what parts of the geometry you want to draw.

However, at the moment the user is on their own. If they specify a bigger count than the attributes can handle WebGL will complain.

WestLangley commented 9 years ago

Sorry, this is one of those cases where I have come up with a lot of complaints and not a lot of solutions. Apologies.

I wish we could rename groups to materialRange or something, so users would understand that groups are used for that purpose. Previously, groups were hidden in the renderer. Now they are exposed. Maybe the API could be changed to setMaterialRange( start, count, index ).

WestLangley commented 9 years ago

Then, the intersection of drawRange and materialRange would be honored -- rather than one or the other. Just thinking out loud...

mrdoob commented 9 years ago

The groups naming come from the OBJ format where some parts of the geometry are grouped g even if they're just part of a single object o. This is also a common concept in 3D software as far as I remember. And that's probably the reason I see quite often people that want to color specific parts of their geometry.

mrdoob commented 9 years ago

I'm not sure if it was you, but I remember someone saying that Geometry shouldn't know anything about materials. I thought that was a good point and that's why I kept it separated like that.

I can see you're suggesting to add materialIndex to the group/materialRange. At some point it was like that, but I can't remember why I reverted, I think it complicated things internally.

mrdoob commented 9 years ago

Then, the intersection of drawRange and materialRange would be honored -- rather than one or the other.

Do you mean that you would like to apply drawRange on top of group/materialRange?

Like this?

start = Math.max( drawRange.start, materialRange[ i ].start );
count = Math.min( drawRange.count, materialRange[ i ].count );
WestLangley commented 9 years ago

Yes. Take the intersetion between the materialRange and drawRange intervals. Never draw outside the specified drawRange. That way drawRange is always honored; materialRange is only used by MultiMaterial.

mrdoob commented 9 years ago

Right, I see... Yes, that makes sense. Sorry it took me so long to understand 😅

WestLangley commented 9 years ago

I can see you're suggesting to add materialIndex to the group/materialRange.

groups already has a materialIndex. I was suggesting a property name change from groups to materialRange.

mrdoob commented 9 years ago

I can see you're suggesting to add materialIndex to the group/materialRange.

groups already has a materialIndex. I was suggesting a property name change from groups to materialRange.

Oh... I'm getting old...

Yeah, both groups and materialRange signify the same to me. I prefer groups because it's shorter/less error prone.

makc commented 9 years ago

I'm not sure if it was you, but I remember someone saying that Geometry shouldn't know anything about materials.

I was saying that about raycast failing silently if the mesh had no material, instead of assuming default side-ness.

WestLangley commented 9 years ago

We have to agree to disagree on this one.

I find it confusing. Groups of what?

mrdoob commented 9 years ago

I find it confusing. Groups of what?

Groups of vertices (or indices).

mrdoob commented 9 years ago

I was saying that about raycast failing silently if the mesh had no material, instead of assuming default side-ness.

Right. Yeah... that's why all the groups stuff was removed from raycast. I'm surprised no one has complained about that 😶

WestLangley commented 9 years ago

Groups of vertices (or indices).

Yes, but for what purpose? : - )

There is no need to belabor it anymore, but I think that drawRange and materialRange are better terms than drawRange and groups. Also addMaterialRange() is better than addGroup().

At a minimun, I do think it would be better if drawRange was always honored.

makc commented 9 years ago

There was also an opposite opinion (regarding materials and raycast) from another user - he wanted to have uvs reported as they are in textures, not faces.

mrdoob commented 9 years ago

At a minimun, I do think it would be better if drawRange was always honored.

Done! The internal code/logic also read better now.

Thanks! 😊