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

vertices need update not working - r72 #7179

Closed
NicolasRannou opened this issue Sep 17, 2015 · 30 comments
Closed

vertices need update not working - r72 #7179

NicolasRannou opened this issue Sep 17, 2015 · 30 comments
Labels

Comments

@NicolasRannou
Copy link
Contributor

With r71 I could update vertices of my BoxGeometry for as below:

  var vertices = [];
  vertices.push(new THREE.Vector3(0.5, 0.5, 0.5));
  vertices.push(new THREE.Vector3(0.5, 0.5, -0.5));
  vertices.push(new THREE.Vector3(0.5, -0.5, 0.5));
  vertices.push(new THREE.Vector3(0.5, -0.5, -0.5));
  vertices.push(new THREE.Vector3(-0.5, 0.5, -0.5));
  vertices.push(new THREE.Vector3(-0.5, 0.5, 0.5));
  vertices.push(new THREE.Vector3(-0.5, -0.5, -0.5));
  vertices.push(new THREE.Vector3(-0.5, -0.5, 0.5));

  this._mesh.geometry.vertices = vertices;
  this._mesh.geometry.applyMatrix(
      new THREE.Matrix4().makeTranslation(
        dataCoordinates.x,
        dataCoordinates.y,
        dataCoordinates.z));
  this._mesh.geometry.verticesNeedUpdate = true;

It doesn't work anymore on r72.

It doesn't raise any error.

Is it supposed to work? If not, which is the current best practice to update vertices of a geometry?

@mrdoob
Copy link
Owner

mrdoob commented Sep 17, 2015

Do you mind creating a working jsfiddle that we can work with?

@NicolasRannou
Copy link
Contributor Author

Here you go:
r71 (working): http://codepen.io/nicolasrannou/pen/xwVJRr
r72 (NOT working): http://codepen.io/nicolasrannou/pen/KdzBBz

(in the JS settings of codepen, you can change the target THREE.JS build)

Thanks!

@WestLangley
Copy link
Collaborator

@NicolasRannou You are creating a new array of vertices each frame. That is not a very good practice. Instead, use copy and replace existing values.

@mrdoob The problem is that he is creating a new array of vertices each frame and DirectGeometry.vertices is assigned Geometry.vertices.

Maybe we are going to have to make Geometry.vertices -- and other select properties -- immutable.

@NicolasRannou
Copy link
Contributor Author

@WestLangley Thanks - just calling "set" on every single vertex of the vertices array seems to work.

r72 (working): http://codepen.io/nicolasrannou/pen/zvqyrj

@mrdoob
Copy link
Owner

mrdoob commented Sep 19, 2015

@mrdoob The problem is that he is creating a new array of vertices each frame and DirectGeometry.vertices is assigned Geometry.vertices.

Uhm... but I think it should still work...?
https://github.com/mrdoob/three.js/blob/dev/src/core/DirectGeometry.js#L110

@mrdoob mrdoob added the Bug label Sep 19, 2015
@WestLangley
Copy link
Collaborator

@mrdoob I believe the problem is here:

https://github.com/mrdoob/three.js/blob/dev/src/core/BufferGeometry.js#L368

geometry.__directGeometry is not being updated.

@tforgione
Copy link
Contributor

Any news on this ? I have the same problem here.

@WestLangley
Copy link
Collaborator

@tforgione Replace vertex values, do not reassign them.

@tforgione
Copy link
Contributor

Replacing vertex values whould make me change a lot of stuff, I just wanted to know if there will be a fix soon, or if I should stay with the r71.

@WestLangley
Copy link
Collaborator

@tforgione How this will be resolved has not been decided. But it will not occur until r.73.

@tforgione
Copy link
Contributor

Ok, thanks for the information !

@dimarudol
Copy link
Contributor

uvsNeedUpdate flag does not work in r72. Probably it is related to this bug.

Simple example of the problem:
r71 version: https://jsfiddle.net/7hxz6h0c/ (green and red trias, correct)
r72 version: https://jsfiddle.net/a0q4ujfo/ (green and green trias, wrong)

@WestLangley
Copy link
Collaborator

@dimarudol

Try adding this code block to BufferGeometry.updateFromObject():

if ( geometry.uvsNeedUpdate ) {

    var attribute = this.attributes.uv;

    if ( attribute !== undefined ) {

        attribute.copyVector2sArray( geometry.uvs );
        attribute.needsUpdate = true;

    }

    geometry.uvsNeedUpdate = false;

}

three.js r.72 master

The lineDistance logic needs to be looked at, also.

@mrdoob
Copy link
Owner

mrdoob commented Sep 29, 2015

@dimarudol fixed. Thanks @WestLangley!

@WestLangley
Copy link
Collaborator

@mrdoob I'm thinking the uv fix should go in master, too.... Hmmm. Maybe it is time to begin a new release renumbering: 0.72.1.

@mrdoob
Copy link
Owner

mrdoob commented Sep 29, 2015

@mrdoob I'm thinking the uv fix should go in master, too.... Hmmm. Maybe it is time to being a new release renumbering: 0.72.1.

It's not a super critical bug... r73 will be out in two weeks anyway 😊

@WestLangley
Copy link
Collaborator

OK, well we still have the main issue of this bug report: you can't reassign the vertices array after the first render.

@mrdoob
Copy link
Owner

mrdoob commented Sep 30, 2015

Seems like a workaround is to do delete box.geometry.__directGeometry;. Maybe we can do add a new flag that recreated the DirectGeometry.

@airycanon
Copy link

@mrdoob When I deleted geometry.__directGeometry,the geometry did update,but the rendering performance dropped down,is there a better way to make the geometry update?

@jniac
Copy link

jniac commented May 6, 2016

Could this problem occur with r75 ?
It seems i got the same problem, when i check via the console the faceVertexUvs has changed but it still display the uv used at the creation of the mesh.
Should i produce a jsfiddle ?

@WestLangley
Copy link
Collaborator

@jniac If you believe you have identified a bug, please see "How to report a bug" in the guidelines.

@jniac
Copy link

jniac commented May 6, 2016

well, maybe i should, i'm quite a noob on github, i dare not open a new discussion
here's the jsfiddle : https://jsfiddle.net/1opm3681/14/
using ThreeJS r75

if i do understand
geometry.uvsNeedUpdate = true
should be enough to allow the update of the uv, but it doesn't seems ti work.

@WestLangley
Copy link
Collaborator

@jniac Please read my comments above. Change UV values using Vector2.set() -- do not replace the faceVertexUvs array.

@jniac
Copy link

jniac commented May 6, 2016

Thank you !
I was thinking of something like that. Nonetheless i thought that uvsNeedUpdate would allow to refresh the buffer / cache. I believed that uvsNeedUpdate was intended to dispense to update the uv points (by using set). Anyway, thank you.

here is the JSFiddle using Vector2.copy :
https://jsfiddle.net/1opm3681/16/
may it help someone

@duhaime
Copy link
Contributor

duhaime commented Nov 25, 2017

Thank you so much @WestLangley! It took me nearly all day to identify the fact that this was cursing me, but I'm glad to see your fix.

@WestLangley
Copy link
Collaborator

#7179 (comment)

If the decision is to not support the reassignment of any attribute, and require copy() or set() instead, then this issue can be closed.

@duhaime
Copy link
Contributor

duhaime commented Nov 25, 2017

@WestLangley If your last statement is the intended direction, would it be possible to update the documentation on threejs.org? The section on How to update things reads:

The following flags control updating of various geometry attributes. Set flags only for attributes that you need to update, updates are costly. Once buffers change, these flags reset automatically back to false:

var geometry = new THREE.Geometry();
geometry.verticesNeedUpdate = true;
geometry.elementsNeedUpdate = true;
geometry.morphTargetsNeedUpdate = true;
geometry.uvsNeedUpdate = true;
geometry.normalsNeedUpdate = true;
geometry.colorsNeedUpdate = true;
geometry.tangentsNeedUpdate = true;

There should be some note there that one can use copy() or set() but that one can't just assign these booleans to true.

For those accustomed to older Three.js versions, it might be helpful to add an optional warning that springs if one attempts to mutate one of these values directly. The warning could be disabled in the constructor for those who don't need them--that would be very helpful to those who would otherwise end up here or reconsulting the docs or migration guide...

@zwcloud
Copy link

zwcloud commented May 14, 2018

Is this open issue still not fixed?

@mrdoob
Copy link
Owner

mrdoob commented May 21, 2018

@zwcloud can you share the code that is causing you problems?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 3, 2019

If the decision is to not support the reassignment of any attribute, and require copy() or set() instead, then this issue can be closed.

I vote to consider reassignment as an invalid operation and close the issue.

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

No branches or pull requests

10 participants