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: Fix issue 19430. and enable rendering without vertex data. #19451

Closed
wants to merge 1 commit into from

Conversation

sjpt
Copy link

@sjpt sjpt commented May 25, 2020

Allow use of procedural geometry (a) with no index or position attributes, and/or (b) no instance attributes.

@sjpt sjpt mentioned this pull request May 25, 2020
@sjpt
Copy link
Author

sjpt commented May 25, 2020

Note: some discussion on background at #19430
This also resolves the original issue raised there.

@Mugen87 Mugen87 changed the title Clean version of https://github.com/mrdoob/three.js/pull/19440 WebGLRenderer: Enable rendering without vertex data. May 25, 2020
@sjpt
Copy link
Author

sjpt commented May 25, 2020

Original sample at https://jsfiddle.net/sjpt/Lrhjkstz
For a more realistic example of procedural geometry see https://jsfiddle.net/sjpt/hgfsvLzn

@xinaesthete
Copy link

hacky replacement of gui with dat.gui version for second sample above https://jsfiddle.net/kse6vd3g/

@sjpt sjpt changed the title WebGLRenderer: Enable rendering without vertex data. WebGLRenderer: Fix issue 19430. and enable rendering without vertex data. May 28, 2020
@sjpt
Copy link
Author

sjpt commented May 28, 2020

Changed title to reflect that this resolves the change introduced in 117 that broke code that previously worked, as well as allowing new usage.

@duhaime
Copy link
Contributor

duhaime commented Oct 5, 2020

2**64 thanks @sjpt! I just hit this exact problem attempting to upgrade from 110 to 111 and had a heck of a time figuring out what was going on.

It turns out I was not passing an attribute named "position" to my BufferGeometry. I was passing positional information, but under a different buffer name.

In version 110, this was allowed, and I could just set my vertex position in the vertex shader without issue. In version 111, nothing is rendered unless I rename my buffer with position data "position"!

@filonik
Copy link
Contributor

filonik commented Jun 1, 2022

I just submitted an issue (#24165) that got rightfully closed as a duplicate. This looks like a great improvement. Any chance of something like this being adopted? What is still lacking? It is a bit discouraging that the last activity seems 2 years ago...

Edit: I suppose one thing that could be improved is cleaning up logic/control flow. I think it is a bit hard to follow. Essentially, the problem seems to be that we want to compute drawStart and drawEnd, while being fairly defensive about it to prevent indexing outside of the provided buffers. I am not sure if it is necessary to be that defensive, but that's a different story. Regardless, in my mind, the logic should simply go as follows (prioritized in the given order):

  1. Initialize drawStart and drawEnd with the values in drawRange.
  2. If a group is specified, update drawStart and drawEnd accordingly.
  3. If the geometry is indexed, clamp drawStart and drawEnd to 0 and index.count (optional).
  4. If the geometry is non-indexed with a position attribute, clamp drawStart and drawEnd to 0 and position.count (optional).

I have labelled the last two as optional, because personally I would generally prefer a warning rather than silently changing the given drawRange under the hood. But I understand if it is not desirable to break the existing behavior.

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 22, 2022

Closing in favor of #24179.

@Mugen87 Mugen87 closed this Oct 22, 2022
Mugen87 pushed a commit that referenced this pull request Oct 29, 2022
* Clean up of #19451.

* Additional check and example.

* Removed temporary inline objects.

* Added screenshot for example.

* Updated draw range in example.

* Added screenshot for example.

* Added description for example.

* Removed old code from example.

* Changed noise function in example.

* Renamed example.

* Prettier example.

* Simplified conditional.

* Fixed _maxInstanceCount undefined.

* Directly initialize drawStart/drawEnd from drawRange.
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

5 participants