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

Fix: prevent crash when calling BufferGeometry.setFromPoints with too few points #29956

Merged
merged 4 commits into from
Nov 25, 2024

Conversation

rotu
Copy link
Contributor

@rotu rotu commented Nov 24, 2024

Description

#29696 introduced a crash condition where calling BufferGeometry.setFromPoints would cause a crash like three.module.js:10894 Uncaught TypeError: Cannot read properties of undefined (reading 'x') if trying to set fewer points than is in the position attribute.

@rotu
Copy link
Contributor Author

rotu commented Nov 24, 2024

It may be appropriate to also emit a warning or to also set the draw range to only the newly set positions.

Copy link

github-actions bot commented Nov 24, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 339.14
78.99
339.14
78.99
+0 B
+0 B
WebGPU 483.15
134.15
483.15
134.15
+0 B
+0 B
WebGPU Nodes 482.62
134.06
482.62
134.06
+0 B
+0 B

🌳 Bundle size after tree-shaking

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

Before After Diff
WebGL 464.59
111.96
464.62
111.98
+25 B
+17 B
WebGPU 552.25
149.49
552.28
149.51
+25 B
+17 B
WebGPU Nodes 508.13
139.2
508.16
139.22
+25 B
+17 B

@Mugen87 Mugen87 added this to the r171 milestone Nov 25, 2024
@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 25, 2024

It may be appropriate to also emit a warning or to also set the draw range to only the newly set positions.

If the developer intentionally wants to update just a part of the position buffer but rendering the entire range, updating draw range or emitting a warning would be confusing though. I think the PR is fine as it is.

@Mugen87 Mugen87 merged commit add7f9b into mrdoob:dev Nov 25, 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.

2 participants