-
Notifications
You must be signed in to change notification settings - Fork 26
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
feat: add support for linear and constant point scaling via a new property called pointScaleMode
#194
Conversation
…constant keeps point size constant, default keeps the current smart scaling strategy
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.
Thanks for taking a stab at different point scale functions. 🎉 The scaling modes look fine but we need to make a couple changes to ensure good performance.
Apart from that there are several missing aspects before I can merge the PR and cut a new release:
- The new
pointScaleMode
property needs to be documented inREADME.md
- The new
pointScaleMode
property needs to be typed intypes.d.ts
- The new
pointScaleMode
property needs to be changeable via theset
function and gettable via theget
function. - The
CHANGELOG.md
needs to be updated. Since this is a new feature, I suggest adding the change a new headline for1.11.0
. - The new scaling functions need to be tested. Unlike Jupyter Scatter, regl-scatterplot is rigorously tested ;)
I can help with some of this over the next days.
@@ -77,7 +77,7 @@ const scatterplot = createScatterplot({ | |||
pointSize, | |||
showReticle, | |||
reticleColor, | |||
lassoInitiator: true, |
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.
Let's revert this unnecessary change
@@ -502,6 +502,7 @@ | |||
</head> | |||
|
|||
<body> | |||
<div>test</div> |
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.
Please remove this test code :)
if (pointScaleMode === 'proportional') { | ||
return camera.scaling[0] * window.devicePixelRatio; | ||
} | ||
if (pointScaleMode === 'constant') { | ||
return window.devicePixelRatio; | ||
} | ||
if (pointScaleMode === 'default') { | ||
if (camera.scaling[0] > 1) { | ||
return ( | ||
(Math.asinh(max(1.0, camera.scaling[0])) / Math.asinh(1)) * | ||
window.devicePixelRatio | ||
); | ||
} | ||
return max(minPointScale, camera.scaling[0]) * window.devicePixelRatio; |
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.
While this technically works, it's not the most performant way. Every time we call getPointScale()
we would do the string comparison against pointScaleMode
even though pointScaleMode
is almost guaranteed to always be the same. This is particularly tricky because getPointScale
is called very frequently when one zooms in or out.
A more efficient way would be to define three functions and assign either one to getPointScale
according to pointScaleMode
.
const getConstantPointScale = () => {
return window.devicePixelRatio;
}
const getLinearPointScale = () => {
return max(minPointScale, camera.scaling[0]) * window.devicePixelRatio;
}
const getAsinhPointScale = () => {
if (camera.scaling[0] > 1) {
return (
(Math.asinh(max(1.0, camera.scaling[0])) / Math.asinh(1)) *
window.devicePixelRatio
);
}
return max(minPointScale, camera.scaling[0]) * window.devicePixelRatio;
};
let getPointScale = getAsinhPointScale;
if (pointScaleMode === 'linear') {
getPointScale = getLinearPointScale;
}
if (pointScaleMode === 'constant') {
getPointScale = getConstantPointScale;
}
Some other notes: the linear scale must still respect minPointScale
.
pointScaleMode
I made a bunch of changes to address most outstanding issues. I'll commit them later today or tomorrow |
Thanks for addressing most of the outstanding issues! This is my first time working with javascript, so also thanks for the warm and encouraging response :) |
I can't figure out how to push to your branch so I'll merge your branch into a new one I just created and then I'll push my changes onto the new branch :) |
…perty called `pointScaleMode` (#195) * feat: add support for linear and constant point scaling via a new property called `pointScaleMode` (#194) * add pointScaleMode: proportional scales points proportional to zoom, constant keeps point size constant, default keeps the current smart scaling strategy * keep default mode to default; formatting --------- Co-authored-by: basta <[email protected]> * fix: remove nonsense * refactor: ensure `getPointScale()` runs as fast as possible * docs: update changelog * docs: document and type `pointScaleMode` * chore: revert accidentally removed comma * fix: allow changing scale mode interactively * test: add a test for point scale mode --------- Co-authored-by: abast <[email protected]> Co-authored-by: basta <[email protected]>
This introduces two new modes:
(1) point size is kept constant, independent of zoom
(2) point size is proportional to zoom.
Fixes #169
Checklist
CHANGELOG.md
updatedREADME.md
added or updated