Skip to content

Conversation

@Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Nov 2, 2018

Fixes the problem mentioned in #12771 (comment)

WebGLGeometries now ensures that the wireframe attribute of a geometry is actually updated when the underlying index buffer changes. Before this fix, the wireframe attribute was created once and never updated.

I've created a fiddle with the fixed build in order to illustrate the new behavior:

https://jsfiddle.net/f2Lommf5/15691/

The PR also does an optimization. Before the fix, getWireframeAttribute() always created an array object right here:

Because of hoisting, it does not matter that the statement was placed after the return. The array was already created at this point. This is now fixed.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Nov 2, 2018

BTW: Updating the position buffer is no problem since updateWireframeAttribute() creates a new set of indices for the wireframe attribute. Even if the position buffer changes, the indices are still valid.

@mrdoob mrdoob added this to the r99 milestone Nov 9, 2018
@mrdoob
Copy link
Owner

mrdoob commented Nov 9, 2018

The PR also does an optimization. Before the fix, getWireframeAttribute() always created an array object right here:

Because of hoisting, it does not matter that the statement was placed after the return. The array was already created at this point. This is now fixed.

Uh oh... Thanks!

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Nov 9, 2018

Because of hoisting, it does not matter that the statement was placed after the return. The array was already created at this point.

Oh sorry, I've made a mistake here. Because of hoisting, the variable indices is just declared and has the value undefined. The array is actually created after the return. So in this context, everything is fine even with the latest builds 😅

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Nov 14, 2018

I think this PR is ready to go now 😊🚀

@mrdoob mrdoob modified the milestones: r99, r100 Nov 29, 2018
@mrdoob mrdoob modified the milestones: r100, r101 Dec 31, 2018
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 3, 2019

/bump @mrdoob

@mrdoob mrdoob modified the milestones: r101, r102 Jan 31, 2019
@mrdoob mrdoob modified the milestones: r102, r103 Feb 28, 2019
@mrdoob mrdoob modified the milestones: r103, r104 Mar 27, 2019
@Mugen87 Mugen87 closed this Apr 9, 2019
@Mugen87 Mugen87 reopened this Jul 10, 2019
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jul 10, 2019

see #10638 (comment)

@Mugen87 Mugen87 removed this from the r104 milestone Jul 13, 2019
@Mugen87 Mugen87 added this to the r107 milestone Jul 14, 2019
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jul 14, 2019

Okay, I've added an additional change to ensure that the respective WebGLBuffer of an obsolete wireframe attribute is actually deleted. This change should prevent an evil memory leak^^

@mrdoob
Copy link
Owner

mrdoob commented Sep 24, 2019

Late thanks! 😇

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.

2 participants