vertexColors is now a boolean (since THREE.js r114 / A-Frame 1.1.0) #5250

diarmidmackenzie · 2023-02-22T15:19:29Z

Description:

Since THREE.js r114, vertexColors is a boolean, and THREE.VertexColors is undefined.
mrdoob/three.js#18584

This means the vertexColors property of the material component has been broken since 1.1.0.

Changes proposed:

Remove vertexColors "face" option from the material component interface. Face vertexColoring is not supported on BufferGeometries, hence not usable any more.
Rest of the material component interface remains unchanged (it would be more natural to move vertexColors to a Boolean, but we don't want to break back-compatibility unecessarily).
Change internal code in material component to map to boolean value.
Update UTs & docs
Add warning to assist users upgrading from older versions of A-Frame.

dmarcos · 2023-02-22T21:37:39Z

docs/components/material.md

@@ -70,7 +70,7 @@ depending on the material type applied.
 | shader       | Which material to use. Defaults to the [standard material][standard]. Can be set to the [flat material][flat] or to a registered custom shader material. | standard      |
 | side         | Which sides of the mesh to render. Can be one of `front`, `back`, or `double`.                                                                    | front         |
 | transparent  | Whether material is transparent. Transparent entities are rendered after non-transparent entities.                                                | false         |
-| vertexColors | Whether to use vertex or face colors to shade the material. Can be one of `none`, `vertex`, or `face`.                                            | none          |
+| vertexColors | Whether to use vertex colors to shade the material. Can be one of `none` or `vertex`.                                            | none          |


Should we probably rename to vertexColorsEnabled since it's now a boolean?

Advantage of keeping material="vertexColors:vertex;" is that we don't break anyone's code. And this will be less issues created in this repo when people will update their projects to latest aframe. :)
I would prefer to keep vertexColors instead of vertexColorsEnabled having a name different than the THREE.Material property. Maybe we can keep vertexColors but transform it to boolean, can we keep a warning if we give something different than false/true?. I'm not sure if this is possible.

My intention here was to keep back-compatibility on the schema as much as possible, so I didn't convert to a boolean: its a string that takes 2 values: "none" and "vertex".

We could change the property to a Boolean, but that would mean that applications written against the pre-1.4.1. API that used vertexColors would need updating to use the new API.

It depends whether you want to prioritize back compatibility, or prioritize having a cleaner API going forward?

Another option would be to have 2 properties on the schema:

vertexColors, string, maintained for back-compatibility. Deprecated, and retired at some point in the future

vertexColorsEnabled, boolean. Recommended to use this going forwards.

This last option probably gives best of both worlds - I'll refactor the PR based on this.

@vincentfretin - my reply crossed with yours, so I'd not seen your comments when I wrote mine. I've now updated the PR based on my proposal. Happy to rework again if you can suggest a better way forwards.

Your changes are ok to keep backward compatibility for now and to have a proper warning message to move to the other property, that's what matter I think.

OK, so are you happy with the PR in it's current state?

vertexColors has to remain on the schema, if we are to deliver a bespoke deprecation warning. If we remove it, users will just get a generic warning, which is more likely to lead to support tickets / queries.

if it's left on the schema, I think it should also remain in docs (marked as deprecated).

I would personally go ahead and change the name directly to vertexColorsEnabled and will make a note on the release notes.

I think one consequence of that approach is more broken community components, which is already a problem that IMO is harming A-Frame adoption.

However I care more about having a fix for the bug that I do the back-compatibility, and there probably aren't very many community components that use material.vertexColors - so I'm OK to update this PR as you suggest.

PR now updated - hopefully now mergeable

I think one consequence of that approach is more broken community components, which is already a problem that IMO is harming A-Frame adoption.

I hear you. A-Frame API has been pretty stable for a while. Backward compat issues are mainly a consequence of being on THREE train. Could lock A-Frame to an old THREE version or maintain own fork but thing those are worse and less sustainable solutions. I care about backwards compat but even more so about API usability, consistency and flexibility to evolve.

However I care more about having a fix for the bug that I do the back-compatibility, and there probably aren't very many community components that use material.vertexColors - so I'm OK to update this PR as you suggest.

Thanks so much for the flexibility. Super appreciated. You rock.

vincentfretin · 2023-02-23T09:33:06Z

obj.material.vertexColors = THREE.VertexColors; was still working on aframe 1.3.0 three 0.137.0, THREE.VertexColors was returning 2. obj.material.vertexColors was maybe a boolean already at that time, the check was probably loose so 2 was considered true.
Indeed it doesn't work anymore on aframe 1.4.1, THREE.VertexColors is undefined, you really need to set obj.material.vertexColors = true

diarmidmackenzie · 2023-02-23T09:49:44Z

obj.material.vertexColors = THREE.VertexColors; was still working on aframe 1.3.0 three 0.137.0, THREE.VertexColors was returning 2. obj.material.vertexColors was maybe a boolean already at that time, the check was probably loose so 2 was considered true. Indeed it doesn't work anymore on aframe 1.4.1, THREE.VertexColors is undefined, you really need to set obj.material.vertexColors = true

vertexColors has been a boolean since r114, but I guess THREE.VertexColors wasn't actually removed from the codebase until more recently.

OK - I see what happened - in r114, the constants were moved into Three.Legacy.js, which is presumably the standard practice for deprecations. Then they finally got removed here, which first came into A-Frame at 1.4.0.

…patibility

diarmidmackenzie · 2023-02-23T10:19:48Z

I don't understand the test failure:

FAILED TESTS:
  sound
    ✖ "before each" hook for "creates sound"
      Chrome 110.0.0.0 (Linux x86_64)
    Error: Timeout of 3000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.
        at AScene.<anonymous> (build/commons.js:254942:9)
        at AScene.emit (build/commons.js:162037:10)
        at emitLoaded (build/commons.js:161814:12)

Seems completely unrelated to my changes...

vincentfretin · 2023-02-23T10:29:41Z

The error doesn't seem to be related to your changes indeed. My guess is that

aframe/tests/components/sound.test.js

Line 10 in ddba34f

el.sceneEl.addEventListener('loaded', function () { done(); });

wasn't called because the scene was already loaded. Those tests are using entityFactory helper. Other tests are using the elFactory helper that proper check for scene.hasLoaded to call the done() function.

vincentfretin · 2023-02-23T10:33:33Z

You can change the setup for this test to be like this one I guess

aframe/tests/extras/primitives/primitives/a-cubemap.test.js

Lines 18 to 21 in ddba34f

    
           if (sceneEl.hasLoaded) { done(); } 
        
           sceneEl.addEventListener('loaded', function () { 
        
             done(); 
        
           });

diarmidmackenzie · 2023-02-23T10:36:11Z

I forced a re-run (making a trivial change to a comment in the test case was the only way I found to do this), and the tests work this time. So it seems we got unlucky with an intermittent failure.

I'll make th change you suggest as it seems it can't hurt, but if the test failure is intermittent, it's hard to know if we fixed it.

Problem is intermittent, so not possible to validate fix, but this fix should address a possible race condition, which may be what caused the failure.

Per this request: aframevr#5250 (comment)

vertexColors is now a boolean (since THREE.js r114 / A-Frame 1.1.0)

3ab0b69

diarmidmackenzie marked this pull request as ready for review February 22, 2023 15:37

dmarcos reviewed Feb 22, 2023

View reviewed changes

Move to boolean for vertexColors property, while maintaining back-com…

e3fcc88

…patibility

Update comments

88c6e39

diarmidmackenzie added 2 commits February 23, 2023 10:40

Attempt to fix intermittent problem observed in sound UT

b2161a7

Problem is intermittent, so not possible to validate fix, but this fix should address a possible race condition, which may be what caused the failure.

Remove vertexColors entirely in favor of vertexColorsEnabled.

a26bf5c

Per this request: aframevr#5250 (comment)

dmarcos merged commit 3acb6cb into aframevr:master Feb 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vertexColors is now a boolean (since THREE.js r114 / A-Frame 1.1.0) #5250

vertexColors is now a boolean (since THREE.js r114 / A-Frame 1.1.0) #5250

diarmidmackenzie commented Feb 22, 2023 •

edited

Loading

dmarcos Feb 22, 2023

vincentfretin Feb 23, 2023

diarmidmackenzie Feb 23, 2023

diarmidmackenzie Feb 23, 2023

vincentfretin Feb 23, 2023

diarmidmackenzie Feb 23, 2023

dmarcos Feb 24, 2023

diarmidmackenzie Feb 25, 2023

diarmidmackenzie Feb 25, 2023

dmarcos Feb 26, 2023

vincentfretin commented Feb 23, 2023

diarmidmackenzie commented Feb 23, 2023

diarmidmackenzie commented Feb 23, 2023

vincentfretin commented Feb 23, 2023

vincentfretin commented Feb 23, 2023

diarmidmackenzie commented Feb 23, 2023

vertexColors is now a boolean (since THREE.js r114 / A-Frame 1.1.0) #5250

vertexColors is now a boolean (since THREE.js r114 / A-Frame 1.1.0) #5250

Conversation

diarmidmackenzie commented Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincentfretin commented Feb 23, 2023

diarmidmackenzie commented Feb 23, 2023

diarmidmackenzie commented Feb 23, 2023

vincentfretin commented Feb 23, 2023

vincentfretin commented Feb 23, 2023

diarmidmackenzie commented Feb 23, 2023

diarmidmackenzie commented Feb 22, 2023 •

edited

Loading