#28440 Fixed BufferGeometryUtils.mergeVertices to handle morphAttributes #28441
Conversation
| @@ -720,7 +722,7 @@ function mergeVertices( geometry, tolerance = 1e-4 ) { | |||
|
|
|||
| const tmpAttribute = tmpAttributes[ name ]; | |||
|
|
|||
| result.setAttribute( name, new BufferAttribute( | |||
| result.setAttribute( name, new tmpAttribute.constructor( | |||
There was a problem hiding this comment.
Replaced everywhere in mergeVertices new BufferAttribute with new <oldThing>.constructor to preserve eventual optimization that the specialized class might have.
There was a problem hiding this comment.
If someone would use Float16BufferAttribute, the new approach is required in any event. It is definitely more safe.
| @@ -0,0 +1,60 @@ | |||
| import { BufferGeometry } from '../../../../src/core/BufferGeometry.js'; | |||
There was a problem hiding this comment.
I understand that there is no maintenance for testing things outside src folder. However, I think adding tests for addons when possible would contribute to their stability.
There was a problem hiding this comment.
If you want to add your unit test, you have to revert some parts of #23352.
- Create an
addons/utilsfolder inunitand place your tests there. - Create
test/unit/three.addons.unit.jsand addBufferGeometryUtils.tests.js. - Introduce a new script
test-unit-addonsinpackage.jsonwhich should look like so:
"test-unit-addons": "qunit -r failonlyreporter -f !-webonly test/unit/three.addons.unit.js",
- Add the new script to the
testscript.
"test": "npm run lint && npm run test-unit && npm run test-unit-addons",
There was a problem hiding this comment.
Thanks a lot for the hints.
I read the issue related to that commit and I understand the motivation.
On another hand, when I look at ThreeJs I see it as a fully-fledged framework with all "batteries included".
I'm asking myself, what would ThreeJS be without FBX/GLTF loaders, without OrbitControls and all that super useful stuff that lives in addons.
IMO it would be like a car made of the engine only (no wheels, not anything else) :)
This is not for me to decide the importance of "addons" but I thank you for allowing adding back the means to test examples/jsm stuff.
There was a problem hiding this comment.
Done, I moved the test location as suggested.
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
| @@ -58,6 +58,7 @@ | |||
| "lint": "npm run lint-core", | |||
| "lint-fix": "npm run lint-core -- --fix && npm run lint-addons -- --fix && npm run lint-examples -- --fix && npm run lint-docs -- --fix && npm run lint-editor -- --fix && npm run lint-playground -- --fix && npm run lint-manual -- --fix && npm run lint-test -- --fix && npm run lint-utils -- --fix", | |||
| "test-unit": "qunit -r failonlyreporter -f !-webonly test/unit/three.source.unit.js", | |||
| "test-unit-addons": "qunit -r failonlyreporter -f !-webonly test/unit/three.addons.unit.js", | |||
There was a problem hiding this comment.
@mrdoob Are you fine with reactivating unit tests for addons?
There was a problem hiding this comment.
I'm personally in favor of having unit tests for addons. In several cases I've found it more more difficult to test changes (GLTFExporter and BufferGeometryUtils for example) without them, and would be easier with unit tests.
That said ... deleting the addons unit tests back in #23352 was a pretty big change! I feel like we should at least open an issue to address why we're adding them back, and what (if anything?) should be different this time. Without that I worry that new unit tests could be deleted again, and this isn't an encouraging outcome for contributors volunteering on test coverage.
There was a problem hiding this comment.
I created this one: #28452
There was a problem hiding this comment.
At the end of the day it's all about how much value they bring versus how much maintenance and discussions they cost.
There was a problem hiding this comment.
As a note perhaps for some future PR, I think we could improve performance of this method a bit by doing a first pass over the geometry to build a mapping (stored as Uint32Array) from vertex index in the input geometry to vertex index in the output geometry, incrementing the number of unique vertices as we go. Then in the second pass we could write the destination geometry, without needing the larger intermediate tmp allocations.
But, not really related to this PR, which looks good to me — thank you for the report and the fix!
Fixed #28440
Description
Reverted
morphAttrto be a collection of BufferAttributes from being currently a BufferAttribute by itself.