-
Notifications
You must be signed in to change notification settings - Fork 0
refactor: do not premultiply point sizes, improve point discarding #51
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #51 +/- ##
===========================================
+ Coverage 4.41% 100.00% +95.58%
===========================================
Files 77 5 -72
Lines 4324 71 -4253
Branches 112 17 -95
===========================================
- Hits 191 71 -120
+ Misses 4098 0 -4098
+ Partials 35 0 -35
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Pull request overview
This PR refactors the point rendering system to avoid premultiplying point sizes on the CPU, moving size calculations to the GPU shader instead. This improves the separation of concerns and allows the shader to handle size transformations more accurately.
Changes:
- Removed
sizeFactorparameter fromLoadUtils.loadSizeData()to store raw size values without premultiplication - Added new uniforms to the shader for viewport size, canvas size, and device pixel ratio to enable proper size calculations
- Modified the WebGL controller to pass size factors separately in a uniform buffer object instead of premultiplying them into size data
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| packages/@tissuumaps-core/src/utils/LoadUtils.ts | Removed sizeFactor parameter and all premultiplication logic from size data loading |
| packages/@tissuumaps-core/src/controllers/WebGLPointsController.ts | Added new uniform locations, modified buffer layout to store size factors separately, updated state tracking |
| packages/@tissuumaps-core/src/assets/shaders/points.vert | Added new uniforms, implemented size calculations in shader, improved point discarding logic with viewport culling |
Comments suppressed due to low confidence (1)
packages/@tissuumaps-core/src/controllers/WebGLPointsController.ts:629
- Incorrect buffer target for loading uniform buffer data. The code is using
this._gl.ARRAY_BUFFERas the target when loading data intothis._buffers.objectsUBO, but this buffer should be bound asthis._gl.UNIFORM_BUFFERsince it's a uniform buffer object (UBO). While WebGL may be lenient about this, it's semantically incorrect and could lead to issues or confusion.
WebGLUtils.loadBuffer(
this._gl,
this._gl.ARRAY_BUFFER,
this._buffers.objectsUBO,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.