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

LineSegmentsGeometry: Calling "setPositions" twice does not update the lines correctly #21488

Open
gkjohnson opened this issue Mar 20, 2021 · 7 comments

Comments

@gkjohnson
Copy link
Collaborator

Describe the bug

Calling LineSegmentsGeometry.setPositions a second time with more line segments still only renders the amount of line segments specified in the first call. Everything works as expected if you call "dispose" before calling "setPositions" again.

I'm not exactly sure why it's happening. Perhaps it's an issue related to instanced geometry rendering?

To Reproduce

Steps to reproduce the behavior:

  1. Create a LineSegments2 object
  2. Call line.geometry.setPositions with 1 line segment
  3. See that it renders 1 line
  4. 1 second later call line.geometry.setPositions with 2 line segments
  5. See that it only renders the first of the two lines

Code

line = new LineSegments2();
line.geometry.setPositions( [ 0, -0.5, 0, 0, 0.5, 0 ] );

setTimeout( () => {

  // uncomment to display both lines
  // line.geometry.dispose();
  line.geometry.setPositions( [
    0, -0.5, 0, -0.5, 0.5, 0,
    0, 0, 0, 0.5, 0, 0,
  ] );

}, 1000 );

Live example

https://jsfiddle.net/rhjeL01n/

Expected behavior

The correct amount of lines are rendered after calling "setPositions".

Screenshots

If applicable, add screenshots to help explain your problem (drag and drop the image).

Platform:

  • Device: Desktop
  • OS: Windows
  • Browser: Chrome
  • Three.js version: latest (r126)
@WestLangley
Copy link
Collaborator

three.js has not supported the resizing of buffers. See https://threejs.org/docs/#manual/en/introduction/How-to-update-things.

@gkjohnson
Copy link
Collaborator Author

gkjohnson commented Mar 20, 2021

three.js has not supported the resizing of buffers

A new buffer is being created when you call setPositions, though:

var instanceBuffer = new InstancedInterleavedBuffer( lineSegments, 6, 1 ); // xyz, xyz
this.setAttribute( 'instanceStart', new InterleavedBufferAttribute( instanceBuffer, 3, 0 ) ); // xyz
this.setAttribute( 'instanceEnd', new InterleavedBufferAttribute( instanceBuffer, 3, 3 ) ); // xyz

Also if you perform the same test with a triangular mesh (create a position buffer with 1 triangle, then 1 second later replace it with a buffer with 2 triangles) then it works. I though resizing buffers meant updating the array within which is why the setArray function had been removed.

@WestLangley
Copy link
Collaborator

A new buffer is being created when you call setPositions, though:

Granted. But it is an example, and the method was only intended to be called once.

I think your issue deserves reconsideration, however. Thanks for bringing it up. :)

/ping @Mugen87

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 21, 2021

I think we've discussed the same issue in #20933 already.

TBH, this is not the intended usage of the API.

@gkjohnson
Copy link
Collaborator Author

I think we've discussed the same issue in #20933 already.

The behavior is a bit different here. Looking deeper it looks like this is due to InstancedBufferGeometry._maxInstanceCount being set on first render and always being reused instead of taking the minimum of the current set of attributes, which seems more fixable at least when setting InstancedBufferGeometry.needsUpdate = true.

TBH, this is not the intended usage of the API.

Yes apologies I'm reminded you mentioned that in the previous issue. Is this definitive now? Is it the position of the project that setting or removing BufferAttributes on BufferGeometry between renders without calling dispose is not allowed? If so it would be nice to update the docs with clarification and maybe even log warnings if the user does this. It works 99% of the time so it's easy to misunderstand the expectation of the library. Afaik InstancedBufferGeometry and wireframe materials are the only cases where it actually breaks. The phrasing I see frequently of "resizing" a buffer sounds like creating a new array for an existing buffer and not creating a new buffer that is set on a BufferGeometry.

@WestLangley if the _maxInstanceCount issue will not be fixed are you open to adding a function that will update the existing instanceStart and instanceEnd buffers and draw ranges for LineSegments2 so thick lines can be changed in place more easily?

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 22, 2021

Is it the position of the project that setting or removing BufferAttributes on BufferGeometry between renders without calling dispose is not allowed?

You should modify the existing buffers. Not overwrite them (with new sizes). Nowhere in the examples is your pattern demonstrated.

If you need to change the size, dispose the geometry (since you can't dispose buffer attributes) and create a new one. Feel free to clarify this in the documentation.

@WestLangley
Copy link
Collaborator

@gkjohnson

  1. Could you create a sufficiently-large buffer and then set InstancedBufferGeometry.instanceCount? That would be one way to resize.

  2. Yes, _maxInstanceCount is a bit hacky. We knew that when it was added.

  3. I agree it is hard to update the values in the buffers because the indexing is complex. I would welcome API improvements in that regard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants