-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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 WebGL vertex property issue #7605
base: dev-2.0
Are you sure you want to change the base?
Fix WebGL vertex property issue #7605
Conversation
for (const { point, dir, color } of potentialCaps.values()) { | ||
this._addCap(point, dir, color); | ||
this._addCap(point, dir, color); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to how functions like addCap
, addSegment
, etc copy the positions and colors into new buffers, I think we'd have to do the same for vertex properties. The reason for this is that while the data is initially recorded per point on the line, those points then get turned into triangles to give the line thickness, so we end up having to repeat the data a few times for each point in the triangle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll update the implementation
Could you explain a bit what the the projectionMatrix, viewMatrix, and modelMatrix changes are? I didn't see code in the Files Changed tab relating to those at first glance. |
You're right, I mistakenly mentioned those matrices in the PR description, but they weren’t actually changed. The focus of this PR is fixing the WebGL vertex property issue. I'll update the description. Thanks for pointing it out! And for now I’ll update the implementation to copy vertex properties into new buffers, similar to how |
hey @davepagurek i've updated the pr. The issue with vertex properties has been resolved, and manual testing confirms the fix. |
Resolves #7519
Changes:
dev-2.0
branch.PR Checklist
npm run lint
passes