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

added setDrawRange to Object3D #9929

Closed
wants to merge 1 commit into from
Closed

Conversation

spite
Copy link
Contributor

@spite spite commented Oct 24, 2016

Similar to #9662, there are attributes that are assigned at the geometry level that should be at the instance (mesh) level. The onBeforeRender and onAfterRender callback are an extreme solution to something that, as the uniforms defined at mesh level, should be managed internally by the engine.

It's assigned to Object3D since that's the base object that the method accepts, but if there are concerns for overhead, there can be an explicit check that the object supports geometry ranges.

@mrdoob
Copy link
Owner

mrdoob commented Nov 29, 2016

@WestLangley what do you think about this idea?

@WestLangley
Copy link
Collaborator

@WestLangley what do you think about this idea?

Gee, I am having a hard time understanding why one would want to do that, quite frankly.

@spite
Copy link
Contributor Author

spite commented Nov 29, 2016

Think of it as having properties that right now are in the geometry and material moved into the object. If you have 100 different objects, each with a different material.color, right now every mesh uses the color defined in the material, so -without getting into vertexColors, custom attributes, etc.- you would need 100 different materials, each with different color. If the color property were in the mesh, and assigned to the material/program when drawing, you'd only need one. This is the same thing but for data related to geometry and data buffers.

The original PR is buggy, btw. It would need a better method to mix properties.

@WestLangley
Copy link
Collaborator

@spite I see. So, in other words, you are proposing moving the uniforms out of the material -- along with perhaps some geometry properties -- into a "renderable object".

So we would have Object3D --> RenderableObject -> Mesh/Points/Line/LineSegments/Sprite.

@spite
Copy link
Contributor Author

spite commented Nov 29, 2016

Rather than moving, having a property in the instance that, if defined, supersedes the one defined in the material or the geometry.

@WestLangley
Copy link
Collaborator

Rather than moving, having a property in the instance that, if defined, supersedes the one defined in the material or the geometry.

Oh, yes. That suggestion has come up before -- and likely in similar suggestions in other posts, too.

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 9, 2019

Regardless of whether we want to implement this change or not, we can not merge the PR in its current state. At least updating three.legacy.js is necessary in order to avoid breakage. There are also some examples using BufferGeometry.drawRange (plus docs and TS files).

But do we really want to move drawRange to Object3D? Doing this would imply a 3D object has something drawable (a geometry) which is not always true. As long as there is no some sort of RenderableObject (like mentioned by @WestLangley here), I would keep drawRange at BufferGeometry.

@WestLangley
Copy link
Collaborator

But do we really want to move drawRange to Object3D?

No. I think the proposal in this thread can more-accurately be stated by #9929 (comment).

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 10, 2019

Okay, I see. The idea is to keep BufferGeometry.drawRange but also add this property to Object3D. Um 🤔

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 10, 2019

I don't know...if we start with a geometry property, users could also expect material properties to be added to Object3D. TBH, I have the bad feeling that would bloat the API and make everything more confusing...

@WestLangley
Copy link
Collaborator

The idea is to keep BufferGeometry.drawRange but also add this property to Object3D

@Mugen87 No, the suggestion is not to add properties to Object3D. It is to allow .userData properties to override material or geometry properties if they both exist.

We tried to support something similar to this with .onBeforeRender().

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 10, 2019

It is to allow .userData properties to override material or geometry properties if they both exist.

I read the word .userData for the first time in this thread...

@WestLangley
Copy link
Collaborator

@Mugen87 You are correct. I should have said "instance properties, such as userData".

I inferred -- perhaps incorrectly -- that the OP changed his original proposal as a result of the discussion.

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 22, 2021

Closing. The workaround for this particular use case is the usage of onBeforeRender() like demonstrated here: https://jsfiddle.net/khrojvtd/

Rather than moving, having a property in the instance that, if defined, supersedes the one defined in the material or the geometry.

With the new material system, custom nodes can be used to supersede material or geometry properties. There is already a WebGPU example that demonstrates this approach: https://threejs.org/examples/#webgpu_instance_uniform

This approach will also be possible with WebGLRenderer, see #21305 (comment). And it is definitely more scalable than assuming users can add all kinds of properties to Object3D.

@Mugen87 Mugen87 closed this Mar 22, 2021
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