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(demo): fix delaunay-voronoi demo #1758

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kelvin27315
Copy link

🐛 Bug Fix

  • Fixed the bug where hovering the mouse over the 0th point does not set the ID.
  • Fixed the issue where an "invalid bounds" error occurs when creating a Voronoi diagram with negative values for innerWidth or innerHeight.

Removed closest from the if statement as const closest = voronoiDiagram.delaunay.find(point.x, point.y); always returns the index of the nearest point, with the find(x: number, y: number, i?: number): number; signature ensuring a value is always present.

Copy link
Collaborator

@williaster williaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @kelvin27315 🙏 I had a couple of comments

@@ -71,7 +73,7 @@ function Example({ width, height, margin = defaultMargin }: VoronoiProps) {

const closest = voronoiDiagram.delaunay.find(point.x, point.y);
// find neighboring polygons to hightlight
if (closest && data[closest].id !== hoveredId) {
if (data[closest].id !== hoveredId) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is find guaranteed to return something? if closest could be undefined, this would then throw an error when evaluating data[closest].id

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the voronoiDiagram.delaunay.find(point.x, point.y); line, the find method of the Delaunay class from d3-delaunay is being used, which is defined as follows in @types/d3-delaunay, guaranteeing that a number type will always be returned according to the type definition:

/**
 * Returns the index of the input point that is closest to the specified point ⟨x, y⟩.
 * The search is started at the specified point i. If i is not specified, it defaults to zero.
 */
find(x: number, y: number, i?: number): number;

DefinitelyTyped Definition

Furthermore, the actual d3-delaunay implementation is as follows: if ((x = +x, x !== x) || (y = +y, y !== y)) return -1; checks if x and y are numbers, and returns -1 if they are not. However, since this Demo is adopting TypeScript, I believe there should be no issue as long as it adheres to TypeScript's type checking.

find(x, y, i = 0) {
  if ((x = +x, x !== x) || (y = +y, y !== y)) return -1;
  const i0 = i;
  let c;
  while ((c = this._step(i, x, y)) >= 0 && c !== i && c !== i0) i = c;
  return c;
}

d3-delaunay Implementation

Additionally, this delaunay.find is already used as follows in the demo of visx-delaunay-triangulation, and there isn’t a check for undefined there.

const closest = delaunayDiagram.find(point.x - margin.left, point.y - margin.top);
setHoveredId(data[closest].id);

visx-delaunay-triangulation Demo

@@ -37,6 +37,8 @@ function Example({ width, height, margin = defaultMargin }: VoronoiProps) {
const innerWidth = width - margin.left - margin.right;
const innerHeight = height - margin.top - margin.bottom;

if (innerWidth < 0 || innerHeight < 0) return null;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an early return statement will break the rules of hooks, where they would then be called conditionally/a different # of times per render.

maybe a different approach (since we already return null if width < 10), is to add something like const innerWidth = Math.max(0, width - margin.left - margin.right)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely forgot about the rules of hooks. 😅
I've pushed a commit reflecting your comment here. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants