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

WebGLRenderer: Enable rendering of geometries with just an index. #18044

Merged
merged 1 commit into from Dec 20, 2019

Conversation

bunnybones1
Copy link
Contributor

This recently added graceful-failcheck, from #17982, assumes an attribute called "position". One of our projects loads custom geometry that compute position in the vertex shader based on other attributes. With this small change/rollback, this graceful-failcheck should still work based simply on detecting a lack of index, which all standard and custom geometries need.

@bunnybones1
Copy link
Contributor Author

bunnybones1 commented Dec 2, 2019

I just fixed a leftover reference to position attribute in graceful-failcheck. But now re-reading the code, I'm starting to second-guess the solution. I didn't realize we could render geometry without an index, or can we? @Mugen87
Is there a better way to keep the early-out check but not assume an attribute named "position"?

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 3, 2019

I didn't realize we could render geometry without an index, or can we? @Mugen87

Yes, an index is not necessary.

Is there a better way to keep the early-out check but not assume an attribute named "position"?

Not sure about this. But using no position attribute is not something I would regard as a valid use case in context of WebGLRenderer. Wireframe rendering and bounding volume computation do not work without vertices. And that means you have no correct view frustum culling, no raycasting support etc...

So I do not vote to merge this PR. I would rather recommend to fork the renderer and then make your custom changes.

@bunnybones1
Copy link
Contributor Author

bunnybones1 commented Dec 3, 2019

That makes sense for standard usage, but this actually makes a lot of custom effects not render. For example: We have a dynamic 9-slice UI geometry that has a couple attributes called sizeMask and sizeOffset, and a uniform called size. The vertex shader computes position like this:

position = sizeOffset + sizeMask * size

An attribute called position would make no sense here, so a lot of valid effects would now be considered invalid unless they get a position attribute hacked into them. In my opinion, debugging something like this would be painful for most users.

Also, for index, I have a custom effect using a Point geometry that also stopped rendering until I added a simple index of [0..total].

Both of these bugs required me to breakpoint down into this new graceful fail-check to diagnose (after checking layers, valid matrices, culling, etc.).

Right now the experience is that updating threejs to r111 makes valid effects invisible, and very hard to debug. Maybe some other kind of detection is possible. How about detecting any attribute length instead of position? That should satisfy every need.

Also, regarding boundingBoxes, in cases like these, the error we get is very obvious and helpful, and it just reminds us to provide a custom bounding method as part of the effect class. Very easy to debug.

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 3, 2019

Sorry, but if we merge this PR, we have to reopen other issues again that are also valid complains. The current status is definitely more consistent regarding to the rest of the API. So my opinion still stands. Let's see how others evaluate this issue.

@bunnybones1
Copy link
Contributor Author

Sure thing, if we're the only team with this complaint we'll manage it in a fork :) but I know a few WebGL devs who regularly don't use position. Let's give it a week or so to see if anyone else reports this issue.

@mrdoob
Copy link
Owner

mrdoob commented Dec 3, 2019

One of our projects loads custom geometry that compute position in the vertex shader based on other attributes.

Can you share a example/demo of that?

@bunnybones1
Copy link
Contributor Author

bunnybones1 commented Dec 3, 2019

Here's an example with r110:
https://horizon-games.github.io/threejs-nineslice-demo/
The most relevant parts are the Vertex shader:
https://github.com/horizon-games/threejs-nineslice-demo/blob/master/src/materials/NineSliceMaterial/vert.glsl
and the Geometry:
https://github.com/horizon-games/threejs-nineslice-demo/blob/master/src/meshes/geometry/NineSliceGeometry.ts
I've extracted it from one of our projects, but there are other similar use-cases, where position is not an attribute, but a result of some other attributes/uniforms.
In short, RawShaderMaterials are quietly not rendering if they don't follow the secret requirement.

@WestLangley
Copy link
Collaborator

I agree with @bunnybones1.

Using a geometry without a position attribute (where positions are set in the vertex shader) is something that has come up before and -- if I remember correctly -- has been supported. GPGPU does not necessarily require a position attribute. I assume we would want to support that, too, with RawShaderMaterial.

I would not expect users in these cases to care about bounding boxes or raycasting on the CPU.

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 3, 2019

The GPGPU examples work without issues. A demo in this context would be helpful to further validate this use case.

https://raw.githack.com/mrdoob/three.js/dev/examples/index.html?q=gpgpu

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 3, 2019

In any event, you need at least an index for rendering if no position attribute is present. So maybe you can rewrite your PR like so:

  • If no index is provided, the renderer tests for the position attribute. If no such attribute is present, there is nothing to render -> early out.
  • If an index is provided, check if its count property is not 0. If so, early out. The test for position is skipped in this case.

@bunnybones1
Copy link
Contributor Author

@Mugen87 Ok, I made the modification. It works well for us. I can't imagine many use-cases when we'll create a custom effect without an index OR positions. Thanks for your suggestion.

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 18, 2019

Can you please undo the changes to package-lock.json?

@Mugen87 Mugen87 changed the title Fix regression on geometries with custom attributes WebGLRenderer: Enable rendering of geometries with just an index. Dec 18, 2019
@bunnybones1
Copy link
Contributor Author

I fixed package-lock.json. Sorry about that!

@mrdoob mrdoob added this to the r112 milestone Dec 20, 2019
@mrdoob mrdoob merged commit 93f5836 into mrdoob:dev Dec 20, 2019
@mrdoob
Copy link
Owner

mrdoob commented Dec 20, 2019

Thanks!

@elalish
Copy link
Contributor

elalish commented Dec 20, 2019

One of our projects loads custom geometry that compute position in the vertex shader based on other attributes.

Can you share a example/demo of that?

This was the startup I was at a couple years ago: https://www.omnivor.io/ I built this volumetric video renderer in much the same way. In this case I used a video texture to drive positions in the vertex shader.

@mrdoob
Copy link
Owner

mrdoob commented Dec 20, 2019

@elalish
Copy link
Contributor

elalish commented Dec 20, 2019

How do you like my compression format? Sorry, I patented it (though not owned by me).

@elalish
Copy link
Contributor

elalish commented Dec 20, 2019

Also looks like they haven't updated their website since I left :(

@mrdoob
Copy link
Owner

mrdoob commented Dec 20, 2019

Ah.. Patents...

@brianpeiris
Copy link
Contributor

For future reference, I just ran into this in Hubs as well while upgrading from r105 to r111 (we can't update to r112 yet). We've implemented an efficient UI sprite system that works off spritesheets, batching them into a single BufferGeometry with various custom attributes, a custom RawShaderMaterial, and custom raycasting. Our geometry did have vertices and an index, except we just happen to use "a_vertices" as our attribute name, instead of "position". It was a tough one to debug! But I eventually realized that #17982 was the cause (thank goodness for the release notes).

If it is the case that the "position" attribute is treated specially in the three.js render path, perhaps it ought to be explicitly documented in the BufferGeometry docs. Although, as far as I can tell, that isn't really the case, except for #17982 and ImmediateRenderObjects.

I think we can work around this for now in Hubs, but thanks for fixing it in r112, and for this discussion.

@mrdoob
Copy link
Owner

mrdoob commented Jan 8, 2020

Out of curiosity, why you can't update to r112 yet? Because of the drawMode change?

@arpu
Copy link

arpu commented Jan 8, 2020

i cannot update to r112 on vrland because of removing webvr in r112 (in occulusBrowser i need webvr for linkTraversal and high refresh rate on oculusgo) linkTraversal will come with https://github.com/immersive-web/navigation

@mrdoob
Copy link
Owner

mrdoob commented Jan 8, 2020

Ah, I see. Understandable.

@brianpeiris
Copy link
Contributor

We're stuck on r111 since the latest version of a-frame is also on r111 and also because Firefox and Firefox Reality don't support WebXR yet. In theory we could use the webxr polyfill if a-frame happened to be compatible with r112, but I'm trying to avoid additional complications for now. The upgrade is tricky enough as it since, since we need to merge into our customized three.js fork.

@sjpt
Copy link

sjpt commented May 23, 2020

In any event, you need at least an index for rendering if no position attribute is present.

That is the case with three.js as currently implemented, but it is not logically necessary. Where the shader establishes position from gl_VertexID all you need is to know the number of vertices; which is already provided for by geometry.drawRange.

The only situation where you can't sensibly draw is where there is no index, no vertex attributes, and drawRange.count is Infinity. (eg rendering without even an index)

@sjpt
Copy link

sjpt commented May 25, 2020

I have raised a couple of related issues that may be of interest on this thread. I'd ask anyone interested in procedural rendering to comment on those threads, please.

There is a breaking change in 117 that means that if a position attribute is provided to the geometry but not used in the shader it is ignored and not used to contribute to the count. Procedural geometry that relied on such a dummy attribute no longer renders. #19430

I have submitted a pull request that both resolves this issue, and also permits geometry backed by a procedural shader to work with no attributes or indices.
#19440

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.

None yet

8 participants