-
Notifications
You must be signed in to change notification settings - Fork 19
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
Implement useGeometry
hook to ease creation/update of geometries
#1515
Conversation
a3455e6
to
4b62388
Compare
ordinateScale, | ||
ignoreValue, | ||
}, | ||
hasR3FEventHandlers(pointsProps), |
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.
This detects whether the component received any R3F event handlers as props. When that's the case, we pass true
for the isInteractive
argument so geometry's bounding sphere gets recomputed on update.
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.
Is this really needed ? I guess it would not be a huge performance dent to recompute the bounding sphere even if it is not interactive.
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.
Hehe, unfortunately it is. Reproduction:
- Comment out this line
- Start Storybook and expand the sidebar to fill half the screen
- Navigate to the DataCurve > Interactive story — notice that all points are interactive
- Shrink the sidebar back to its original size — notice that the points at the start and end of the curve are no longer interactive.
What happens is, when the bounding sphere is not computed, Three computes it automatically on first paint. Then, when we update the geometry and repaint, the bounding sphere still exists so Three doesn't know it needs to be recomputed.
For a bit more context: when an interaction occurs, Three checks to see which objects on the scene interacts with the "ray". To speed this up, it first checks if the ray intersects the bounding sphere of each object's geometry. If the bounding sphere doesn't cover the whole geometry and a ray occurs outside of it, then the raycaster will discard the object as a potential intersection candidate very quickly.
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.
The above is one solution though. The advantage is that it just works for components like Line
and Glyphs
; the user doesn't have to know about computeBoundingSphere
at all. The downside is that it's a bit magical/implicit.
Another option would be to let users call computeBoundingSphere
themselves, perhaps by providing an onAfterUpdate
hook (otherwise, they'd have to write a useLayoutEffect
with the same dependencies as inside useGeometry
...)
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.
Great improvement !
I only noticed that errors are not displayed in the Story Data Curve -> With Errors ?
ordinateScale, | ||
ignoreValue, | ||
}, | ||
hasR3FEventHandlers(pointsProps), |
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.
Is this really needed ? I guess it would not be a huge performance dent to recompute the bounding sphere even if it is not interactive.
This is a big one, even though I've tried to keep a few things out of it.
Goal
The goal is to make it more straightforward for us internally but also for consumers of
@h5web/lib
to implement and use custom buffer geometries (like maybe thick/dashed lines that don't change while zooming). Attn @PeterC-DLS, as this might be of interest to you for Davidia.Custom buffer geometries are useful to avoid blowing up the bundle and increasing maintenance cost by adding
three-stdlib
anddrei
as (peer) dependencies to@h5web/lib
and consumer applications. Moreover, Three's/drei's built-in geometry code can be:index
buffer ... and even a third time insidearrayNeedsUint32
;needsUpdate
, etc.Implementation
The main entry point is a new hook called
useGeometry
, which accepts:H5WebGeometry
(see below);dataLength
);H5WebGeometry
is an abstract class that inheritsBufferGeometry
and specifies two methods:prepare
andupdate
.useGeometry
creates a new instance of the givenH5WebGeometry
sub-class. The sub-class's constructor typically takes care of initialising the buffer attributes.useGeometry
updates the geometry instance as follows: first, it calls theprepare
method to store the updated parameters in the geometry instance; then it starts afor
loop that calls theupdate
methoddataLength
times. It is then up to theupdate
method to update the content of the various buffer attributes in the geometry.Justification
With this approach geometries are fully independent from one another. We no longer try to over-optimise things by sharing buffer attributes across geometries or by coupling lots of unrelated computations inside the same loop, which led to code that was difficult to read and impossible to abstract for re-use.
Sure, we end up with more buffers in memory in some cases (like when displaying both the line and points with
DataCurve
/LineVis
), but we gain in code readability and make the geometry code much easier to refactor in the future (for instance to fix the subtle glitches when drawing lines that include points with non-finite coordinates, which are moved to(0, 0, CAMERA_FAR)
).My initial idea was to implement a
useGeometries
hook that allowed updating multiple geometries within a single loop. However, this required having a small loop (to iterate over the geometries) inside a big loop (to iterate over the data), and I realised that this had no theoretical performance benefit over doing the big loop multiple times in a row (i.e. via multiple, consecutive calls touseGeometry
).Moreover,
useGeometry
(rather thanuseGeometries
) allows having re-usable components likeLine
,Glyphs
andErrorBars
that each create/update their own geometries independently from one another. As a result,DataCurve
is now just a dumb component that helps reduce duplication inLineVis
.In this PR
useGeometry
hook andH5WebGeometry
abstract class (documented inUtilities.mdx
).DataCurve
into a dumb component:Line
,LineGeometry
Glyphs
,GlyphsGeometry
ErrorBars
,ErroBarsGeometry
,ErrorCapsGeometry
ScatterPointsGeometry
(used inScatterPoints
)SurfaceMeshGeometry
(used inSurfaceMesh
)createBufferAttribute
andcreateIndex
(documented inUtilities.mdx
).Naming decisions
I wasn't sure about the naming of the geometries and the components in which they are used, because:
SurfaceMeshGeometry
is passed to both<points>
and<mesh>
);GlyphsGeometry
would work find with<points>
's default material,PointsMaterial
, instead ofGlyphMaterial
).As a result, I decided to use the names of the components in which they are used - i.e.
SurfaceMesh
,ScatterPoints
,Line
, etc. Was this a good decision?If it was, are the names of the new components,
Line
,Glyphs
,ErrorBars
, appropriate?Line
/LineGeometry
kind of follows Three's naming (<line>
,<lineBasicMaterial>
), but obviously conflicts with it ... and somewhat withSvgLine
too. It's perhaps too generic.Glyphs
/GlyphsGeometry
are named after the existingGlyphMaterial
, but as mentioned, the geometry itself would work perfectly fine withPointsMaterial
and others (for instance, I used it inSurfaceMesh
at some point to display the points when debugging).I wondered about using the names
LineCurve
/LineCurveGeometry
andGlyphCurve
/GlyphCurveGeometry
, for instance.