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

#28440 Fixed BufferGeometryUtils.mergeVertices to handle morphAttributes #28441

Conversation

catalin-enache
Copy link
Contributor

@catalin-enache catalin-enache commented May 20, 2024

Fixed #28440

Description

Reverted morphAttr to be a collection of BufferAttributes from being currently a BufferAttribute by itself.

@@ -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(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced everywhere in mergeVertices new BufferAttribute with new <oldThing>.constructor to preserve eventual optimization that the specialized class might have.

Copy link
Collaborator

@Mugen87 Mugen87 May 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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';
Copy link
Contributor Author

@catalin-enache catalin-enache May 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@Mugen87 Mugen87 May 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to add your unit test, you have to revert some parts of #23352.

  • Create an addons/utils folder in unit and place your tests there.
  • Create test/unit/three.addons.unit.js and add BufferGeometryUtils.tests.js.
  • Introduce a new script test-unit-addons in package.json which should look like so:
"test-unit-addons": "qunit -r failonlyreporter -f !-webonly test/unit/three.addons.unit.js",
  • Add the new script to the test script.
"test": "npm run lint && npm run test-unit && npm run test-unit-addons",

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, I moved the test location as suggested.

Copy link

github-actions bot commented May 20, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
679.1 kB (168.3 kB) 679.1 kB (168.3 kB) +0 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
457.1 kB (110.4 kB) 457.1 kB (110.4 kB) +0 B

@@ -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",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mrdoob Are you fine with reactivating unit tests for addons?

Copy link
Collaborator

@donmccurdy donmccurdy May 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created this one: #28452

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the end of the day it's all about how much value they bring versus how much maintenance and discussions they cost.

@Mugen87 Mugen87 added this to the r165 milestone May 20, 2024
@Mugen87 Mugen87 requested a review from donmccurdy May 20, 2024 11:07
Copy link
Collaborator

@donmccurdy donmccurdy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@Mugen87 Mugen87 merged commit 9e00010 into mrdoob:dev May 24, 2024
12 checks passed
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.

BufferGeometryUtils.mergeVertices crashes when feeding a geometry with morphAttributes (regression)
4 participants