Dirty refactoring?

[alteredq]

I think __dirtyXXX properties definitely proved to be useful and it's time to finally graduate them from hackily exposing WebGLRenderer's internals into something looking more like API.

We could make them lose underscores:

geometry.dirtyVertices = false;
geometry.dirtyMorphTargets = false;
geometry.dirtyElements = false;
geometry.dirtyUvs = false;
geometry.dirtyNormals = false;
geometry.dirtyColors = false;
geometry.dirtyTangents = false;

Or they could be more in line with textures and custom attributes:

geometry.needsUpdateVertices = true;
geometry.needsUpdateElements = true;
geometry.needsUpdateMorphTargets = true;
geometry.needsUpdateUvs = true;
geometry.needsUpdateNormals = true;
geometry.needsUpdateColors = true;
geometry.needsUpdateTangents = true;

A bit different variant:

geometry.verticesNeedUpdate = true;
geometry.elementsNeedUpdate = true;
geometry.morphTargetsNeedUpdate = true;
geometry.uvsNeedUpdate = true;
geometry.normalsNeedUpdate = true;
geometry.colorsNeedUpdate = true;
geometry.tangentsNeedUpdate = true;

What do you think?

[chandlerprall]

I like the needsUpdate prefix, I think it helps categorize them together and would group them in a debug console, API, etc. Maybe using hasDirty* instead?

[zz85]

i kind of like needsUpdate too. however, to really hide the internals, i think methods looks nicer..

geometry.needUpdateVertices();
geometry.needUpdateElements();
geometry.needUpdateMorphTargets();
geometry.needUpdateUvs();
geometry.needUpdateNormals();
geometry.needUpdateColors();
geometry.needUpdateTangents();

[mrdoob]

Uhm... I like this other one instead:

geometry.verticesNeedUpdate = true;
geometry.elementsNeedUpdate = true;
geometry.morphTargetsNeedUpdate = true;
geometry.uvsNeedUpdate = true;
geometry.normalsNeedUpdate = true;
geometry.colorsNeedUpdate = true;
geometry.tangentsNeedUpdate = true;

I think it's better for the API having the properties sorted by object type than by actions. We already have some properties following this pattern:

object.matrixAutoUpdate
object.matrixWorldNeedUpdate
object.rotationAutoUpdate
...

For instance, I just realised shadows don't follow the pattern:

object.castShadow
object.receiveShadow

isn't it cleaner (and easier to scan when debugging) to have it like this instead?

object.shadowCast
object.shadowReceive

[chandlerprall]

There is also computeTangents, computeFaceNormals, etc. Is the style actionProperty for methods and groupSetting for values? (I think *NeedUpdate is fine).

@zz85 I don't think function calls should be used here for a few reasons. First, the stickler in me says they waste a few bytes of memory and a few processor cycles only to set one value. Second, chaining them would look funny imo. object.verticesNeedUpdate().normalsNeedUpdate(); vs object.verticesNeedUpdate = object.normalsNeedUpdate = true. Last, the __dirty* values are specific to the WebGLRenderer and making them methods would most likely mean adding them to the Geometry object itself, regardless of renderer used.

[mrdoob]

There is also computeTangents, computeFaceNormals, etc. Is the style actionProperty for methods and groupSetting for values?

So it seems! Kind of works for me :)

[zz85]

@chandlerprall true, true (no pun intended :), these are valid points i didn't think of.

[valette]

waking this issue up, I began doing such modifications in #1623, just for the __dirtyVertices property.

I can do the same for other properties, too. What do you think?

[zz85]

hmm... looking at this again, I think I would still prefer the needUpdate* grouping - the prefix kinds of tell me that it is a flag much faster and logically than if its in the suffix.

Let's say if I have dynamic geometry, I would think that my mind would have a chain of thought that place needUpdate first, followed by vertices. Perhaps its possible to think of a property, then wonder if there's a flag to that, but I would think that needUpdate first is more natural.

Its like how many would prefix their variables like
_private instead of private_ although i'm pretty sure i've seen some code in the latter style.

If we look at the affected code in WebGLRenderer https://github.com/valette/three.js/commit/9dfa133d0f23b6516897f3afb4db9746e13ea1c2#L5L4046

would be nicer with

if ( geometry.verticesNeedUpdate || geometry.colorsNeedUpdate || object.particlesNeedsSorting || customAttributesDirty )

vs

if ( geometry.needUpdateVertices || geometry.needUpdateColors || object.sortParticles || customAttributesDirty )

[mrdoob]

If you focus on sibling action properties it makes sense the way you're saying.
However, if you look at the bigger picture I don't think it sits that well well.

geometry.vertices = [];
geometry.verticesNeedUpdate = false;
mesh.matrix = [];
mesh.matrixNeedsUpdate = false;

vs

geometry.vertices = [];
geometry.needsUpdateVertices = false;
mesh.matrix = [];
mesh.needsUpdateMatrix = false;

[mrdoob]

6 months later, the change has been done :P
We'll see how it feels.

[zz85]

https://github.com/mrdoob/three.js/wiki/Updates needs update :D

[mrdoob]

Haha :)